From 3a6fe9c8904273166e6bd14f9569b9dee4caa6d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 21:39:49 +0000 Subject: [PATCH 1/3] fix(lint): one-line verdicts for the record-write rules; `os explain RULE_ID` carries their reasoning Stage 2 of the card, slice 3: the 14 rule ids of the hook-body, action-body and flow-node write rules and the three readonly write rules each print one verdict sentence. The reasoning moves into 14 RULE_EXPLANATIONS entries; the prose the three write surfaces shared is written once there (and the verdict clauses once in the rule files) so the families cannot drift apart. Rule ids, severities, paths, hints and accept/refuse behaviour are unchanged. Claude-Session: https://claude.ai/code/session_01VZqqwTj2wsihZEbfT6yyYN Co-authored-by: Claude --- packages/lint/src/rule-explanations.ts | 302 ++++++++++++++++++ .../src/validate-action-body-writes.test.ts | 96 +++++- .../lint/src/validate-action-body-writes.ts | 47 ++- .../src/validate-flow-node-writes.test.ts | 76 ++++- .../lint/src/validate-flow-node-writes.ts | 39 ++- .../src/validate-hook-body-writes.test.ts | 85 ++++- .../lint/src/validate-hook-body-writes.ts | 104 +++--- .../validate-readonly-action-writes.test.ts | 45 ++- .../src/validate-readonly-action-writes.ts | 12 +- .../src/validate-readonly-flow-writes.test.ts | 62 +++- .../lint/src/validate-readonly-flow-writes.ts | 45 ++- .../src/validate-readonly-hook-writes.test.ts | 54 +++- .../lint/src/validate-readonly-hook-writes.ts | 33 +- 13 files changed, 827 insertions(+), 173 deletions(-) diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index 8e90b745b7a..436bf3f8fd1 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 a ctx.record assignment 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 ` + From 4c78055635859a30757385962b69e7d17a8aa933 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 21:50:16 +0000 Subject: [PATCH 2/3] =?UTF-8?q?chore(changeset):=20slice=203=20of=20the=20?= =?UTF-8?q?one-line=20verdicts=20=E2=80=94=20the=20doors=20that=20print=20?= =?UTF-8?q?the=20new=20text?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01VZqqwTj2wsihZEbfT6yyYN Co-authored-by: Claude --- .changeset/22161-lint-slice-3-one-line.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .changeset/22161-lint-slice-3-one-line.md 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. From 77821c2e1d2094a8f11dd25fb22dd6f4880228e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 22:07:27 +0000 Subject: [PATCH 3/3] fix(lint): the record-snapshot explanation's `covers` phrase carries no dot Claude-Session: https://claude.ai/code/session_01VZqqwTj2wsihZEbfT6yyYN Co-authored-by: Claude --- packages/lint/src/rule-explanations.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index 436bf3f8fd1..378d31296ef 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -903,7 +903,7 @@ const ACTION_BODY_WRITE_UNKNOWN_FIELD_EXPLANATION: RuleExplanation = { const ACTION_RECORD_WRITE_DISCARDED_EXPLANATION: RuleExplanation = { rule: 'action-record-write-discarded', - covers: 'why a ctx.record assignment is 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 ' +