diff --git a/.changeset/22161-lint-slice-3-one-line.md b/.changeset/22161-lint-slice-3-one-line.md new file mode 100644 index 00000000000..dab08ca60cd --- /dev/null +++ b/.changeset/22161-lint-slice-3-one-line.md @@ -0,0 +1,18 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): 14 more author-time findings print one verdict line, and `os explain ` carries their reasoning + +Clause-②: no + +- **Shorter verdicts.** Each finding of these 14 rule ids now prints a `message` of one verdict sentence. Every finding the rules' own test suites fire, and the CLI suite's handler-hook variants, is at most 200 characters; before, the longest of each ran from 212 to 792 characters. The ids: + - hook bodies: `hook-body-write-unknown-field`, `hook-body-write-unprovisioned-anchor`, `hook-body-source-unparseable`; + - action bodies: `action-body-write-unknown-field`, `action-body-write-unprovisioned-anchor`, `action-record-write-discarded`, `action-body-source-unparseable`; + - flow nodes: `flow-node-write-unknown-field`, `flow-node-write-unprovisioned-anchor`; + - readonly writes: `flow-update-readonly-field`, `flow-update-readonly-when-field`, `hook-api-update-readonly-field`, `hook-api-update-readonly-when-field`, `action-api-update-readonly-when-field`. + + The three write surfaces now share one wording per question: an undeclared field ends on "so the write is refused (INVALID_FIELD / 400)", an unprovisioned anchor reads "'FIELD' is an injected column with no storage on external object 'OBJECT', so … can never land", an unparseable hook or action body reads "L2 body did not parse (…), so writes in its unread part go unchecked", and a readonly write says it is "silently stripped" (a `readonlyWhen` field "where its predicate is TRUE"). The values the author wrote still close each verdict — the field, the object, the `ctx.api` call, the run identity — so a verdict over a long name grows with it, and a hook lowered from an inline `handler` keeps its " (judged on the metadata body lowered from the inline handler)" suffix. The `fix` (the CLI's `fix:` line, the runtime issue's `hint`), every rule id, severity and `path`, and what each rule accepts or refuses are unchanged. A tool that matched the old message text should match on `rule` and `path` instead. +- **`os explain ` takes these 14 ids**, for example `os explain hook-body-write-unknown-field`. It prints the reasoning the verdicts no longer carry: how each write channel reaches the engine's declared-field door and what the refusal takes down with it, why a write to an unprovisioned anchor on an external object passes the validator and never lands, what a partially parsed body leaves unchecked, why an action's `ctx.record` is never written back, and which readonly strip a system context waives and which it does not. The paragraphs the three surfaces share are one text, printed under every id they explain. The `rule:` line under each of these findings now ends with `` — `os explain ` for … ``. The no-argument listing and its `--json` `rules` array list the 14 ids, and so does the unknown-id error's `Rules with an explanation:` line. `RULE_EXPLANATIONS` in `@objectstack/lint` gains the 14 entries. +- **Where the new text prints.** On the CLI, `os validate`, `os build` (and `os compile`, which `os dev` runs on every compile), `os lint`, `os verify` and `os init`'s scaffold check print the new `message` on the text face, and `os validate --json` and `os build --json` carry it in their `warnings` and author-time `issues`. At the runtime publish gate (Studio, REST `/meta`, MCP), a `flow` write carries the four flow ids: `flow-node-write-unknown-field` and `flow-update-readonly-field` are errors, so the 422 issue's `message` and the refusal log line under `OS_ALLOW_UNLINTED_METADATA_WRITES` change; `flow-node-write-unprovisioned-anchor` and `flow-update-readonly-when-field` are warnings, so the `message` in the 2xx response's `advisories` and the deduped `[Protocol] authoring advisory` server log line change. Each issue's `hint` is unchanged. +- **Never at the runtime gate:** the ten hook and action ids. A `hook` or `action` write does not dispatch these rules (they parse authored JavaScript, which the publish path never loads), and a `flow` write's snapshot carries no hook or action body for them to read; they speak only on the CLI doors above. diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index 8e90b745b7a..378d31296ef 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -756,6 +756,294 @@ const VISIBILITY_BARE_IDENTIFIER_EXPLANATION: RuleExplanation = { ], }; +// ── Record writes (the body, flow-node and readonly write rules) ──────────── +// Three surfaces write a record's fields by name: an L2 (`language: 'js'`) +// hook body, an L2 action body, and the `fields` map of a flow +// `create_record` / `update_record` node. Three families of rules ask the same +// questions of all three — does the field exist (`validate-hook-body-writes.ts`, +// `validate-action-body-writes.ts`, `validate-flow-node-writes.ts`), does it +// have storage, and is it writable through this channel +// (`validate-readonly-{flow,hook,action}-writes.ts`) — so the reasoning they +// share is written ONCE, as the constants below that every entry it explains +// references, and the families cannot drift apart. The rule files share their +// verdict clauses the same way. + +/** Why the engine refuses a write that names an undeclared field — the three undeclared-field ids. */ +const DECLARED_FIELD_DOOR = + 'The engine\'s declared-field door judges every caller-supplied write payload against the object\'s ' + + 'declared fields and refuses a key the object does not declare: INVALID_FIELD / 400, identically on ' + + 'every driver, before any statement is built. The whole payload is refused, so the correctly named ' + + 'fields beside the bad key never land either. The refusal arrives at run time, on whichever record ' + + 'first exercises the write and far from the line that made the mistake, which is why the rule ' + + 'reports it at author time, naming the field, the object and the write.'; + +/** The cause and the consequence of writing an unprovisioned anchor — the three anchor ids. */ +const UNPROVISIONED_ANCHOR_WRITE: readonly string[] = [ + 'The registry injects system columns — the ownership anchors `owner_id` and `organization_id`, the ' + + 'audit family — into every object. On an ADR-0015 external object the remote database owns the ' + + 'schema, so the platform registers these anchors without provisioning a column: the name resolves, ' + + 'but no storage stands behind it.', + 'A write naming such an anchor can never land. The anchor exists only in the registered schema, which ' + + 'is what carries it PAST the write-path validator that refuses an undeclared name outright ' + + '(INVALID_FIELD); the remote database is what rejects it. On a SQL remote that is an untyped driver ' + + 'error (`no such column`) that aborts the whole statement, so the correctly named fields of the same ' + + 'payload never land either; on a schemaless remote the key is persisted into a column no read ' + + 'surface returns.', + 'Only a name that resolved BECAUSE it is injected is judged: an author-declared column of the same ' + + 'name maps a remote column the author vouches for and is never reported. The finding is a warning ' + + 'because the rule cannot see the remote table, only that the platform provisions no storage for the ' + + 'anchor.', +]; + +/** What a body that does not parse leaves unchecked — the two body-source ids. */ +const BODY_PARSE_FAILURE: readonly string[] = [ + 'The body is parsed — never executed, never type-checked — so its writes can be checked. A body with ' + + 'a syntax error is only partially recovered by the parser, so the write set the checks read comes ' + + 'from that recovered tree: a write in the part the parser could not read is judged by no rule.', + 'It is reported rather than skipped because the body would otherwise come back with nothing to ' + + 'report — the same silence the undeclared write itself has at run time, this time wearing the ' + + 'checker\'s badge. It is a warning: it says what the checker could read, not a second syntax verdict.', +]; + +/** The static `readonly` strip — the flow and hook static-readonly ids. */ +const READONLY_STATIC_STRIP = + 'The engine strips every static `readonly: true` field from a caller-supplied write payload, on ' + + 'UPDATE and on INSERT alike, unless the write runs in a system context. The strip is silent: the call ' + + 'or step still reports success, the rest of the payload lands, and on INSERT the column falls back to ' + + 'the field\'s `defaultValue`; only a run-time warning naming the dropped field records it. `readonly` ' + + 'governs the end-user and API surface, not trusted system writers, so a system context is the ' + + 'intended channel for maintaining such a field.'; + +/** The conditional `readonlyWhen` strip — the three readonlyWhen ids. */ +const READONLY_WHEN_STRIP: readonly string[] = [ + 'A `readonlyWhen` field is locked per record. On an UPDATE the engine strips it from the ' + + 'caller-supplied payload wherever its predicate is TRUE for the record being written over, and a ' + + 'bulk update strips it from every matched row once any one of them is locked. The strip is silent, ' + + 'so whether the write lands depends on the record\'s state, which is why the finding is a warning.', + 'Unlike the static `readonly` strip, the conditional lock is NOT waived by a system context, so ' + + 'elevation is no workaround. A value a `beforeUpdate` hook derives is not caller-supplied and does ' + + 'land, even on a locked record. An INSERT is never judged: there is no prior record for the ' + + 'predicate to read, and the engine runs no conditional strip there.', +]; + +/** How a flow CRUD node's `fields` map reaches the engine — the two flow readonly ids. */ +const FLOW_FIELDS_CALLER_PAYLOAD = + 'A `create_record` or `update_record` node hands its `fields` map to the engine as a caller-supplied ' + + 'payload, under the flow\'s run identity: `runAs`, which defaults to \'user\'.'; + +/** How a hook body's `ctx.api` write reaches the engine — the two hook readonly ids. */ +const HOOK_API_CALLER_PAYLOAD = + 'A hook\'s `ctx.api` is a scoped handle over the TRIGGERING operation\'s context, so on every ' + + 'non-system trigger a `ctx.api.object(NAME).update / updateById / insert(…)` payload is an ordinary ' + + 'caller-supplied one.'; + +const HOOK_BODY_WRITE_UNKNOWN_FIELD_EXPLANATION: RuleExplanation = { + rule: 'hook-body-write-unknown-field', + covers: 'why an undeclared field write is refused', + paragraphs: [ + 'A hook body writes a record\'s fields through two channels, and both reach the same door. A ' + + '`ctx.input.NAME = …` write mutates the triggering record: the sandboxed script runs clean, the ' + + 'value is copied back onto the record payload unfiltered, and the declared-field door runs a ' + + 'second time over the payload the `before*` hooks produced. A ' + + '`ctx.api.object(NAME).insert / update / updateById(…)` write is a second, nested engine call: ' + + '`ctx.api` is a scoped handle on the running engine, so its payload arrives as an ordinary CALLER ' + + 'write.', + DECLARED_FIELD_DOOR, + 'What fails depends on the channel. A `ctx.input` write takes the triggering record write down with ' + + 'it: the record is never written, and the refusal names the field far from the body that wrote it. ' + + 'A `ctx.api` write lands nothing, and its refusal escapes the body and fails the operation that ' + + 'triggered the hook.', + 'A `ctx.input` write is judged against the hook\'s target objects, and a multi-target hook is ' + + 'reported only when the field is missing on every one of them: the body may branch per object. ' + + 'What the parser cannot pin down is skipped silently — a dynamic object name, an `object: \'*\'` ' + + 'hook\'s input, a target another package declares — because a false positive costs an advisory ' + + 'rule more than a miss. The rule stays a warning: it reads the body through a parser, never by ' + + 'running it.', + ], +}; + +const HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION: RuleExplanation = { + rule: 'hook-body-write-unprovisioned-anchor', + covers: 'why a write to an unprovisioned anchor never lands', + paragraphs: [ + ...UNPROVISIONED_ANCHOR_WRITE, + 'On a hook body both write channels are judged. A `ctx.input` write is judged against the hook\'s ' + + 'target objects, and a multi-target hook is reported only when the anchor is unprovisioned on ' + + 'every one of them: the body may branch per object, so an anchor that is real on one target is a ' + + 'legitimate write there. A `ctx.api.object(NAME)` write is judged against the object it names.', + ], +}; + +const HOOK_BODY_SOURCE_UNPARSEABLE_EXPLANATION: RuleExplanation = { + rule: 'hook-body-source-unparseable', + covers: 'what an unparseable body leaves unchecked', + paragraphs: [ + ...BODY_PARSE_FAILURE, + 'The gating readonly rule on the same body (`hook-api-update-readonly-field`) skips an unparseable ' + + 'body rather than guess at what the unread part wrote, so this finding is the one that describes ' + + 'the problem.', + ], +}; + +const ACTION_BODY_WRITE_UNKNOWN_FIELD_EXPLANATION: RuleExplanation = { + rule: 'action-body-write-unknown-field', + covers: 'why an undeclared field write is refused', + paragraphs: [ + 'An action body persists records only through ' + + '`ctx.api.object(NAME).insert / create / update / updateById(…)`. `ctx.api` is a scoped handle on ' + + 'the running engine, so the payload arrives as an ordinary CALLER write. An action\'s `ctx.input` ' + + 'is its params bag, validated against the action\'s own `params`, so it is never resolved against ' + + 'fields.', + DECLARED_FIELD_DOOR, + 'The nested write lands nothing, and its refusal escapes the body and fails the action. Hook bodies ' + + 'get the same check (`hook-body-write-unknown-field`) through the same extractor, so the two rules ' + + 'judge the same write shape the same way.', + ], +}; + +const ACTION_RECORD_WRITE_DISCARDED_EXPLANATION: RuleExplanation = { + rule: 'action-record-write-discarded', + covers: 'why an assignment to the record snapshot is discarded', + paragraphs: [ + 'The runtime hands an action body a plain snapshot of the record as `ctx.record` and never writes it ' + + 'back: the handler returns the body\'s value and applies nothing to the record. So ' + + '`ctx.record.NAME = …` changes a copy that dies with the sandbox, whether or not NAME is a declared ' + + 'field, and the action still returns success. The snapshot stays read-only by design: an action\'s ' + + 'write channel is `ctx.api`.', + 'This is its own rule id rather than a case of `action-body-write-unknown-field` on purpose: ' + + 'reporting only the undeclared half would imply that a write to a declared field persists, the ' + + 'false completion the rule exists to stop.', + 'It is reported only when the write is provably dead: `ctx.record` never leaves the body as a ' + + 'value. Handed to anything — an argument, an assignment, a spread, a return — the snapshot may be ' + + 'a payload under construction, so every write in that body is skipped. An alias ' + + '(`const r = ctx.record`) counts as an escape; a property read does not.', + ], +}; + +const ACTION_BODY_SOURCE_UNPARSEABLE_EXPLANATION: RuleExplanation = { + rule: 'action-body-source-unparseable', + covers: 'what an unparseable body leaves unchecked', + paragraphs: [ + ...BODY_PARSE_FAILURE, + 'The readonly rule on the same body (`action-api-update-readonly-when-field`) skips an unparseable ' + + 'body rather than guess at what the unread part wrote, so this finding is the one that describes ' + + 'the problem.', + ], +}; + +const ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION: RuleExplanation = { + rule: 'action-body-write-unprovisioned-anchor', + covers: 'why a write to an unprovisioned anchor never lands', + paragraphs: [ + ...UNPROVISIONED_ANCHOR_WRITE, + 'On an action body only the `ctx.api.object(NAME)` write is judged: an action\'s `ctx.input` is its ' + + 'params bag, not a record, and its `ctx.record` is a snapshot the runtime never writes back ' + + '(`action-record-write-discarded`).', + ], +}; + +const FLOW_NODE_WRITE_UNKNOWN_FIELD_EXPLANATION: RuleExplanation = { + rule: 'flow-node-write-unknown-field', + covers: 'why an undeclared field write is refused', + paragraphs: [ + 'A `create_record` or `update_record` node hands its `fields` map to the data engine directly — the ' + + 'flow executor calls the engine\'s insert or update, not the metadata API — so the map arrives as an ' + + 'ordinary caller payload.', + DECLARED_FIELD_DOOR, + 'The node folds the refusal into a step failure, so the run fails. On `create_record` the row is ' + + 'never created at all, so every later node that expected the new record\'s id is working from a ' + + 'record that does not exist.', + 'This rule gates where the hook and action body rules advise: nothing here is parsed — the key and ' + + 'the object name are both literal metadata — so a finding is a certainty. Not judged: a templated ' + + 'object name (resolved from flow variables at run time), a non-literal `fields` map, a dotted key ' + + '(a nested path the document drivers forward verbatim) and an object another package declares. ' + + '`runAs` is not consulted: no run identity conjures a column.', + ], +}; + +const FLOW_NODE_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION: RuleExplanation = { + rule: 'flow-node-write-unprovisioned-anchor', + covers: 'why a write to an unprovisioned anchor never lands', + paragraphs: [ + ...UNPROVISIONED_ANCHOR_WRITE, + 'On a flow the `fields` map of a `create_record` or `update_record` node is judged against the ' + + 'node\'s literal object name. This finding is a warning where the node\'s undeclared-field finding ' + + 'is an error: that one is a certainty about this stack, this one is a claim about a remote schema ' + + 'the build cannot see.', + ], +}; + +const FLOW_UPDATE_READONLY_FIELD_EXPLANATION: RuleExplanation = { + rule: 'flow-update-readonly-field', + covers: 'why a readonly field write is silently dropped', + paragraphs: [ + FLOW_FIELDS_CALLER_PAYLOAD, + READONLY_STATIC_STRIP, + 'A `runAs: \'system\'` flow bypasses the static strip and legitimately maintains readonly fields ' + + '(users cannot edit them, automation does), so it is never reported here; the same flow is still ' + + 'judged for `readonlyWhen` fields, whose lock elevation does not waive. A `create_record` on a ' + + 'platform-internal object (an engine-owned, append-only or better-auth bucket, or a `sys_` name) ' + + 'is not judged: the engine runs no create-side strip there. The rule gates because a literal ' + + 'field name against a declared `readonly: true` is a certain no-op.', + ], +}; + +const FLOW_UPDATE_READONLY_WHEN_FIELD_EXPLANATION: RuleExplanation = { + rule: 'flow-update-readonly-when-field', + covers: 'why a readonlyWhen field write may not land', + paragraphs: [ + FLOW_FIELDS_CALLER_PAYLOAD, + ...READONLY_WHEN_STRIP, + 'So a `runAs: \'system\'` flow is judged here like any other, and the verdict names the run ' + + 'identity it was judged under. Only `update_record` is judged.', + ], +}; + +const HOOK_API_UPDATE_READONLY_FIELD_EXPLANATION: RuleExplanation = { + rule: 'hook-api-update-readonly-field', + covers: 'why a readonly field write is silently dropped', + paragraphs: [ + HOOK_API_CALLER_PAYLOAD, + READONLY_STATIC_STRIP, + 'Never reported, because it never reaches the strip: a hook that declares `runAs: \'system\'` (its ' + + '`ctx.api` gets a system context, so the write lands and the triggering user is still stamped on ' + + 'the record), and a `ctx.input.NAME = …` stamp in the record\'s own `beforeInsert` / `beforeUpdate` ' + + 'hook, a server value rather than a caller\'s, which survives the strip. The rule keys on the write ' + + 'channel, not the field, so that correct stamp is never touched. `ctx.api.sudo()` is no way out ' + + 'from a body: `sudo()` lives on the in-process scoped context and is not marshalled into the ' + + 'sandbox, so calling it is a TypeError at run time.', + 'The rule gates because both halves are declared in this stack: the field\'s `readonly` and the ' + + 'body\'s literal `ctx.api` write. `id` in an update payload is the write\'s address, not a field ' + + 'write, and is not judged; a body that does not parse is skipped, and its ' + + '`hook-body-source-unparseable` finding describes the problem.', + ], +}; + +const HOOK_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION: RuleExplanation = { + rule: 'hook-api-update-readonly-when-field', + covers: 'why a readonlyWhen field write may not land', + paragraphs: [ + HOOK_API_CALLER_PAYLOAD, + ...READONLY_WHEN_STRIP, + 'Neither `runAs: \'system\'` nor `ctx.api.sudo()` helps: a system context does not waive the ' + + 'conditional lock, and `sudo()` is not marshalled into the sandbox in any case.', + ], +}; + +const ACTION_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION: RuleExplanation = { + rule: 'action-api-update-readonly-when-field', + covers: 'why a readonlyWhen field write may not land', + paragraphs: [ + 'An action body\'s `ctx.api` runs elevated by design, in a system context. That exempts its writes ' + + 'from the STATIC `readonly` strip — a `readonly: true` field written there lands, so this surface ' + + 'has no static-readonly rule — but NOT from the conditional one.', + ...READONLY_WHEN_STRIP, + 'Only `ctx.api.object(NAME).update / updateById(…)` is judged. `ctx.record` is not a write surface ' + + '(`action-record-write-discarded` owns that shape), and `id` in an update payload is the write\'s ' + + 'address, not a field write.', + ], +}; + /** * Every rule explanation this package ships, keyed by rule id. `os explain * ` reads this table and nothing else. @@ -798,6 +1086,20 @@ export const RULE_EXPLANATIONS: Readonly> = Obje [VISIBILITY_PREDICATE_SYNTAX_EXPLANATION.rule]: VISIBILITY_PREDICATE_SYNTAX_EXPLANATION, [VISIBILITY_PREDICATE_UNKNOWN_FUNCTION_EXPLANATION.rule]: VISIBILITY_PREDICATE_UNKNOWN_FUNCTION_EXPLANATION, [VISIBILITY_BARE_IDENTIFIER_EXPLANATION.rule]: VISIBILITY_BARE_IDENTIFIER_EXPLANATION, + [HOOK_BODY_WRITE_UNKNOWN_FIELD_EXPLANATION.rule]: HOOK_BODY_WRITE_UNKNOWN_FIELD_EXPLANATION, + [HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION.rule]: HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION, + [HOOK_BODY_SOURCE_UNPARSEABLE_EXPLANATION.rule]: HOOK_BODY_SOURCE_UNPARSEABLE_EXPLANATION, + [ACTION_BODY_WRITE_UNKNOWN_FIELD_EXPLANATION.rule]: ACTION_BODY_WRITE_UNKNOWN_FIELD_EXPLANATION, + [ACTION_RECORD_WRITE_DISCARDED_EXPLANATION.rule]: ACTION_RECORD_WRITE_DISCARDED_EXPLANATION, + [ACTION_BODY_SOURCE_UNPARSEABLE_EXPLANATION.rule]: ACTION_BODY_SOURCE_UNPARSEABLE_EXPLANATION, + [ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION.rule]: ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION, + [FLOW_NODE_WRITE_UNKNOWN_FIELD_EXPLANATION.rule]: FLOW_NODE_WRITE_UNKNOWN_FIELD_EXPLANATION, + [FLOW_NODE_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION.rule]: FLOW_NODE_WRITE_UNPROVISIONED_ANCHOR_EXPLANATION, + [FLOW_UPDATE_READONLY_FIELD_EXPLANATION.rule]: FLOW_UPDATE_READONLY_FIELD_EXPLANATION, + [FLOW_UPDATE_READONLY_WHEN_FIELD_EXPLANATION.rule]: FLOW_UPDATE_READONLY_WHEN_FIELD_EXPLANATION, + [HOOK_API_UPDATE_READONLY_FIELD_EXPLANATION.rule]: HOOK_API_UPDATE_READONLY_FIELD_EXPLANATION, + [HOOK_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION.rule]: HOOK_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION, + [ACTION_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION.rule]: ACTION_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION, }); /** The explanation for `rule`, or `undefined` when the rule has none. Exact id match. */ diff --git a/packages/lint/src/validate-action-body-writes.test.ts b/packages/lint/src/validate-action-body-writes.test.ts index 9b9e90182ec..169c15b777d 100644 --- a/packages/lint/src/validate-action-body-writes.test.ts +++ b/packages/lint/src/validate-action-body-writes.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { - validateActionBodyWrites, + validateActionBodyWrites as validateActionBodyWritesUnrecorded, ACTION_BODY_WRITE_PATTERNS, ACTION_BODY_WRITE_PATTERN_IDS, ACTION_RECORD_WRITE_PATTERNS, @@ -18,6 +18,30 @@ import { HOOK_BODY_WRITE_PATTERNS, IMPLICIT_FIELDS, } from './validate-hook-body-writes.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the rule ids this file's rule shortened is one +// verdict sentence; the reasoning it used to carry is the id's `os explain` +// entry. Every call below records what it fired, and the last case in this +// file holds each recorded verdict of those ids to one line of at most 200 +// characters — so the pin covers every firing variant this suite exercises, +// not a chosen few. Run the whole file: that case reads what the cases above +// fired. +const SHORTENED_RULE_IDS: readonly string[] = [ + ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR, + ACTION_BODY_WRITE_UNKNOWN_FIELD, + ACTION_RECORD_WRITE_DISCARDED, + ACTION_BODY_SOURCE_UNPARSEABLE, +]; +const firedShortened: Array<{ rule: string; message: string }> = []; +const validateActionBodyWrites: typeof validateActionBodyWritesUnrecorded = (...args) => { + const findings = validateActionBodyWritesUnrecorded(...args); + for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f); + return findings; +}; + +/** The `os explain` text of `rule`, one string. */ +const explanationOf = (rule: string): string => explainRule(rule)?.paragraphs.join('\n') ?? ''; // Target objects: array-shaped and map-shaped `fields`, so both authoring // shapes are resolved. @@ -163,7 +187,7 @@ describe('validateActionBodyWrites — ctx.api writes', () => { expect(findings[0].path).toBe('actions[0].body.source'); expect(findings[0].message).toContain('discont_total'); expect(findings[0].message).toContain('crm_deal'); - expect(findings[0].message).toContain('INVALID_FIELD / 400, identically on every driver'); + expect(findings[0].message).toContain('INVALID_FIELD / 400'); expect(findings[0].hint).toContain("'discount_total'"); }); @@ -176,24 +200,30 @@ describe('validateActionBodyWrites — ctx.api writes', () => { // The old text promised a driver-level error on SQL and a persisted stray // key on schemaless — neither happens on this path, and has not since // #8682/#8738 put the declared-field door ahead of any statement. + // + // [#22161] The verdict names the refusal; the rest of the measured account + // is `os explain action-body-write-unknown-field`, so it is pinned there. it('states the measured refusal — INVALID_FIELD / 400 on every driver — and no driver split', () => { const [finding] = validateActionBodyWrites( stackWith("await ctx.api.object('crm_deal').update({ discont_total: 0 });"), ); + const explanation = explanationOf(ACTION_BODY_WRITE_UNKNOWN_FIELD); expect(finding.message).toContain('INVALID_FIELD / 400'); - expect(finding.message).toContain('identically on every driver'); - expect(finding.message).toContain('before any statement is built'); + expect(explanation).toContain('identically on every driver'); + expect(explanation).toContain('before any statement is built'); // The reason the door — not a driver — is what answers. - expect(finding.message).toContain('ordinary CALLER write'); - // The action-side blast radius, the one word that differs from the hook - // sibling's sentence. Pinned so a future sweep cannot flatten the two. - expect(finding.message).toContain('fails the action'); - - expect(finding.message).not.toMatch(/driver-level error/); - expect(finding.message).not.toMatch(/schemaless/); - expect(finding.message).not.toMatch(/is persisted/); - expect(finding.message).not.toMatch(/write-path validator skips/); + expect(explanation).toContain('ordinary CALLER write'); + // The action-side blast radius, the one sentence that differs from the + // hook sibling's explanation. Pinned so a future sweep cannot flatten the two. + expect(explanation).toContain('fails the action'); + + for (const text of [finding.message, explanation]) { + expect(text).not.toMatch(/driver-level error/); + expect(text).not.toMatch(/schemaless/); + expect(text).not.toMatch(/is persisted/); + expect(text).not.toMatch(/write-path validator skips/); + } }); it('checks insert/update payloads (argument 0) and updateById at argument 1', () => { @@ -203,7 +233,7 @@ describe('validateActionBodyWrites — ctx.api writes', () => { "await ctx.api.object('crm_deal').updateById(ctx.recordId, { stag: 'won' });", ), ); - expect(findings.map((f) => f.message.match(/writing '(\w+)'/)?.[1])).toEqual(['emial', 'stag']); + expect(findings.map((f) => f.message.match(/writing undeclared field '(\w+)'/)?.[1])).toEqual(['emial', 'stag']); expect(findings[0].message).toContain("ctx.api.object('crm_contact').insert"); expect(findings[1].message).toContain('updateById'); }); @@ -313,7 +343,10 @@ describe('validateActionBodyWrites — discarded ctx.record writes (#4345)', () expect(findings[0].where).toBe('action "close_deal" › body'); expect(findings[0].path).toBe('actions[0].body.source'); expect(findings[0].message).toContain('ctx.record.stage'); - expect(findings[0].message).toContain("The snapshot stays read-only by design: an action's write channel is ctx.api."); + // [#22161] Why the snapshot is not a write surface is the id's `os explain` text. + expect(explanationOf(ACTION_RECORD_WRITE_DISCARDED)).toContain( + "The snapshot stays read-only by design: an action's write channel is `ctx.api`.", + ); expect(findings[0].hint).toContain('updateById'); }); @@ -528,7 +561,7 @@ describe('[#8663] validateActionBodyWrites — unprovisioned anchor writes', () expect(findings[0].where).toBe('action "stamp_owner" › body'); expect(findings[0].path).toBe('actions[0].body.source'); expect(findings[0].message).toContain("'owner_id'"); - expect(findings[0].message).toContain('external object (ADR-0015)'); + expect(findings[0].message).toContain("external object 'wh_order'"); expect(findings[0].message).toContain('can never land'); }); @@ -592,3 +625,34 @@ describe('an unparseable action body is reported, not scored clean (#10653)', () } }); }); + +describe('[#22161] one-line verdicts — the rule ids this file shortened', () => { + it('every verdict the cases above fired for those ids is one line of at most 200 characters', () => { + // The coverage control first: each shortened id fired at least once, so + // the shape assertion below cannot pass over an empty record. + expect([...new Set(firedShortened.map((f) => f.rule))].sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + for (const f of firedShortened) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What each verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR]: ['external object', 'ADR-0015', 'PAST the write-path validator', 'no such column', 'schemaless remote'], + [ACTION_BODY_WRITE_UNKNOWN_FIELD]: ['scoped handle on the running engine', 'ordinary CALLER write', 'before any statement is built', 'fails the action'], + [ACTION_RECORD_WRITE_DISCARDED]: ['whether or not NAME is a declared field', 'read-only by design', 'provably dead'], + [ACTION_BODY_SOURCE_UNPARSEABLE]: ['partially recovered', 'judged by no rule', 'action-api-update-readonly-when-field'], + }; + + it('covers exactly the shortened ids', () => { + expect(Object.keys(MOVED).sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + }); + + it.each([...SHORTENED_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts index 0f88159a61a..134c72868e9 100644 --- a/packages/lint/src/validate-action-body-writes.ts +++ b/packages/lint/src/validate-action-body-writes.ts @@ -83,18 +83,16 @@ import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; -import { describeParseFailure, PARSE_FAILURE_HINT } from './checked-parse.js'; -import { - indexUnprovisionedAnchors, - unprovisionedAnchorCause, - unprovisionedAnchorHint, -} from './system-fields.js'; +import { PARSE_FAILURE_HINT } from './checked-parse.js'; +import { indexUnprovisionedAnchors, unprovisionedAnchorHint } from './system-fields.js'; import { + bodyParseFailureVerdict, extractHookBodyWriteSet, indexObjectFields, judgeableFieldsOf, IMPLICIT_FIELDS, - unprovisionedAnchorWriteConsequence, + undeclaredApiWriteVerdict, + unprovisionedAnchorWriteVerdict, HOOK_BODY_WRITE_PATTERNS, type BodyWritePatternExclusion, type HookBodyWritePattern, @@ -341,9 +339,8 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ rule: ACTION_BODY_SOURCE_UNPARSEABLE, where, path: site.path, - message: - `L2 body did not parse (${describeParseFailure(parseFailure)}), so its write set was read from a ` + - `partially recovered tree — an undeclared field write in the unread part is not reported.`, + // [#22161] The hook twin's sentence, from the one function both use. + message: bodyParseFailureVerdict(parseFailure), hint: PARSE_FAILURE_HINT, }); } @@ -365,11 +362,13 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ rule: ACTION_RECORD_WRITE_DISCARDED, where, path: site.path, + // [#22161] One verdict sentence; that the snapshot is read-only by + // design whether or not the field is declared, and why only a + // provably dead write is reported, is + // `os explain action-record-write-discarded`. message: - `body assigns ctx.record.${w.field}, but an action's ctx.record is a plain snapshot the runtime ` + - `never writes back — the action returns success and the assignment is discarded, whether or not ` + - `'${w.field}' is a declared field. The snapshot stays read-only by design: an action's ` + - `write channel is ctx.api.`, + `body assigns ctx.record.${w.field}, but an action's ctx.record is a snapshot the runtime never ` + + `writes back, so the assignment is discarded while the action returns success`, hint: `To persist it, write through the API: ctx.api.object('').updateById(ctx.recordId, ` + `{ ${w.field}: … }). Reported only because ctx.record is never passed anywhere in this body — ` + @@ -408,9 +407,7 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ rule: ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR, where, path: site.path, - message: - `body calls ctx.api.object('${w.object}').${w.method ?? 'update'}(…) writing '${w.field}', and ` + - `${unprovisionedAnchorCause(w.object, w.field)} — ${unprovisionedAnchorWriteConsequence()}`, + message: unprovisionedAnchorWriteVerdict(w.object, w.field, `the body's ${w.method ?? 'update'}(…) write`), hint: unprovisionedAnchorHint(w.object, w.field), }); continue; @@ -422,16 +419,12 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ rule: ACTION_BODY_WRITE_UNKNOWN_FIELD, where, path: site.path, - message: - `body calls ctx.api.object('${w.object}').${w.method ?? 'update'}(…) writing '${w.field}', but ` + - // [#13858] Same door, same measurement as the hook sibling — ctx.api - // is a ScopedContext over the running engine, so this payload is - // CALLER-supplied and #8682/#8738 refuse it before any driver. - `object '${w.object}' declares no such field. ctx.api is a scoped handle on the running ` + - `engine, so the payload arrives as an ordinary CALLER write and the declared-field door ` + - `REFUSES it at run time — INVALID_FIELD / 400, identically on every driver, before ` + - `any statement is built. The write lands nothing, and the refusal escapes the body and ` + - `fails the action.`, + // [#13858] Same door, same measurement as the hook sibling — ctx.api + // is a ScopedContext over the running engine, so this payload is + // CALLER-supplied and #8682/#8738 refuse it before any driver. + // [#22161] The hook sibling's verdict, from the one function both use; + // the reasoning is `os explain action-body-write-unknown-field`. + message: undeclaredApiWriteVerdict(w.object, w.method ?? 'update', w.field), hint: fixHint(w.field, [...known]), }); } diff --git a/packages/lint/src/validate-flow-node-writes.test.ts b/packages/lint/src/validate-flow-node-writes.test.ts index 98ed2a1ad7e..129bac12e79 100644 --- a/packages/lint/src/validate-flow-node-writes.test.ts +++ b/packages/lint/src/validate-flow-node-writes.test.ts @@ -9,13 +9,29 @@ import { } from '@objectstack/spec/automation'; import { - validateFlowNodeWrites, + validateFlowNodeWrites as validateFlowNodeWritesUnrecorded, FLOW_NODE_WRITE_UNKNOWN_FIELD, FLOW_NODE_WRITE_UNPROVISIONED_ANCHOR, FLOW_WRITE_NODE_TYPES, FLOW_WRITE_NODE_TYPES_DEFERRED, } from './validate-flow-node-writes.js'; import { IMPLICIT_FIELDS } from './validate-hook-body-writes.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the rule ids this file's rule shortened is one +// verdict sentence; the reasoning it used to carry is the id's `os explain` +// entry. Every call below records what it fired, and the last case in this +// file holds each recorded verdict of those ids to one line of at most 200 +// characters — so the pin covers every firing variant this suite exercises, +// not a chosen few. Run the whole file: that case reads what the cases above +// fired. +const SHORTENED_RULE_IDS: readonly string[] = [FLOW_NODE_WRITE_UNPROVISIONED_ANCHOR, FLOW_NODE_WRITE_UNKNOWN_FIELD]; +const firedShortened: Array<{ rule: string; message: string }> = []; +const validateFlowNodeWrites: typeof validateFlowNodeWritesUnrecorded = (...args) => { + const findings = validateFlowNodeWritesUnrecorded(...args); + for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f); + return findings; +}; // Target objects — map-shaped and array-shaped `fields`, so both authoring // shapes are resolved by the shared index. @@ -192,28 +208,35 @@ describe('validateFlowNodeWrites', () => { // the node folded that into `create_record(deal) failed: …`, the run failed, // and nothing was stored on either family — no row on create, an untouched // row and no shadow column on update. + // + // [#22161] The verdict names the refusal and that the step fails the run; + // the rest of that measured account is `os explain flow-node-write-unknown-field`, + // so it is pinned there. it('states the measured refusal — INVALID_FIELD / 400 on every datasource — and no driver split', () => { const [finding] = validateFlowNodeWrites({ objects: [dealObject], flows: [flowWith({ stagee: 'won' })], }); + const explanation = explainRule(FLOW_NODE_WRITE_UNKNOWN_FIELD)?.paragraphs.join('\n') ?? ''; expect(finding.message).toContain('INVALID_FIELD / 400'); - expect(finding.message).toContain('identically on every datasource'); - expect(finding.message).toContain('before any statement is built'); + expect(explanation).toContain('identically on every driver'); + expect(explanation).toContain('before any statement is built'); // Why the door answers and not a datasource: the node hands `fields` // straight to the data engine, so it is a caller payload. - expect(finding.message).toContain('ordinary caller payload'); + expect(explanation).toContain('ordinary caller payload'); // The severity's own justification, unchanged by the rewrite and still // stated: the refusal is WHOLE, so correctly named siblings are lost too. - expect(finding.message).toContain('never land either'); + expect(explanation).toContain('never land either'); expect(finding.message).toContain('the step fails the run'); // The retired driver split, both halves. - expect(finding.message).not.toMatch(/no such column/); - expect(finding.message).not.toMatch(/schemaless/); - expect(finding.message).not.toMatch(/is persisted/); - expect(finding.message).not.toMatch(/Nothing between the node and storage/); + for (const text of [finding.message, explanation]) { + expect(text).not.toMatch(/no such column/); + expect(text).not.toMatch(/schemaless/); + expect(text).not.toMatch(/is persisted/); + expect(text).not.toMatch(/Nothing between the node and storage/); + } }); it('flags every unknown key in one node, and only those', () => { @@ -369,7 +392,7 @@ describe('validateFlowNodeWrites', () => { // The INSERT consequence is strictly worse than the UPDATE one and the // message says so: the row never exists, so `{created.id}` downstream is // reading from a record that was never written. - expect(findings[0].message).toContain('the record is never created at all'); + expect(findings[0].message).toContain('the record is never created'); }); it('names only the UPDATE consequence for an update_record node', () => { @@ -377,7 +400,7 @@ describe('validateFlowNodeWrites', () => { objects: [dealObject], flows: [flowWith({ stagee: 'won' })], }); - expect(findings[0].message).not.toContain('never created at all'); + expect(findings[0].message).not.toContain('never created'); }); it('takes every skip on create_record too', () => { @@ -560,7 +583,7 @@ describe('[#8663] validateFlowNodeWrites — unprovisioned anchor writes', () => expect(findings[0].severity).toBe('warning'); expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.owner_id'); expect(findings[0].where).toBe('flow "stamp_owner" › node "Stamp"'); - expect(findings[0].message).toContain('external object (ADR-0015)'); + expect(findings[0].message).toContain("external object 'wh_order'"); expect(findings[0].message).toContain('can never land'); }); @@ -580,3 +603,32 @@ describe('[#8663] validateFlowNodeWrites — unprovisioned anchor writes', () => expect(findings[0].severity).toBe('error'); }); }); + +describe('[#22161] one-line verdicts — the rule ids this file shortened', () => { + it('every verdict the cases above fired for those ids is one line of at most 200 characters', () => { + // The coverage control first: each shortened id fired at least once, so + // the shape assertion below cannot pass over an empty record. + expect([...new Set(firedShortened.map((f) => f.rule))].sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + for (const f of firedShortened) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What each verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [FLOW_NODE_WRITE_UNPROVISIONED_ANCHOR]: ['ADR-0015', 'PAST the write-path validator', 'no such column', 'a claim about a remote schema'], + [FLOW_NODE_WRITE_UNKNOWN_FIELD]: ['ordinary caller payload', 'before any statement is built', 'never land either', 'never created at all'], + }; + + it('covers exactly the shortened ids', () => { + expect(Object.keys(MOVED).sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + }); + + it.each([...SHORTENED_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-flow-node-writes.ts b/packages/lint/src/validate-flow-node-writes.ts index 63f8854ee31..99582c11f40 100644 --- a/packages/lint/src/validate-flow-node-writes.ts +++ b/packages/lint/src/validate-flow-node-writes.ts @@ -92,13 +92,10 @@ import { indexObjectFields, judgeableFieldsOf, IMPLICIT_FIELDS, - unprovisionedAnchorWriteConsequence, + UNDECLARED_FIELD_WRITE_REFUSAL, + unprovisionedAnchorWriteVerdict, } from './validate-hook-body-writes.js'; -import { - indexUnprovisionedAnchors, - unprovisionedAnchorCause, - unprovisionedAnchorHint, -} from './system-fields.js'; +import { indexUnprovisionedAnchors, unprovisionedAnchorHint } from './system-fields.js'; import { walkFlowNodes, flowNodeLabel } from './flow-walk.js'; import { recordsOf } from './object-graph.js'; @@ -266,9 +263,7 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { rule: FLOW_NODE_WRITE_UNPROVISIONED_ANCHOR, where: `flow "${flowName}" › ${nodeWhere}`, path: `${nodePath}.config.fields.${fieldName}`, - message: - `${node.type} writes '${fieldName}', and ${unprovisionedAnchorCause(objectName, fieldName)} — ` + - unprovisionedAnchorWriteConsequence(), + message: unprovisionedAnchorWriteVerdict(objectName, fieldName, `this ${node.type} write`), hint: unprovisionedAnchorHint(objectName, fieldName), }); continue; @@ -282,19 +277,21 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { rule: FLOW_NODE_WRITE_UNKNOWN_FIELD, where: `flow "${flowName}" › ${nodeWhere}`, path: `${nodePath}.config.fields.${fieldName}`, + // [#13858] The node hands `fields` to the data engine directly + // (`data.insert` / `data.update` in service-automation's + // crud-nodes), so it is a CALLER payload and the #8682/#8738 + // declared-field door refuses it before any datasource is reached. + // Measured on driver-sql and driver-memory alike. [#22161] The + // verdict ends on the refusal clause the hook and action body rules + // share; that reasoning, and that the correctly named fields of the + // same payload never land either, is + // `os explain flow-node-write-unknown-field`. message: - // [#13858] The node hands `fields` to the data engine directly - // (`data.insert` / `data.update` in service-automation's - // crud-nodes), so it is a CALLER payload and the #8682/#8738 - // declared-field door refuses it before any datasource is reached. - // Measured on driver-sql and driver-memory alike. - `${node.type} writes '${fieldName}', but object '${objectName}' declares no such field. The ` + - `node hands its fields map to the engine as an ordinary caller payload, so the ` + - `declared-field door REFUSES the whole write — INVALID_FIELD / 400, identically on every ` + - `datasource, before any statement is built. The correctly named fields in this same payload ` + - `never land either${ - node.type === 'create_record' ? ' and the record is never created at all' : '' - }, and the step fails the run.`, + `${node.type} writes '${fieldName}', but object '${objectName}' declares no such field, ` + + UNDECLARED_FIELD_WRITE_REFUSAL + + (node.type === 'create_record' + ? ', the record is never created and the step fails the run' + : ' and the step fails the run'), hint: fixHint(fieldName, [...known]), }); } diff --git a/packages/lint/src/validate-hook-body-writes.test.ts b/packages/lint/src/validate-hook-body-writes.test.ts index 9a7eea63849..8df4378592f 100644 --- a/packages/lint/src/validate-hook-body-writes.test.ts +++ b/packages/lint/src/validate-hook-body-writes.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { - validateHookBodyWrites, + validateHookBodyWrites as validateHookBodyWritesUnrecorded, extractHookBodyWrites, extractHookBodyWriteSet, hookBodyFindingLocation, @@ -11,7 +11,31 @@ import { HOOK_BODY_WRITE_EXCLUSIONS, HOOK_BODY_WRITE_UNKNOWN_FIELD, HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR, + HOOK_BODY_SOURCE_UNPARSEABLE, } from './validate-hook-body-writes.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the rule ids this file's rule shortened is one +// verdict sentence; the reasoning it used to carry is the id's `os explain` +// entry. Every call below records what it fired, and the last case in this +// file holds each recorded verdict of those ids to one line of at most 200 +// characters — so the pin covers every firing variant this suite exercises, +// the lowered-handler suffix included, not a chosen few. Run the whole file: +// that case reads what the cases above fired. +const SHORTENED_RULE_IDS: readonly string[] = [ + HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR, + HOOK_BODY_WRITE_UNKNOWN_FIELD, + HOOK_BODY_SOURCE_UNPARSEABLE, +]; +const firedShortened: Array<{ rule: string; message: string }> = []; +const validateHookBodyWrites: typeof validateHookBodyWritesUnrecorded = (...args) => { + const findings = validateHookBodyWritesUnrecorded(...args); + for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f); + return findings; +}; + +/** The `os explain` text of `rule`, one string. */ +const explanationOf = (rule: string): string => explainRule(rule)?.paragraphs.join('\n') ?? ''; // Target objects: array-shaped and map-shaped `fields`, plus a second object // for cross-object `ctx.api` writes and multi-target hooks. @@ -107,7 +131,7 @@ describe('validateHookBodyWrites — ctx.input writes', () => { expect(findings[0].path).toBe('hooks[0].body.source'); expect(findings[0].message).toContain("discont_total"); expect(findings[0].message).toContain('crm_deal'); - expect(findings[0].message).toContain('INVALID_FIELD / 400, identically on every driver'); + expect(findings[0].message).toContain('INVALID_FIELD / 400'); expect(findings[0].hint).toContain("'discount_total'"); }); @@ -288,28 +312,34 @@ describe('validateHookBodyWrites — ctx.api writes', () => { // 'stagee' on object 'deal'", the target row was untouched, and the memory // family stored no shadow column. Same door the caller-payload half of // `undeclared-field-write-driver-split.integration.test.ts` pins. + // + // [#22161] The verdict names the refusal; the rest of that measured account + // is `os explain hook-body-write-unknown-field`, so it is pinned there. it('states the measured refusal — INVALID_FIELD / 400 on every driver — and no driver split', () => { const [finding] = validateHookBodyWrites( stackWith("await ctx.api.object('crm_deal').update({ id, stag: 'won' });"), ); + const explanation = explanationOf(HOOK_BODY_WRITE_UNKNOWN_FIELD); // What the author actually gets, in the vocabulary #13657 landed for the // `ctx.input` sibling one branch over — one door, one phrasing. expect(finding.message).toContain('INVALID_FIELD / 400'); - expect(finding.message).toContain('identically on every driver'); - expect(finding.message).toContain('before any statement is built'); + expect(explanation).toContain('identically on every driver'); + expect(explanation).toContain('before any statement is built'); // Why it is refused there rather than by a driver: the payload is a // CALLER's, which is the fact the whole rewrite turns on. - expect(finding.message).toContain('ordinary CALLER write'); + expect(explanation).toContain('ordinary CALLER write'); // ...and the blast radius that makes an author-time rule worth having. - expect(finding.message).toContain('fails the operation that triggered the hook'); + expect(explanation).toContain('fails the operation that triggered the hook'); // The retired claim, in both halves. Neither may come back without a // measurement saying it should. - expect(finding.message).not.toMatch(/driver-level error/); - expect(finding.message).not.toMatch(/schemaless/); - expect(finding.message).not.toMatch(/is persisted/); - expect(finding.message).not.toMatch(/write-path validator skips/); + for (const text of [finding.message, explanation]) { + expect(text).not.toMatch(/driver-level error/); + expect(text).not.toMatch(/schemaless/); + expect(text).not.toMatch(/is persisted/); + expect(text).not.toMatch(/write-path validator skips/); + } }); // [#22212] The firing control and the aliased spelling, side by side: the @@ -518,7 +548,7 @@ describe('[#8663] validateHookBodyWrites — unprovisioned anchor writes', () => expect(findings[0].where).toBe('hook "stamp" › body'); expect(findings[0].path).toBe('hooks[0].body.source'); expect(findings[0].message).toContain("'owner_id'"); - expect(findings[0].message).toContain('external object (ADR-0015)'); + expect(findings[0].message).toContain("external object 'wh_order'"); // The measured consequence, not the read-axis one: the value cannot land. expect(findings[0].message).toContain('can never land'); expect(findings[0].hint).toContain("declare it in wh_order's own fields"); @@ -726,3 +756,36 @@ describe('hookBodyFindingLocation / validateHookBodyWrites — #16546: path redi expect(findings[0].message).not.toContain('lowered from the inline handler'); }); }); + +describe('[#22161] one-line verdicts — the rule ids this file shortened', () => { + it('every verdict the cases above fired for those ids is one line of at most 200 characters', () => { + // The coverage control first: each shortened id fired at least once, so + // the shape assertion below cannot pass over an empty record. + expect([...new Set(firedShortened.map((f) => f.rule))].sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + // ...and the lowered-handler variant is among them: its location suffix + // rides the same message, so it is held to the same bound. + expect(firedShortened.some((f) => f.message.includes('lowered from the inline handler'))).toBe(true); + for (const f of firedShortened) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What each verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR]: ['ADR-0015', 'PAST the write-path validator', 'no such column', 'schemaless remote', 'every one of them'], + [HOOK_BODY_WRITE_UNKNOWN_FIELD]: ['copied back onto the record payload unfiltered', 'scoped handle on the running engine', 'identically on every driver', 'the record is never written'], + [HOOK_BODY_SOURCE_UNPARSEABLE]: ['partially recovered', 'judged by no rule', 'hook-api-update-readonly-field'], + }; + + it('covers exactly the shortened ids', () => { + expect(Object.keys(MOVED).sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + }); + + it.each([...SHORTENED_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index 8d2faa2499b..cad2d567256 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -100,8 +100,8 @@ import { import { SYSTEM_FIELDS, indexUnprovisionedAnchors, - unprovisionedAnchorCause, unprovisionedAnchorHint, + unprovisionedAnchorVerdict, } from './system-fields.js'; import { recordsOf } from './object-graph.js'; @@ -370,7 +370,7 @@ const INPUT_ENVELOPE_KEYS: ReadonlySet = new Set(['id', 'options', 'ast' * the platform actually provisioned storage for it on the object being written. * On an ADR-0015 `external` object the two diverge — the registered anchor has * no column behind it — so every consumer pairs this membership test with - * {@link unprovisionedAnchorWriteConsequence}'s check rather than treating a + * {@link unprovisionedAnchorWriteVerdict}'s check rather than treating a * hit here as the end of the question. The pairing is why the union may stay * generous: over-inclusion here no longer buys silence on a federated object. */ @@ -380,10 +380,11 @@ export const IMPLICIT_FIELDS: ReadonlySet = new Set([ ]); /** - * [commit 192213f66] The CONSEQUENCE clause every unprovisioned-anchor WRITE diagnostic - * states — one wording across the three write surfaces (hook body, action body, - * flow node), paired with `unprovisionedAnchorCause` / `unprovisionedAnchorHint` - * from `system-fields.ts` the way #8340's four read-axis rules pair with them. + * [commit 192213f66] [#22161] The verdict every unprovisioned-anchor WRITE finding prints — + * one wording across the three write surfaces (hook body, action body, flow + * node): the shared one-clause {@link unprovisionedAnchorVerdict} from + * `system-fields.ts`, then what it means for the write `write` names (`the + * body's update(…) write`, `this update_record write`). * * Shared rather than re-typed for #8340's reason: the sentence is the finding's * evidentiary content, and a rule that re-words it drifts from its siblings and @@ -392,7 +393,13 @@ export const IMPLICIT_FIELDS: ReadonlySet = new Set([ * that module's own note reserves the per-site consequence to the site, and the * "site" for this family is the family, not any one of its three files. * - * ## Every clause below is measured, not inferred (commit 192213f66) + * The consequence the finding used to spell out in full — the anchor rides the + * registered schema PAST the write-path validator, and the remote database is + * what rejects it — is the three ids' `os explain` text, written ONCE in + * `rule-explanations.ts` (the paragraphs the three entries share), so the + * three families cannot drift apart there either. + * + * ## Every clause of that explanation is measured, not inferred (commit 192213f66) * * The card that produced this rule asserted a structural resemblance to the * read-axis gap and explicitly declined to guess the runtime behaviour. Measured @@ -415,16 +422,42 @@ export const IMPLICIT_FIELDS: ReadonlySet = new Set([ * own upstream refusal cannot close — which is why this is a finding and not a * duplicate of the unknown-field rule next to it. */ -export function unprovisionedAnchorWriteConsequence(): string { +export function unprovisionedAnchorWriteVerdict(objectName: string, field: string, write: string): string { + return `${unprovisionedAnchorVerdict(objectName, field)}, so ${write} can never land`; +} + +/** + * [#22161] The refusal clause every undeclared-field WRITE finding ends on — + * one wording across the three write surfaces (hook body, action body, flow + * node), because all three are refused by the same door: the engine's + * declared-field door, `INVALID_FIELD` / 400. Why that door answers, and what + * it takes down with it, is the three ids' `os explain` text + * (`rule-explanations.ts`, written once there and shared by the three entries). + */ +export const UNDECLARED_FIELD_WRITE_REFUSAL = 'so the write is refused (INVALID_FIELD / 400)'; + +/** + * [#22161] The verdict an undeclared `ctx.api` write prints on BOTH body + * surfaces — this rule's and `validate-action-body-writes.ts`' — which judge the + * identical `api-crud-literal` shape, so one function spells it for both. + */ +export function undeclaredApiWriteVerdict(objectName: string, method: string, field: string): string { return ( - `so the value can never land: the anchor exists only in the registered schema, which is what carries ` + - `it PAST the write-path validator that refuses an undeclared name outright (INVALID_FIELD). The remote ` + - `database is what rejects it — on a SQL remote with an untyped driver error ('no such column') that ` + - `aborts the whole statement, so the correctly named fields in the same payload never land either; on a ` + - `schemaless remote the key is persisted into a column no read surface returns.` + `body calls ctx.api.object('${objectName}').${method}(…) writing undeclared field '${field}', ` + + UNDECLARED_FIELD_WRITE_REFUSAL ); } +/** + * [#10653] [#22161] The verdict an unparseable L2 body prints on BOTH body + * surfaces (`hook-body-source-unparseable`, `action-body-source-unparseable`): + * the same extractor, the same parse, so one sentence. What a partially + * recovered tree means for the checks is their shared `os explain` text. + */ +export function bodyParseFailureVerdict(failure: SourceParseFailure): string { + return `L2 body did not parse (${describeParseFailure(failure)}), so writes in its unread part go unchecked`; +} + type AnyRec = Record; const isRec = (v: unknown): v is AnyRec => !!v && typeof v === 'object' && !Array.isArray(v); @@ -1060,10 +1093,9 @@ export function validateHookBodyWrites( rule: HOOK_BODY_SOURCE_UNPARSEABLE, where: `hook "${hookName}" › body`, path: loc.path, - message: - `L2 body did not parse (${describeParseFailure(extracted.parseFailure)}), so its write set was ` + - `read from a partially recovered tree — an undeclared field write in the unread part is not ` + - `reported.${loc.messageSuffix}`, + // [#22161] One verdict sentence; what a partially recovered tree means + // for the checks is `os explain hook-body-source-unparseable`. + message: `${bodyParseFailureVerdict(extracted.parseFailure)}${loc.messageSuffix}`, hint: PARSE_FAILURE_HINT, }); } @@ -1121,8 +1153,7 @@ export function validateHookBodyWrites( where, path, message: - `body writes '${w.field}' to its input, and ${unprovisionedAnchorCause(anchorObj, w.field)} — ` + - unprovisionedAnchorWriteConsequence() + + unprovisionedAnchorWriteVerdict(anchorObj, w.field, `the body's write to its input`) + loc.messageSuffix, hint: unprovisionedAnchorHint(anchorObj, w.field), }); @@ -1142,13 +1173,12 @@ export function validateHookBodyWrites( where, path, message: - `body writes '${w.field}' to its input, but ${objDesc} ${declares}. The sandboxed script runs ` + // The post-hook declared-field door (commit b003cf2e8) is what refuses it; the // id stays in this comment rather than in the string, which reaches - // authors and operators who cannot resolve a tracker number. - `clean and the value is copied back onto the record payload unfiltered, so the write is then ` + - `REFUSED at run time — INVALID_FIELD / 400, identically on every driver. The ` + - `record is never written, and the refusal names the field far from the body that wrote it.` + + // authors and operators who cannot resolve a tracker number. [#22161] + // How the value reaches that door is `os explain hook-body-write-unknown-field`. + `body writes '${w.field}' to its input, but ${objDesc} ${declares}, ` + + UNDECLARED_FIELD_WRITE_REFUSAL + loc.messageSuffix, hint: fixHint(w.field, unionCandidates(targetSets)), }); @@ -1169,8 +1199,7 @@ export function validateHookBodyWrites( where, path, message: - `body calls ctx.api.object('${w.object}').${w.method ?? 'update'}(…) writing '${w.field}', and ` + - `${unprovisionedAnchorCause(w.object, w.field)} — ${unprovisionedAnchorWriteConsequence()}` + + unprovisionedAnchorWriteVerdict(w.object, w.field, `the body's ${w.method ?? 'update'}(…) write`) + loc.messageSuffix, hint: unprovisionedAnchorHint(w.object, w.field), }); @@ -1183,20 +1212,15 @@ export function validateHookBodyWrites( rule: HOOK_BODY_WRITE_UNKNOWN_FIELD, where, path, - message: - `body calls ctx.api.object('${w.object}').${w.method ?? 'update'}(…) writing '${w.field}', but ` + - // [#13858] ctx.api is a ScopedContext over the running engine - // (`ObjectQL.buildHookApi`), so this payload is CALLER-supplied and - // the declared-field door (#8682 insert, #8738 update) is what - // refuses it. Measured on both families; the ids stay in comments - // rather than in the string, which reaches authors and operators - // who cannot resolve a tracker number. - `object '${w.object}' declares no such field. ctx.api is a scoped handle on the running ` + - `engine, so the payload arrives as an ordinary CALLER write and the declared-field door ` + - `REFUSES it at run time — INVALID_FIELD / 400, identically on every driver, before ` + - `any statement is built. The nested write lands nothing, and the refusal escapes the body ` + - `and fails the operation that triggered the hook.` + - loc.messageSuffix, + // [#13858] ctx.api is a ScopedContext over the running engine + // (`ObjectQL.buildHookApi`), so this payload is CALLER-supplied and + // the declared-field door (#8682 insert, #8738 update) is what + // refuses it. Measured on both families; the ids stay in comments + // rather than in the string, which reaches authors and operators + // who cannot resolve a tracker number. [#22161] That reasoning is + // `os explain hook-body-write-unknown-field`; the verdict names the + // refusal. + message: undeclaredApiWriteVerdict(w.object, w.method ?? 'update', w.field) + loc.messageSuffix, hint: fixHint(w.field, [...known]), }); } diff --git a/packages/lint/src/validate-readonly-action-writes.test.ts b/packages/lint/src/validate-readonly-action-writes.test.ts index 126eaf5a093..bded5c8d411 100644 --- a/packages/lint/src/validate-readonly-action-writes.test.ts +++ b/packages/lint/src/validate-readonly-action-writes.test.ts @@ -32,12 +32,27 @@ import { describe, expect, it } from 'vitest'; import { HOOK_BODY_WRITE_PATTERNS } from './validate-hook-body-writes.js'; import { - validateReadonlyActionWrites, + validateReadonlyActionWrites as validateReadonlyActionWritesUnrecorded, ACTION_API_UPDATE_READONLY_WHEN_FIELD, READONLY_ACTION_WRITE_PATTERN_IDS, READONLY_ACTION_WRITE_EXCLUSIONS, READONLY_ACTION_INSERT_SILENCE, } from './validate-readonly-action-writes.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the rule id this file's rule shortened is one +// verdict sentence; the reasoning it used to carry is the id's `os explain` +// entry. Every call below records what it fired, and the last case in this +// file holds each recorded verdict to one line of at most 200 characters — so +// the pin covers every firing variant this suite exercises, not a chosen few. +// Run the whole file: that case reads what the cases above fired. +const SHORTENED_RULE_IDS: readonly string[] = [ACTION_API_UPDATE_READONLY_WHEN_FIELD]; +const firedShortened: Array<{ rule: string; message: string }> = []; +const validateReadonlyActionWrites: typeof validateReadonlyActionWritesUnrecorded = (...args) => { + const findings = validateReadonlyActionWritesUnrecorded(...args); + for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f); + return findings; +}; /** * A stack shaped like the shipped showcase invoice (a state lock: once an @@ -544,3 +559,31 @@ describe('READONLY_ACTION_WRITE_PATTERN_IDS - ledger partition', () => { } }); }); + +describe('[#22161] one-line verdicts — the rule id this file shortened', () => { + it('every verdict the cases above fired for that id is one line of at most 200 characters', () => { + // The coverage control first: the shortened id fired at least once, so + // the shape assertion below cannot pass over an empty record. + expect([...new Set(firedShortened.map((f) => f.rule))].sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + for (const f of firedShortened) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What the verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [ACTION_API_UPDATE_READONLY_WHEN_FIELD]: ['STATIC `readonly` strip', 'NOT from the conditional one', 'bulk update', 'beforeUpdate'], + }; + + it('covers exactly the shortened id', () => { + expect(Object.keys(MOVED).sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + }); + + it.each([...SHORTENED_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-readonly-action-writes.ts b/packages/lint/src/validate-readonly-action-writes.ts index a425235c79b..b481423f3f2 100644 --- a/packages/lint/src/validate-readonly-action-writes.ts +++ b/packages/lint/src/validate-readonly-action-writes.ts @@ -108,7 +108,7 @@ import { extractHookBodyWriteSet, type BodyWritePatternExclusion, } from './validate-hook-body-writes.js'; -import { buildReadonlyIndex } from './validate-readonly-flow-writes.js'; +import { buildReadonlyIndex, READONLY_WHEN_STRIP_SCOPE } from './validate-readonly-flow-writes.js'; import { recordsOf } from './object-graph.js'; export type ReadonlyActionWriteSeverity = 'warning'; @@ -304,12 +304,12 @@ export function validateReadonlyActionWrites(stack: AnyRec): ReadonlyActionWrite 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. + // for it is #9107's LOCK 2. Both ids stay in this comment. [#22161] One + // verdict sentence; which strip elevation waives and which it does not + // is `os explain action-api-update-readonly-when-field`. 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.`, + `body's ${call} writes readonlyWhen field '${w.field}', silently stripped ${READONLY_WHEN_STRIP_SCOPE} ` + + `even though an action body runs elevated`, 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 ` + diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts index 06e2e20fb7b..fbdb2438531 100644 --- a/packages/lint/src/validate-readonly-flow-writes.test.ts +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -2,10 +2,29 @@ import { describe, it, expect } from 'vitest'; import { - validateReadonlyFlowWrites, + validateReadonlyFlowWrites as validateReadonlyFlowWritesUnrecorded, FLOW_UPDATE_READONLY_FIELD, FLOW_UPDATE_READONLY_WHEN_FIELD, } from './validate-readonly-flow-writes.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the rule ids this file's rule shortened is one +// verdict sentence; the reasoning it used to carry is the id's `os explain` +// entry. Every call below records what it fired, and the last case in this +// file holds each recorded verdict of those ids to one line of at most 200 +// characters — so the pin covers every firing variant this suite exercises, +// not a chosen few. Run the whole file: that case reads what the cases above +// fired. +const SHORTENED_RULE_IDS: readonly string[] = [FLOW_UPDATE_READONLY_FIELD, FLOW_UPDATE_READONLY_WHEN_FIELD]; +const firedShortened: Array<{ rule: string; message: string }> = []; +const validateReadonlyFlowWrites: typeof validateReadonlyFlowWritesUnrecorded = (...args) => { + const findings = validateReadonlyFlowWritesUnrecorded(...args); + for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f); + return findings; +}; + +/** The `os explain` text of `rule`, one string. */ +const explanationOf = (rule: string): string => explainRule(rule)?.paragraphs.join('\n') ?? ''; // Target object: a static-readonly field, a conditional readonlyWhen field, and // a plain writable field. Map-shaped `fields` (the common authoring form). @@ -56,7 +75,7 @@ describe('validateReadonlyFlowWrites', () => { expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.approval_status'); expect(findings[0].message).toContain('approval_status'); expect(findings[0].message).toContain('crm_opportunity'); - expect(findings[0].message).toContain('silently strips readonly fields from the UPDATE payload, so this write never lands'); + expect(findings[0].message).toContain("which a runAs:'user' UPDATE silently strips"); expect(findings[0].where).toBe('flow "stamp_approval" › node "Stamp approval"'); }); @@ -143,7 +162,11 @@ describe('validateReadonlyFlowWrites', () => { expect(findings).toHaveLength(1); expect(findings[0].severity).toBe('warning'); expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_WHEN_FIELD); - expect(findings[0].message).toContain('a bulk update strips it from every matched row once any one of them is locked'); + expect(findings[0].message).toContain('where its predicate is TRUE'); + // [#22161] The bulk-update reach is `os explain flow-update-readonly-when-field`. + expect(explanationOf(FLOW_UPDATE_READONLY_WHEN_FIELD)).toContain( + 'a bulk update strips it from every matched row once any one of them is locked', + ); }); // The hint is the WHOLE product of an advisory rule - the finding blocks @@ -233,7 +256,7 @@ describe('validateReadonlyFlowWrites', () => { // The message states the run identity it was judged under, so a reader of // the finding cannot mistake it for the user-run case. expect(findings[0].message).toContain("runAs:'system'"); - expect(findings[0].message).toContain('a bulk update strips it from every matched row once any one of them is locked'); + expect(findings[0].message).toContain('where its predicate is TRUE'); expect(findings[0].hint).toContain('NOT waived by a system context'); }); @@ -364,7 +387,7 @@ describe('validateReadonlyFlowWrites', () => { // 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('from the UPDATE payload, so this write never lands'); + expect(findings[0].message).not.toContain('UPDATE'); // The remedy names the create verb, the system channel and the own-object // beforeInsert stamp. expect(findings[0].hint).toContain("runAs:'system'"); @@ -648,3 +671,32 @@ describe('validateReadonlyFlowWrites', () => { expect(findings[0].path).toBe('flows[0].nodes[0].config.body.nodes[0].config.fields.amount'); }); }); + +describe('[#22161] one-line verdicts — the rule ids this file shortened', () => { + it('every verdict the cases above fired for those ids is one line of at most 200 characters', () => { + // The coverage control first: each shortened id fired at least once, so + // the shape assertion below cannot pass over an empty record. + expect([...new Set(firedShortened.map((f) => f.rule))].sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + for (const f of firedShortened) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What each verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [FLOW_UPDATE_READONLY_FIELD]: ['still reports success', 'defaultValue', 'run-time warning naming the dropped field', 'bypasses the static strip'], + [FLOW_UPDATE_READONLY_WHEN_FIELD]: ['bulk update', 'NOT waived by a system context', 'beforeUpdate', 'INSERT is never judged'], + }; + + it('covers exactly the shortened ids', () => { + expect(Object.keys(MOVED).sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + }); + + it.each([...SHORTENED_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index 47cb0253fd1..c7358e50b40 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -88,6 +88,19 @@ export interface ReadonlyFlowWriteFinding { export const FLOW_UPDATE_READONLY_FIELD = 'flow-update-readonly-field'; export const FLOW_UPDATE_READONLY_WHEN_FIELD = 'flow-update-readonly-when-field'; +/** + * [#22161] The clauses the three readonly write rules' one-line verdicts share + * — this file's, `validate-readonly-hook-writes.ts` and + * `validate-readonly-action-writes.ts` — written once so the three families + * cannot drift apart: where a `readonlyWhen` lock strips a write, and what a + * stripped INSERT leaves behind. The reasoning each verdict no longer carries + * (which strip runs where, what a system context waives and what it does not) + * is the ids' `os explain` text, whose shared paragraphs are likewise written + * once in `rule-explanations.ts`. + */ +export const READONLY_WHEN_STRIP_SCOPE = 'where its predicate is TRUE'; +export const READONLY_INSERT_STRIP_OUTCOME = 'so the row is created WITHOUT this column'; + /** The node type whose payload the STATIC branch alone judges (#15394). */ const CREATE_NODE_TYPE = 'create_record'; /** The node type both branches judge. */ @@ -297,19 +310,16 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind rule: FLOW_UPDATE_READONLY_FIELD, where, 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. 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 ` + - `(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, ` + - `so this write never lands — while the step still reports success.`, + // The create-side strip is the 2026-09-03 ruling (#14147): the 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`). [#22161] One verdict + // sentence naming the verb it was judged on; that the step still + // reports success, and what the column falls back to, is + // `os explain flow-update-readonly-field`. + message: + `writes readonly field '${fieldName}' of object '${objectName}', which a runAs:'${runAs}' ` + + (isCreate ? `INSERT silently strips, ${READONLY_INSERT_STRIP_OUTCOME}` : 'UPDATE silently strips'), 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 ` + @@ -331,11 +341,12 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind rule: FLOW_UPDATE_READONLY_WHEN_FIELD, where, path: `${nodePath}.config.fields.${fieldName}`, + // [#22161] One verdict sentence naming the run identity it was + // judged under; the bulk-update reach and why elevation does not + // waive the lock are `os explain flow-update-readonly-when-field`. message: - `writes field '${fieldName}', which object '${objectName}' declares readonlyWhen. On records ` + - `where that predicate is TRUE, a runAs:'${runAs}' UPDATE strips the field (a bulk update strips ` + - `it from every matched row once any one of them is locked), so this write may silently not ` + - `land depending on the record's state.`, + `writes readonlyWhen field '${fieldName}' of object '${objectName}', which a runAs:'${runAs}' ` + + `UPDATE silently strips ${READONLY_WHEN_STRIP_SCOPE}`, hint: `Elevation is not a workaround here: unlike the static readonly strip, the conditional lock ` + `is NOT waived by a system context, so runAs:'system' strips this field on a locked record ` + diff --git a/packages/lint/src/validate-readonly-hook-writes.test.ts b/packages/lint/src/validate-readonly-hook-writes.test.ts index 34ca4ae9e07..b058595a9dc 100644 --- a/packages/lint/src/validate-readonly-hook-writes.test.ts +++ b/packages/lint/src/validate-readonly-hook-writes.test.ts @@ -12,7 +12,7 @@ import { describe, expect, it } from 'vitest'; import { HOOK_BODY_WRITE_PATTERNS } from './validate-hook-body-writes.js'; import { - validateReadonlyHookWrites, + validateReadonlyHookWrites as validateReadonlyHookWritesUnrecorded, HOOK_API_UPDATE_READONLY_FIELD, HOOK_API_UPDATE_READONLY_WHEN_FIELD, READONLY_HOOK_WRITE_PATTERN_IDS, @@ -20,6 +20,22 @@ import { READONLY_HOOK_STRIP_SUBJECT_METHODS, READONLY_HOOK_METHOD_EXCLUSIONS, } from './validate-readonly-hook-writes.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the rule ids this file's rule shortened is one +// verdict sentence; the reasoning it used to carry is the id's `os explain` +// entry. Every call below records what it fired, and the last case in this +// file holds each recorded verdict of those ids to one line of at most 200 +// characters — so the pin covers every firing variant this suite exercises, +// the lowered-handler suffix included, not a chosen few. Run the whole file: +// that case reads what the cases above fired. +const SHORTENED_RULE_IDS: readonly string[] = [HOOK_API_UPDATE_READONLY_FIELD, HOOK_API_UPDATE_READONLY_WHEN_FIELD]; +const firedShortened: Array<{ rule: string; message: string }> = []; +const validateReadonlyHookWrites: typeof validateReadonlyHookWritesUnrecorded = (...args) => { + const findings = validateReadonlyHookWritesUnrecorded(...args); + for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f); + return findings; +}; /** * A stack shaped like the reference app's motivating case (#13653): a derived @@ -295,7 +311,9 @@ describe('validateReadonlyHookWrites - RED: insert() of a static-readonly field 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'); + // [#22161] That the strip runs on INSERT as on UPDATE is + // `os explain hook-api-update-readonly-field`. + expect(explainRule(HOOK_API_UPDATE_READONLY_FIELD)?.paragraphs.join('\n')).toContain('on UPDATE and on INSERT alike'); 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. @@ -793,3 +811,35 @@ describe('validateReadonlyHookWrites - #16546: path redirect for a lowered hook' expect(findings[0].path).toBe('hooks[0].body.source'); }); }); + +describe('[#22161] one-line verdicts — the rule ids this file shortened', () => { + it('every verdict the cases above fired for those ids is one line of at most 200 characters', () => { + // The coverage control first: each shortened id fired at least once, so + // the shape assertion below cannot pass over an empty record. + expect([...new Set(firedShortened.map((f) => f.rule))].sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + // ...and the lowered-handler variant is among them: its location suffix + // rides the same message, so it is held to the same bound. + expect(firedShortened.some((f) => f.message.includes('lowered from the inline handler'))).toBe(true); + for (const f of firedShortened) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What each verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [HOOK_API_UPDATE_READONLY_FIELD]: ['TRIGGERING operation', 'still reports success', 'defaultValue', "runAs: 'system'", 'not marshalled into the sandbox'], + [HOOK_API_UPDATE_READONLY_WHEN_FIELD]: ['TRIGGERING operation', 'NOT waived by a system context', 'beforeUpdate', 'not marshalled into the sandbox'], + }; + + it('covers exactly the shortened ids', () => { + expect(Object.keys(MOVED).sort()).toEqual([...SHORTENED_RULE_IDS].sort()); + }); + + it.each([...SHORTENED_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-readonly-hook-writes.ts b/packages/lint/src/validate-readonly-hook-writes.ts index 66a21993451..c9c3e63a3cf 100644 --- a/packages/lint/src/validate-readonly-hook-writes.ts +++ b/packages/lint/src/validate-readonly-hook-writes.ts @@ -172,7 +172,12 @@ import { hookBodyFindingLocation, type BodyWritePatternExclusion, } from './validate-hook-body-writes.js'; -import { buildReadonlyIndex, buildInsertStripExemptObjects } from './validate-readonly-flow-writes.js'; +import { + buildReadonlyIndex, + buildInsertStripExemptObjects, + READONLY_INSERT_STRIP_OUTCOME, + READONLY_WHEN_STRIP_SCOPE, +} from './validate-readonly-flow-writes.js'; import { recordsOf } from './object-graph.js'; export type ReadonlyHookWriteSeverity = 'error' | 'warning'; @@ -446,17 +451,14 @@ export function validateReadonlyHookWrites( path, // 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.`) + loc.messageSuffix, + // the message an author reads and cannot resolve. [#22161] One + // verdict sentence; why a hook's ctx.api write is a caller's, and + // that the call still returns success, is + // `os explain hook-api-update-readonly-field`. + message: + `body's ${call} writes readonly field '${w.field}', silently stripped on a non-system trigger` + + (isCreate ? `, ${READONLY_INSERT_STRIP_OUTCOME}` : '') + + loc.messageSuffix, 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, ` + @@ -483,10 +485,11 @@ export function validateReadonlyHookWrites( // The conditional strip is #3042. #9107 REMOVED its one over-reach: // the strip now judges the CALLER's entry snapshot, so a value a // beforeUpdate hook derives is no longer deleted. Both ids stay here. + // [#22161] One verdict sentence; the reasoning is + // `os explain hook-api-update-readonly-when-field`. message: - `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` + - `readonlyWhen. On records whose predicate is TRUE that UPDATE strips the field, so this ` + - `write may silently not land depending on the record's state.` + loc.messageSuffix, + `body's ${call} writes readonlyWhen field '${w.field}', silently stripped ${READONLY_WHEN_STRIP_SCOPE}` + + loc.messageSuffix, hint: `Either confirm this call only targets records whose readonlyWhen predicate is FALSE, or ` + `derive '${w.field}' in a beforeUpdate hook on '${objectName}' - a hook-derived value is not ` +