diff --git a/.changeset/22161-lint-slice-12-one-line-fix.md b/.changeset/22161-lint-slice-12-one-line-fix.md new file mode 100644 index 00000000000..f96331ec587 --- /dev/null +++ b/.changeset/22161-lint-slice-12-one-line-fix.md @@ -0,0 +1,32 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): 16 more author-time rule ids print a `fix:` line of one remedy sentence, and `os explain ` carries what those lines used to enumerate + +Clause-②: no + +- **Shorter fix lines.** Each finding of these 16 rule ids now carries a `hint` (the CLI's `fix:` line, the runtime issue's `hint`) of one remedy sentence of at most 200 characters on every variant the lint suite fires (the lengths here are the `hint` alone; the CLI prints it after `fix: `). The longest of each, before → after: + - validation rules: `validation-rule-json-schema-unknown-format` (646 → 183); + - datasets and cubes: `measure-aggregate-field-type-refused` (645 → 178) and `dimension-json-stored-field-refused` (262 → 177); + - form predicates: `predicate-rhs-path-shaped` (643 → 184), `predicate-path-unrooted` (214 → 122) and `predicate-path-unresolved` (201 → 153); + - RLS predicates: `rls-predicate-unknown-user-variable` (618 → 193) and `rls-predicate-unparseable` (523 → 171); + - hook bodies: `hook-api-update-readonly-when-field` (607 → 173); + - action names: `action-name-undefined` (560 → 195); + - AI skills: `ai-skill-tool-unresolved` (548 → 189); + - organization axis: `org-axis-permission-inheritance` (542 → 145) and `org-axis-cross-org-bu-grant` (315 → 141); + - object names: `object-reference-unknown` (522 → 134) and `object-reference-unregistered-platform` (500 → 187); + - filters: `filter-empty-node` (515 → 113). +- **Lists are no longer echoed.** `object-reference-unknown` no longer lists every object the stack and its artifact define, and `action-name-undefined` no longer lists every action of the stack or of the related object: the verdict's "Did you mean" already names the nearest one. `validation-rule-json-schema-unknown-format` names the nearest registered format and no longer lists all of them, and `ai-skill-tool-unresolved` no longer lists the tool family prefixes. `rls-predicate-unknown-user-variable` names the kernel keys and the nearest one, and so holds no author text. +- **One line, one remedy.** Where a fix line explained its remedy, the explanation moved: why a missing object costs what it costs at each site (the record picker, the view container's index, the dashboard filter, the dataset's reports, the nav entry), the doors that refuse a refused aggregate pair later (a dataset's compile, a cube's query), how an app stages an RLS membership set, why no system context waives a `readonlyWhen` lock, and the declared "match nothing" filter spelling. Where the namespace-prefixed object exists, a view container's fix line is that spelling alone (`write "my_app_order_line", not "order_line"`). +- **`os explain ` carries the rest.** What the fix lines stopped saying is each id's `RULE_EXPLANATIONS` entry in `@objectstack/lint`, which `os explain ` prints. Two ids had no entry and gain one, so their `rule:` line now ends with the `os explain` pointer: `object-reference-unknown` ("how an object name resolves, and what a miss costs") and `org-axis-permission-inheritance` ("why access never walks the organization tree"). The other 14 entries gain paragraphs. The registered format names and the tool family prefixes are written out in their entries and held to the live sets by the rules' tests. +- **Where the new fix lines print.** On the CLI: the `fix:` line under each finding of `os validate`, `os build` (and `os compile`, which `os dev` runs at startup and, under `--verbose`, on each watch recompile), the scaffold check `os init` runs, and `os verify`'s refusal list; `os lint` prints it as `→ fix:` under `--fix` and as `fix` in `--json`; `os validate --json` (`errors`) and `os build --json` (author-time `issues`) carry it on an error-severity finding; their `warnings` carry an advisory as one `where: message` string, without it. At the runtime publish gate (Studio, REST `PUT /api/v1/meta`, and the MCP and AI authors that write through that door), the `hint` FROM → TO, measured at the door: + - an `object` write: `validation-rule-json-schema-unknown-format` (422 `issues[].hint`, 638 / 606 → 175 / 143, with and without a nearest name); + - a `dataset` write: `measure-aggregate-field-type-refused` (422, 606 / 581 → 147 / 122), `dimension-json-stored-field-refused` (422, multi-value 259 → 166, structured JSON 177 unchanged), `object-reference-unknown` (422 on the dataset's `object`, 331 → 134) and `object-reference-unregistered-platform` (2xx `advisories[].hint` and the `[Protocol] authoring advisory` log line, 449 → 187); + - a `view` write of a schema-bound metadata form: `predicate-rhs-path-shaped` (422 on a dotted right-hand side, 384 → 156; 2xx advisory on a bare word, 631 → 180), `predicate-path-unrooted` (422, 214 → 122) and `predicate-path-unresolved` (422, 100 → 36 with a nearest key); + - a `permission` write: `rls-predicate-unparseable` (422, 523 → 171) and `rls-predicate-unknown-user-variable` (422, 566 / 634 → 158 / 190, without and with a nearest key); + - a `flow` or `report` write: `filter-empty-node` (422, 515 / 278 / 170 → 113 / 107 / 69 for the whole filter, a `$or` branch and a `$and` branch). + + Studio's save-advisory toast prints an advisory's `hint` after its message, so the platform-object and bare-word toasts shorten; no Studio, REST or MCP reader in this repository or in objectui matches on `hint` text. A tool that did should match on `rule` and `path` instead. +- **Never at the runtime gate:** `hook-api-update-readonly-when-field`, `action-name-undefined`, `ai-skill-tool-unresolved` and the two `org-axis-*` ids run on the CLI only, as do the object-reference ids on every site but a dataset's `object`. +- **Unchanged:** every `message` (the verdict), rule id, severity and `path`, and what each rule accepts or refuses. diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index f8c8ac51efa..de056bb8a53 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -1042,7 +1042,11 @@ const HOOK_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION: RuleExplanation = { 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.', + 'conditional lock, and `sudo()` is not marshalled into the sandbox in any case (calling it from a ' + + 'body is a TypeError at run time).', + 'So derive the value in a `beforeUpdate` hook on the record\'s own object, or make sure the call only ' + + 'targets records whose `readonlyWhen` predicate is FALSE; otherwise drop the field from the ' + + 'payload. The warning never blocks a build.', ], }; @@ -1152,6 +1156,10 @@ const RLS_PREDICATE_UNPARSEABLE_EXPLANATION: RuleExplanation = { 'An RLS predicate is CEL (ADR-0058 D1). Before parsing, a legacy SQL bridge rewrites the historic subset ' + 'it covers — a bare `=` to `==` and `IN` to `in`, never inside a quoted literal — and everything else must ' + 'already be CEL: SQL `AND` / `OR` / `LIKE`, a subquery or a stray operator does not parse.', + 'So combine with `&&` / `||` rather than SQL `AND` / `OR`, negate with `!`, and use `startsWith` / ' + + '`endsWith` / `contains` rather than `LIKE`. A subquery has no CEL spelling at all: RLS cannot join ' + + '(ADR-0055), so pre-resolve the set into a membership key the runtime exposes ' + + '(`FIELD in current_user.KEY`, ADR-0105 D11) or denormalise the value onto this object.', RLS_UNCOMPILABLE_DROP, RLS_DROPPED_USING, RLS_DROPPED_CHECK, @@ -1309,6 +1317,11 @@ const RLS_PREDICATE_UNKNOWN_USER_VARIABLE_EXPLANATION: RuleExplanation = { 'the runtime stages as ARRAYS and which are therefore usable only as `field in current_user.KEY`. A key ' + 'outside that list in a scalar position — compared with `==` / `!=` / `<` / `>` or handed to a string ' + 'method — can therefore never be supplied by any request.', + 'Of the kernel keys, `current_user.organization_id` is the tenant, `current_user.id` the acting user ' + + 'and `current_user.email` their unique address. A key meant as an app-resolved set must be staged ' + + 'into `ExecutionContext.rlsMembership` by an `IRlsMembershipResolver` that DECLARES it, and can then ' + + 'only be tested with `in` (`FIELD in current_user.KEY`), never compared with `==`: the runtime stages ' + + 'membership sets as arrays and never as scalars.', 'The compiler answers `unresolved-variable` for it in EVERY position — including under `!` and in a ' + 'trailing `||` arm — so `RLSCompiler` DROPS the policy at request time, and one WARN line is the only ' + 'signal.', @@ -1650,9 +1663,16 @@ const VALIDATION_RULE_JSON_SCHEMA_UNKNOWN_FORMAT_EXPLANATION: RuleExplanation = 'the write path and at the publish gate alike. The schema compiles, the rule ships and runs on every ' + 'write, its other keywords (`type`, `required`) are enforced, and this constraint is enforced on no ' + 'record, ever. The record is ACCEPTED, the silent direction, so nothing downstream reports the gap.', - 'The registered names are read off a live instance of that ajv, never written down, so the rule ' + - 'follows `ajv-formats` across an upgrade. Names are case-sensitive and hyphenated (`date-time`, not ' + - '`datetime`); the `fix:` line names the nearest registered one and lists them all.', + 'The rule\'s registered names are read off a live instance of that ajv, never a list written into the ' + + 'rule, so it follows `ajv-formats` across an upgrade. Names are case-sensitive and hyphenated ' + + '(`date-time`, not `datetime`); the `fix:` line names the nearest registered one.', + // Held equal to `registeredFormatNames()` by `validate-rule-schema-formats.test.ts`. + 'The registered names (the default `ajv-formats` set, the one `rule-validator.ts` registers so that a ' + + '`format` is enforced on every write): binary, byte, date, date-time, double, duration, email, float, ' + + 'hostname, int32, int64, ipv4, ipv6, iso-date-time, iso-time, json-pointer, json-pointer-uri-fragment, ' + + 'password, regex, relative-json-pointer, time, uri, uri-reference, uri-template, url, uuid.', + 'If you meant a constraint ajv has no format for, express it with `pattern` instead: a regex is ' + + 'enforced, an unknown format name is not.', 'Only positions JSON Schema defines as subschemas are walked (`properties`, `items`, `$defs`, ' + '`allOf` / `anyOf` / `oneOf`, `if` / `then` / `else` and the rest), because those are the places ' + 'ajv applies the keyword; a `format` inside `default`, `const`, `enum` or `examples` is data. A ' + @@ -1699,6 +1719,9 @@ const MEASURE_AGGREGATE_FIELD_TYPE_REFUSED_EXPLANATION: RuleExplanation = { 'boolean, toggle; `avg`: number, currency, percent, rating, slider, progress, summary, boolean, ' + 'toggle; `min` and `max`: number, currency, percent, rating, slider, progress, summary, date, ' + 'datetime, time, boolean, toggle.', + 'So the fix is a field of an accepted type, or an aggregate the field\'s declaration accepts. A ' + + 'quantity that must be added up or averaged has to be STORED as a numeric field (a computed ' + + 'column) and aggregated as one.', JSON_STORED_COLUMN, 'The per-type row cannot see `multiple: true`, so `count_distinct` over a multi-capable field ' + 'flagged that way is refused by the declaration: the value is a list stored as JSON, and ' + @@ -1726,6 +1749,10 @@ const DIMENSION_JSON_STORED_FIELD_REFUSED_EXPLANATION: RuleExplanation = { 'JSON value is no group key the SQL dialects share: one groups each serialized value apart, another ' + 'refuses the statement. Every report, dashboard or query that selects the dimension gets that ' + 'refusal instead of an answer, and no selection of it is served as a group.', + 'For a structured-JSON column, store the part of the document you group on in a field of its own ' + + 'and point the dimension at that field. For a multi-value field, filter by one member instead of ' + + 'grouping: a record query on the field\'s object with `where { FIELD: { $contains: VALUE } }` ' + + 'counts or lists the records that hold VALUE, one query per member.', JSON_STORED_COLUMN, ANALYTICS_MEMBER_WALK, ], @@ -2004,9 +2031,11 @@ const PREDICATE_PATH_UNRESOLVED_EXPLANATION: RuleExplanation = { covers: 'how a predicate path is resolved, and where it stops', paragraphs: [ PREDICATE_FAIL_OPEN, - 'A `data.`-rooted path is walked segment by segment through the target schema; the first segment ' + - 'the schema does not declare at that point is the verdict. The syntax rules cannot see this: ' + - '`data.tpye == \'formula\'` parses, is rooted, and is rooted on the right root.', + 'Every reference must resolve against the schema the form edits. A `data.`-rooted path is walked ' + + 'segment by segment through that schema; the first segment it does not declare at that point is the ' + + 'verdict, and the `fix:` line names the nearest declared key, or up to twelve of the keys declared ' + + 'there. The syntax rules cannot see this: `data.tpye == \'formula\'` parses, is rooted, and is ' + + 'rooted on the right root.', PREDICATE_FORM_SCOPE, 'Where the walk stops — a missed catch, never a false error: a scope that is not key-bearing (a ' + 'record map\'s values, `unknown`, `any`, an array reached by `.`), below which nothing is ' + @@ -2021,9 +2050,9 @@ const PREDICATE_PATH_UNROOTED_EXPLANATION: RuleExplanation = { covers: 'why a bare schema key in a form predicate never matches', paragraphs: [ 'A metadata-editing form binds the row under edit as `data`, at every depth — inside a repeater ' + - '`data` is the ROW, still spelled `data` — and never flattens its values to top level. A bare ' + - 'identifier that IS a key of the edited schema is that key with its root dropped: it resolves ' + - 'to nothing.', + '`data` is the ROW, still spelled `data`: there is no implicit row scope — and never flattens its ' + + 'values to top level. A bare identifier that IS a key of the edited schema is that key with its ' + + 'root dropped: it resolves to nothing.', PREDICATE_FAIL_OPEN, 'Narrower than `visibility-bare-identifier` on purpose: the schema-key membership makes the ' + 'verdict unambiguous, and it reaches an identifier CEL itself declares, such as `type`, which ' + @@ -2050,10 +2079,13 @@ const PREDICATE_RHS_PATH_SHAPED_EXPLANATION: RuleExplanation = { 'breaks when this surface moves to the real CEL evaluator, where a bare `active` is an ' + 'undeclared reference.', 'The bare word also reads as a `data.` root someone dropped, so the finding carries both ' + - 'readings and the `fix:` line names both spellings. Adding the root in place (`== data.active`) ' + - 'is not one of them: it is a path on the right, this id\'s error arm. For that one position ' + - '`visibility-bare-identifier` and `predicate-path-unrooted` stand down, so the author gets one ' + - 'prescription rather than three contradicting ones.', + 'readings and the `fix:` line names both spellings: quote the word (`== \'active\'`), which is ' + + 'what the surface renders today and so changes no behaviour, or move the field to the LEFT with a ' + + 'literal on the right (`data.active == \'yes\'`). Adding the root in place (`== data.active`) ' + + 'is not one of them: it is a path on the right, this id\'s error arm, parsed as the literal string ' + + '"data.active" and so FALSE on every row. There is no spelling that compares two paths here. For ' + + 'that one position `visibility-bare-identifier` and `predicate-path-unrooted` stand down, so the ' + + 'author gets one prescription rather than three contradicting ones.', 'The right-hand grammar is the console\'s own path-shaped-literal pattern, so the producer and the ' + 'renderer refuse and warn about the same set; `true`, `false`, `null`, numbers and quoted strings ' + 'are literals and never reported. The check asks about a POSITION, not a resolution, so it runs ' + @@ -2645,6 +2677,11 @@ const FILTER_EMPTY_NODE_EXPLANATION: RuleExplanation = { 'branches that survive, which is why the runtime absorbs). As a branch of `$and` it is the ' + 'AND identity and contributes no condition, indistinguishable from a branch whose condition ' + 'was lost in an edit. An error: no reading of the literal behaves as written.', + 'To express "no filter", DELETE the key — an absent `filter` and a filter that reduces to TRUE run ' + + 'identically, and only the absent key says so to the next reader (and to the next AI author that ' + + 'copies this metadata). If you really do want a predicate that selects nothing, ' + + '`{ : { $in: [] } }` is the declared spelling for it (an empty `$in` list matches nothing, on ' + + 'every backend). Otherwise delete the empty branch, or write the condition it was meant to carry.', EMPTY_FILTER_SCOPE, ], }; @@ -2960,31 +2997,73 @@ const OBJECT_FIELD_REF_UNKNOWN_EXPLANATION: RuleExplanation = { // ── Object-name references (`validate-object-references.ts`) ──────────────── +/** The sites judged — both object-reference ids. */ +const OBJECT_REFERENCE_SITES = + 'Judged: the object names no other rule resolves, each a plain string the schema ships as ' + + 'written: a `lookup`, `master_detail` or `user` field\'s `reference`; a view container\'s own ' + + '`object`; an action param\'s `reference` and `objectOverride`; a dashboard global filter\'s ' + + '`optionsFrom.object`; a dataset\'s base `object`; and a nav entry\'s `requiresObject`, with ' + + 'the `objectName` of an entry that carries one. Interpolated names (`${…}`) resolve at render ' + + 'time and are skipped.'; + +/** The resolution ladder — both object-reference ids. Held to `PLATFORM_OBJECT_PREFIXES` by the rule's test. */ +const OBJECT_REFERENCE_LADDER = + 'Each name climbs one ladder: it resolves when this stack, or a package in the same artifact ' + + '(`packages[]`, ADR-0130 D4), defines it, or when it is a known platform object ' + + '(`PLATFORM_PROVIDED_OBJECT_NAMES` in `@objectstack/spec/system`, the objects the platform\'s ' + + 'packages, official plugins and cloud runtime register). An unresolved name with no platform ' + + 'prefix is `object-reference-unknown`, an error. An unresolved name WITH a platform prefix ' + + '(`sys_`, `cloud_`, `ai_`) is `object-reference-unregistered-platform`: no package we know of ' + + 'registers it, but a third-party package still might, so a warning is the honest ceiling.'; + +/** What a miss costs, per site — both object-reference ids. Held to the rule's sites by its test. */ +const OBJECT_REFERENCE_MISS_COST = + 'What a miss costs, per site. A relationship field: the record picker has no object to query, ' + + '`$expand` has nothing to resolve, and the form renders a relationship control that can never ' + + 'resolve a value. A view container: the runtime indexes a container\'s views by this key ' + + '(`getViewsByObject()` / `GET /meta/view?object=`), so none of its views appears for any object. An ' + + 'action param\'s `reference`: the picker degrades to a raw record-id text input. Its ' + + '`objectOverride`: the param inherits its type and options from a field on that object, so it is ' + + 'left untyped. A dashboard filter\'s options source: the dropdown renders an always-empty filter. A ' + + 'dataset\'s base object: a dataset is the FROM of every report and dashboard widget bound to it ' + + '(ADR-0021), so every one of them queries nothing. A nav entry\'s `requiresObject`: the entry is ' + + 'hidden unless the object is registered, so a typo hides it for good, and it suppresses the nav ' + + 'cross-reference check that would have caught the target; the gated `objectName` is exempt from ' + + 'the build-time check, so it is verified only here.'; + +const OBJECT_REFERENCE_UNKNOWN_EXPLANATION: RuleExplanation = { + rule: 'object-reference-unknown', + covers: 'how an object name resolves, and what a miss costs', + paragraphs: [ + // The sites are held to the rule by its test, each fired. + OBJECT_REFERENCE_SITES, + OBJECT_REFERENCE_LADDER, + 'An error: a stack\'s own objects are namespace-prefixed and present, so an unprefixed name that ' + + 'resolves nowhere has no legitimate elsewhere. Nothing reports the miss at run time.', + OBJECT_REFERENCE_MISS_COST, + 'To fix it, point the reference at an object this stack or its artifact defines, or at a platform ' + + 'object by its full name (the platform user object is `sys_user`, not `user`). The verdict names ' + + 'the nearest defined object when one is close; a view container written with an object\'s short ' + + 'name in a namespaced package is told the prefixed spelling.', + ], +}; + const OBJECT_REFERENCE_UNREGISTERED_PLATFORM_EXPLANATION: RuleExplanation = { rule: 'object-reference-unregistered-platform', covers: 'how an object name resolves, and why this rung only warns', paragraphs: [ // The sites are held to the rule by its test, each fired. - 'Judged: the object names no other rule resolves, each a plain string the schema ships as ' + - 'written: a `lookup`, `master_detail` or `user` field\'s `reference`; a view container\'s own ' + - '`object`; an action param\'s `reference` and `objectOverride`; a dashboard global filter\'s ' + - '`optionsFrom.object`; a dataset\'s base `object`; and a nav entry\'s `requiresObject`, with ' + - 'the `objectName` of an entry that carries one. Interpolated names (`${…}`) resolve at render ' + - 'time and are skipped.', + OBJECT_REFERENCE_SITES, // The prefixes are held to `PLATFORM_OBJECT_PREFIXES` by the rule's test. - 'Each name climbs one ladder: it resolves when this stack, or a package in the same artifact ' + - '(`packages[]`, ADR-0130 D4), defines it, or when it is a known platform object ' + - '(`PLATFORM_PROVIDED_OBJECT_NAMES` in `@objectstack/spec/system`, the objects the platform\'s ' + - 'packages, official plugins and cloud runtime register). An unresolved name with no platform ' + - 'prefix is `object-reference-unknown`, an error. An unresolved name WITH a platform prefix ' + - '(`sys_`, `cloud_`, `ai_`) is this rule: no package we know of registers it, but a ' + - 'third-party package still might, so a warning is the honest ceiling.', + OBJECT_REFERENCE_LADDER, 'If nothing provides the name at run time, the reference resolves to nothing and fails ' + - 'silently: a record picker with no object to query, a view container whose views are never ' + - 'found, a filter dropdown that stays empty, a dataset whose reports query nothing, a nav entry ' + - 'hidden for good. A guess by prefix alone cannot tell `sys_user` (real) from ' + - '`sys_approval_process` (removed when approval became a flow node, ADR-0019), which is why the ' + - 'ladder reads the registry.', + 'silently, at a cost each site spells out below. A guess by prefix alone cannot tell `sys_user` ' + + '(real) from `sys_approval_process` (removed when approval became a flow node, ADR-0019), which is ' + + 'why the ladder reads the registry.', + OBJECT_REFERENCE_MISS_COST, + 'So check the spelling against the object the providing package registers (the approval request ' + + 'object is `sys_approval_request`); if a third-party package genuinely provides the name, the ' + + 'warning is expected.', ], }; @@ -3036,13 +3115,21 @@ const ACTION_NAME_UNDEFINED_EXPLANATION: RuleExplanation = { 'without a word, and the header draws no button, with only a browser-console warning. An ' + 'error: there is no cross-package escape to soften it with, and an action another installed ' + 'package contributes is the one legitimate miss.', + 'To fix a miss, define the action — a `stack.actions` entry or an entry of the object\'s `actions` — ' + + 'placed where the surface looks for it, or remove the reference. A `page:header` draws only actions ' + + 'placed at `record_header` or `record_more`, so a header action needs one of the two in its ' + + '`locations`; the selection bar and an alert\'s call-to-action place an action by name, with no ' + + '`locations` entry.', // The drawn locations are held to the rule by its test, over every location. 'A `record:related_list` is the exception, and resolves the way its renderer does: against the ' + 'RELATED object\'s own actions (written on it, or a `stack.actions` entry bound to it by ' + '`objectName`), never the page\'s object or a global action, and only an action placed at a ' + 'location the list draws (`list_toolbar` as a header button, `list_item` and `record_related` ' + 'as a row-menu item): naming it in `actions` is not a placement. An id that misses either test ' + - 'draws no button, only a refusal notice above the list. A related object this stack does not ' + + 'draws no button, only a refusal notice above the list. So define the action on the related ' + + 'object (in its `actions`, or in `stack.actions` with `objectName` naming it) with one of those ' + + 'locations, or name one of the related object\'s own actions; ignore the error only if another ' + + 'installed package binds the action to that object. A related object this stack does not ' + 'define is not judged: its actions live in another package.', 'Not judged elsewhere: whether a view\'s action belongs to the view\'s own object, and whether ' + 'it declares the location the surface filters on. Both are real but distinct questions, and ' + @@ -3209,12 +3296,19 @@ const AI_SKILL_TOOL_UNRESOLVED_EXPLANATION: RuleExplanation = { 'of that universe starts with its prefix.', // The headless paths are held to the rule by its test, each one materialising. 'An action materialises as a tool only when it opts in, `ai.exposed: true` with an ' + - '`ai.description` (ADR-0011), and has a headless path: a declarative `operation: \'update\'`, ' + - 'a `script` action with a target or a body, or an `api` or `flow` action with a target. A ' + - '`url`, `modal` or `form` action is UI-only and stays human-driven by design. Naming ' + + '`ai.description` (ADR-0011) of at least 40 characters, which the model reads, and has a headless ' + + 'path: a declarative `operation: \'update\'`, a `script` action with a target or a body, or an ' + + '`api` or `flow` action with a target. A `url`, `modal` or `form` action is UI-only and stays ' + + 'human-driven by design: that is a legitimate answer, not a gap, and the skill\'s instructions can ' + + 'recommend the UI action instead. Naming ' + '`action_` for an action that exists but never materialises is its own verdict with its ' + 'own fix (opt it in, or drop the reference), because it is a different mistake from naming a ' + 'tool that exists nowhere.', + // Held equal to `PLATFORM_TOOL_FAMILY_PREFIXES` by the rule's test. + 'Family prefixes materialised by the runtime: action_. A materialised tool needs no tool record ' + + '(the ADR-0109 default path), so backing a name means declaring the action, or referencing a ' + + 'platform tool by its registered name; otherwise remove the name and the instructions that mention ' + + 'it. Ignore the warning only if a runtime plugin outside the platform registry provides the name.', 'The runtime drops an unresolved name without a word, so the skill\'s instructions promise a ' + 'capability the agent cannot call, and the assistant improvises or fails when asked to use it. ' + 'A warning, not an error (ADR-0078\'s advisory-first ratchet): a runtime plugin outside the ' + @@ -3281,13 +3375,37 @@ const MANAGED_API_METHOD_UNAFFORDABLE_EXPLANATION: RuleExplanation = { // ── Organization-axis red lines (`validate-org-axis-red-lines.ts`) ───────── +/** What ADR-0105 D6 protects — both org-axis ids. */ +const ORG_AXIS_RED_LINES = + 'ADR-0105 makes the organization tree a reporting and grouping dimension, never a permission ' + + 'hierarchy, and D6 draws two red lines to keep it so: no permission inheritance along the ' + + 'organization tree (`org-axis-permission-inheritance`), and business-unit trees stay inside one ' + + 'organization (`org-axis-cross-org-bu-grant`).'; + +const ORG_AXIS_PERMISSION_INHERITANCE_EXPLANATION: RuleExplanation = { + rule: 'org-axis-permission-inheritance', + covers: 'why access never walks the organization tree', + paragraphs: [ + ORG_AXIS_RED_LINES, + 'An RLS policy (`using` or `check`) or a sharing rule (`condition` or `sharedWith`) that reads ' + + '`parent_organization_id` builds a second permission hierarchy beside the business-unit tree. It ' + + 'would not grant what it appears to either: a Layer-1 policy cannot widen the engine\'s Layer 0 ' + + 'wall, which it sits behind.', + 'Cross-organization visibility comes from MEMBERSHIP: under the `group` tenancy posture the Layer 0 ' + + 'wall is `organization_id IN accessible_org_ids`, so a user who should see several organizations is ' + + 'made a member of each (ADR-0105 D2). For a hierarchy INSIDE one organization, use the business-unit ' + + 'tree: `unit_and_subordinates` sharing, or a depth scope anchored on `sys_user_position`.', + 'An error: it mirrors a real enforcement property, the Layer 0 wall\'s independence, so the failure ' + + 'moves from a rule that silently grants nothing to an authoring fix.', + ], +}; + const ORG_AXIS_CROSS_ORG_BU_GRANT_EXPLANATION: RuleExplanation = { rule: 'org-axis-cross-org-bu-grant', covers: 'why business-unit trees stay inside one organization', paragraphs: [ - 'ADR-0105 makes the organization tree a reporting and grouping dimension, never a permission ' + - 'hierarchy, and D6 draws two red lines to keep it so. This is the second (the first is ' + - '`org-axis-permission-inheritance`): business-unit trees stay inside one organization. ' + + ORG_AXIS_RED_LINES, + 'This is the second line: business-unit trees stay inside one organization. ' + '`sys_business_unit` is org-scoped, and every business-unit mechanism (`unit_and_subordinates` ' + 'sharing, `adminScope` delegation, depth scopes) resolves within one organization.', // The recipients and both opt-out spellings are held to the rule by its test. @@ -3520,6 +3638,7 @@ export const RULE_EXPLANATIONS: Readonly> = Obje [NAV_OBJECT_UNSERVABLE_EXPLANATION.rule]: NAV_OBJECT_UNSERVABLE_EXPLANATION, [NAV_TARGET_UNRESOLVED_EXPLANATION.rule]: NAV_TARGET_UNRESOLVED_EXPLANATION, [OBJECT_FIELD_REF_UNKNOWN_EXPLANATION.rule]: OBJECT_FIELD_REF_UNKNOWN_EXPLANATION, + [OBJECT_REFERENCE_UNKNOWN_EXPLANATION.rule]: OBJECT_REFERENCE_UNKNOWN_EXPLANATION, [OBJECT_REFERENCE_UNREGISTERED_PLATFORM_EXPLANATION.rule]: OBJECT_REFERENCE_UNREGISTERED_PLATFORM_EXPLANATION, [CAPABILITY_REFERENCE_UNKNOWN_EXPLANATION.rule]: CAPABILITY_REFERENCE_UNKNOWN_EXPLANATION, [ACTION_NAME_UNDEFINED_EXPLANATION.rule]: ACTION_NAME_UNDEFINED_EXPLANATION, @@ -3531,6 +3650,7 @@ export const RULE_EXPLANATIONS: Readonly> = Obje [AI_SKILL_TOOL_UNRESOLVED_EXPLANATION.rule]: AI_SKILL_TOOL_UNRESOLVED_EXPLANATION, [FLOW_FILTER_TOKEN_UNKNOWN_EXPLANATION.rule]: FLOW_FILTER_TOKEN_UNKNOWN_EXPLANATION, [MANAGED_API_METHOD_UNAFFORDABLE_EXPLANATION.rule]: MANAGED_API_METHOD_UNAFFORDABLE_EXPLANATION, + [ORG_AXIS_PERMISSION_INHERITANCE_EXPLANATION.rule]: ORG_AXIS_PERMISSION_INHERITANCE_EXPLANATION, [ORG_AXIS_CROSS_ORG_BU_GRANT_EXPLANATION.rule]: ORG_AXIS_CROSS_ORG_BU_GRANT_EXPLANATION, [PERMISSION_RETIRED_LIFECYCLE_RESIDUE_EXPLANATION.rule]: PERMISSION_RETIRED_LIFECYCLE_RESIDUE_EXPLANATION, [SEED_INSERT_MODE_DUPLICATES_ON_REPLAY_EXPLANATION.rule]: SEED_INSERT_MODE_DUPLICATES_ON_REPLAY_EXPLANATION, diff --git a/packages/lint/src/validate-action-name-refs.test.ts b/packages/lint/src/validate-action-name-refs.test.ts index 37d444f9a00..74c205e3133 100644 --- a/packages/lint/src/validate-action-name-refs.test.ts +++ b/packages/lint/src/validate-action-name-refs.test.ts @@ -14,7 +14,7 @@ import { explainRule } from './rule-explanations.js'; // one line of at most 200 characters — every firing variant this suite // exercises, not a chosen few. Run the whole file: those cases read what the // cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateActionNameRefs: typeof validateActionNameRefsUnrecorded = (...args) => { const findings = validateActionNameRefsUnrecorded(...args); fired.push(...findings); @@ -763,7 +763,12 @@ describe('[#22161] one-line verdicts — action-name-undefined', () => { it('the locations the explanation says a related list draws are the ones the rule accepts', () => { // The explanation module imports nothing, so its location list is held to // the rule by running the rule over every location the spec declares. - const text = explainRule(ACTION_NAME_UNDEFINED)!.paragraphs.join('\n'); + // [#22161] Read from the related-list paragraph: the fix paragraph names + // the page header's two locations, which a related list does not draw. + const text = explainRule(ACTION_NAME_UNDEFINED)!.paragraphs.find((p) => + p.startsWith('A `record:related_list` is the exception'), + )!; + expect(text).toBeDefined(); const drawn = ACTION_LOCATIONS.filter((location) => validateActionNameRefsUnrecorded({ objects: [{ name: 'crm_contact', fields: {}, actions: [{ name: 'act', type: 'script', locations: [location] }] }], pages: [{ @@ -804,3 +809,31 @@ describe('[#22161] one-line verdicts — action-name-undefined', () => { } }); }); + +// [#22161] Slice 12: the `fix:` line. Each arm names its remedy in one line; +// the verdict's "Did you mean" names the nearest action, so neither the stack's +// action list nor the related object's is echoed, and the binding, the drawn +// locations and the other-package exception are the explanation's. +describe('[#22161] one-line fixes — action-name-undefined', () => { + it('every `fix:` line the cases in this file fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => f.rule === ACTION_NAME_UNDEFINED); + // Coverage control: the generic arm with both placements, and the two + // related-list arms (not an action of the related object; not placed). + expect(converted.some((f) => f.hint.includes('with the location this surface needs'))).toBe(true); + expect(converted.some((f) => f.hint.includes('the selection bar places it by name'))).toBe(true); + expect(converted.some((f) => f.hint.includes('at a location a related list draws'))).toBe(true); + expect(converted.some((f) => f.hint.startsWith('Add one of '))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + expect(f.hint, 'no action list is echoed').not.toMatch(/Defined actions:|Actions of "/); + } + }); + + it('`os explain` carries what the `fix:` lines no longer say', () => { + const text = explainRule(ACTION_NAME_UNDEFINED)!.paragraphs.join('\n'); + expect(text).toContain('in `stack.actions` with `objectName` naming it'); + expect(text).toContain('ignore the error only if another installed package binds the action to that object'); + expect(text).toContain('an action another installed package contributes is the one legitimate miss'); + }); +}); diff --git a/packages/lint/src/validate-action-name-refs.ts b/packages/lint/src/validate-action-name-refs.ts index a94d1273619..b1183f51b13 100644 --- a/packages/lint/src/validate-action-name-refs.ts +++ b/packages/lint/src/validate-action-name-refs.ts @@ -251,11 +251,12 @@ export function validateActionNameRefs(stack: AnyRec): ActionNameRefFinding[] { message: `${surface} names action "${name}", which no action in this stack defines, so ${consequence}.` + suggestName(name, known), - hint: - `Define an action named "${name}" (in \`stack.actions\` or the object's \`actions\`) ` + - `${placement}, remove the reference, or ignore this if the ` + - `action is contributed by another installed package.` + - (known.size > 0 ? ` Defined actions: ${[...known].sort().join(', ')}.` : ''), + // [#22161] One remedy line. The verdict's "Did you mean" names the + // nearest defined action, so the stack's whole action list is not + // echoed; where an action may be defined, why the header needs one of + // its two locations, and that another installed package may contribute + // it are `os explain action-name-undefined`. + hint: `Define an action named "${name}" ${placement}, or remove the reference`, }); }; @@ -397,12 +398,14 @@ export function validateActionNameRefs(stack: AnyRec): ActionNameRefFinding[] { : '(defined nowhere in this stack)') + ', so the list draws no button for it.' + suggestName(id, childNames), + // [#22161] One remedy line. The verdict's "Did you mean" names the + // nearest of the related object's actions, so their list is not + // echoed; how an action is bound to the related object, the + // locations a related list draws and the other-package exception + // are `os explain action-name-undefined`. hint: - `Define "${id}" on "${child}" — in that object's \`actions\`, or in \`stack.actions\` with ` + - `\`objectName: '${child}'\` — with one of ${RELATED_LIST_PLACEMENTS} in its \`locations\`; ` + - `or name one of "${child}"'s own actions; or remove the reference. Ignore this only if ` + - `another installed package binds the action to "${child}".` + - ` Actions of "${child}": ${childNames.length > 0 ? childNames.join(', ') : '(none)'}.`, + `Define "${id}" on "${child}" at a location a related list draws, name one of "${child}"'s ` + + 'own actions instead, or remove the reference', }); continue; } @@ -499,7 +502,7 @@ export function validateActionNameRefs(stack: AnyRec): ActionNameRefFinding[] { where, `${path}.properties.actions[${hi}]`, 'Page-header actions', - 'with `record_header` or `record_more` in its `locations` (the header draws only actions placed at one of the two)', + 'with `record_header` or `record_more` in its `locations`', 'the header draws no button for it', ); } diff --git a/packages/lint/src/validate-ai-tool-references.test.ts b/packages/lint/src/validate-ai-tool-references.test.ts index 2f355007521..5c5c6c4b921 100644 --- a/packages/lint/src/validate-ai-tool-references.test.ts +++ b/packages/lint/src/validate-ai-tool-references.test.ts @@ -5,6 +5,7 @@ import { validateAiToolReferences as validateAiToolReferencesUnrecorded, AI_SKILL_TOOL_UNRESOLVED, } from './validate-ai-tool-references.js'; +import { PLATFORM_TOOL_FAMILY_PREFIXES } from '@objectstack/spec/system'; import { explainRule } from './rule-explanations.js'; // [#22161] Each finding is one verdict sentence; the reasoning it used to @@ -13,7 +14,7 @@ import { explainRule } from './rule-explanations.js'; // verdict to one line of at most 200 characters — every firing variant this // suite exercises, not a chosen few. Run the whole file: those cases read what // the cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateAiToolReferences: typeof validateAiToolReferencesUnrecorded = (...args) => { const findings = validateAiToolReferencesUnrecorded(...args); fired.push(...findings); @@ -98,7 +99,8 @@ describe('validate-ai-tool-references', () => { it('does NOT resolve a UI-only action type, even when opted in', () => { // `modal`/`form`/`url` have no headless invocation path — staying - // human-driven is a legitimate design answer, and the hint says so. + // human-driven is a legitimate design answer, and `os explain` says so + // ([#22161]: the one-line `fix:` names the UI-action route). for (const type of ['modal', 'form', 'url']) { const stack = { actions: [{ ...exposed('escalate_case'), type }], @@ -106,8 +108,11 @@ describe('validate-ai-tool-references', () => { }; const findings = validateAiToolReferences(stack); expect(findings, type).toHaveLength(1); - expect(findings[0].hint).toContain('stays human-driven by design'); + expect(findings[0].hint).toContain('recommend the UI action'); } + expect(explainRule(AI_SKILL_TOOL_UNRESOLVED)!.paragraphs.join('\n')).toContain( + 'stays human-driven by design: that is a legitimate answer, not a gap', + ); }); it('does NOT resolve an exposed action missing its LLM-facing description', () => { @@ -329,3 +334,27 @@ describe('[#22161] one-line verdicts — ai-skill-tool-unresolved', () => { } }); }); + +// [#22161] Slice 12: the `fix:` line. One remedy line per arm; the design note +// on UI-only actions, the ADR-0109 default path, the family prefixes and the +// runtime-plugin exception are the explanation's. +describe('[#22161] one-line fixes — ai-skill-tool-unresolved', () => { + it('every `fix:` line the cases in this file fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => f.rule === AI_SKILL_TOOL_UNRESOLVED); + // Coverage control: the opt-in arm and the back-it arm both fired. + expect(converted.some((f) => f.hint.startsWith('Opt "'))).toBe(true); + expect(converted.some((f) => f.hint.startsWith('Back "'))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` names exactly the family prefixes the runtime materialises, and the moved text', () => { + const text = explainRule(AI_SKILL_TOOL_UNRESOLVED)!.paragraphs.join('\n'); + expect(text).toContain(`Family prefixes materialised by the runtime: ${PLATFORM_TOOL_FAMILY_PREFIXES.join(', ')}.`); + expect(text).toContain('the ADR-0109 default path'); + expect(text).toContain('at least 40 characters'); + expect(text).toContain('a runtime plugin outside the platform registry provides the name'); + }); +}); diff --git a/packages/lint/src/validate-ai-tool-references.ts b/packages/lint/src/validate-ai-tool-references.ts index 7e11a379f82..c4e364b6998 100644 --- a/packages/lint/src/validate-ai-tool-references.ts +++ b/packages/lint/src/validate-ai-tool-references.ts @@ -239,18 +239,16 @@ export function validateAiToolReferences(stack: AnyRec): AiToolRefFinding[] { : `Tool "${ref}" resolves to no stack, platform or materialised action tool, so the ` + `runtime drops it and the agent cannot call it.` + suggest(ref, universe), + // [#22161] One remedy line per arm. That a UI-only action stays + // human-driven by design, the ADR-0109 default path, the family + // prefixes the runtime materialises (written out there and held to + // `PLATFORM_TOOL_FAMILY_PREFIXES` by this rule's test) and the + // runtime-plugin exception are `os explain ai-skill-tool-unresolved`. hint: unexposed - ? `Either opt "${unexposed}" in — set \`ai: { exposed: true, description: '…' }\` (≥40 ` + - `chars, LLM-facing) and give it a headless type — or drop the reference and have the ` + - `skill's instructions recommend the UI action instead. A \`modal\`/\`form\`/\`url\` ` + - `action stays human-driven by design; that is a legitimate answer, not a gap.` - : `Back "${ref}" with a real executable: declare a declarative action (or flow), opt it ` + - `in with \`ai.exposed: true\` + \`ai.description\`, and reference its materialised ` + - `tool (\`action_\` — the ADR-0109 default path, no tool record needed); or ` + - `reference a platform tool by its registered name; or remove the reference and the ` + - `instructions that mention it. Ignore this only if a runtime plugin outside the ` + - `platform registry provides "${ref}". Family prefixes materialised by the runtime: ` + - `${PLATFORM_TOOL_FAMILY_PREFIXES.join(', ')}.`, + ? `Opt "${unexposed}" in with \`ai: { exposed: true, description: '…' }\` and a headless type, or ` + + `drop the reference and have the instructions recommend the UI action` + : `Back "${ref}" with an opted-in action (\`ai.exposed: true\`, tool \`action_\`) or a ` + + `registered platform tool, or remove it and the instructions that mention it`, }); } } diff --git a/packages/lint/src/validate-dataset-measure-aggregates.test.ts b/packages/lint/src/validate-dataset-measure-aggregates.test.ts index d5f8b464bd2..facfc760755 100644 --- a/packages/lint/src/validate-dataset-measure-aggregates.test.ts +++ b/packages/lint/src/validate-dataset-measure-aggregates.test.ts @@ -40,7 +40,7 @@ import { // 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: those cases read // what the cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateDatasetMeasureAggregates: typeof validateDatasetMeasureAggregatesUnrecorded = (...args) => { const found = validateDatasetMeasureAggregatesUnrecorded(...args); fired.push(...found); @@ -750,8 +750,10 @@ describe('the cube leg — an analyticsCubes member the analytics door refuses i expect(json?.message).toContain('aggregate "count_distinct" over field "meta"'); expect(json?.message).toContain('`json`'); // The door that refuses it later is the cube's, never the dataset compile leg. - expect(json?.hint).toContain('on the cube with `400 INVALID_FIELD`'); + // [#22161] The one-line `fix:` names no door; `os explain` names both. + expect(json?.hint).not.toContain('INVALID_FIELD'); expect(json?.hint).not.toContain('DATASET_INVALID'); + expect(explanationOf(RULE)).toContain("a cube's by the analytics door when a query names the measure"); const [flagged] = findings(cubeStack({ measures: { distinct_stages: measure('count_distinct', 'stages') } })); expect(flagged?.message).toContain('`select` with `multiple: true`'); @@ -942,8 +944,10 @@ describe('the cube leg — every cube measure is judged by the aggregate × fiel expect(issue.message).toContain('(`text` on object "fx_ledger")'); expect(explanationOf(RULE)).toContain(`\`sum\`: ${AGGREGATE_FIELD_TYPE_COMPATIBILITY.sum.join(', ')};`); // The door that refuses it later is the cube's, never the dataset compile leg. - expect(issue.hint).toContain('on the cube with `400 INVALID_FIELD`'); + // [#22161] The one-line `fix:` names no door; `os explain` names both. + expect(issue.hint).not.toContain('INVALID_FIELD'); expect(issue.hint).not.toContain('DATASET_INVALID'); + expect(explanationOf(RULE)).toContain("a dataset's by its compile (`400 DATASET_INVALID`)"); expect( validateDatasetMeasureAggregates( @@ -1140,3 +1144,46 @@ describe('[#22161] one-line verdicts — the two ids', () => { } }); }); + +// [#22161] Slice 12: the `fix:` line. The measure id names the aggregates the +// declaration accepts, and the dimension id its one remedy per class; why +// `count` reaches further, the stored-numeric route, the doors that refuse the +// pair later and the per-member `$contains` query are the explanations'. +describe('[#22161] one-line fixes — the two ids', () => { + const IDS = [MEASURE_AGGREGATE_FIELD_TYPE_REFUSED, DIMENSION_JSON_STORED_FIELD_REFUSED]; + + it('every `fix:` line the cases above fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => IDS.includes(f.rule)); + // Coverage control: each id fired; the measure id on a type row and on a + // flagged declaration, the dimension id on both classes, on a dataset and a cube. + const measure = converted.filter((f) => f.rule === MEASURE_AGGREGATE_FIELD_TYPE_REFUSED).map((f) => f.hint); + expect(measure.some((h) => / type accepts: /.test(h))).toBe(true); + expect(measure.some((h) => / declaration accepts: /.test(h))).toBe(true); + const dimension = converted.filter((f) => f.rule === DIMENSION_JSON_STORED_FIELD_REFUSED).map((f) => f.hint); + expect(dimension.some((h) => h.startsWith('Group by a field that stores one scalar value'))).toBe(true); + expect(dimension.some((h) => h.includes('`$contains`'))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries what the `fix:` lines no longer say', () => { + const facts: Record = { + [MEASURE_AGGREGATE_FIELD_TYPE_REFUSED]: [ + '`count` accepts every type, because it reads no value', + 'STORED as a numeric field (a computed column)', + "a dataset's by its compile (`400 DATASET_INVALID`)", + "a cube's by the analytics door when a query names the measure (`400 INVALID_FIELD`)", + ], + [DIMENSION_JSON_STORED_FIELD_REFUSED]: [ + 'store the part of the document you group on in a field of its own', + '`where { FIELD: { $contains: VALUE } }`', + 'one query per member', + ], + }; + for (const rule of IDS) { + for (const fact of facts[rule]) expect(explanationOf(rule), `${rule} explanation names ${fact}`).toContain(fact); + } + }); +}); diff --git a/packages/lint/src/validate-dataset-measure-aggregates.ts b/packages/lint/src/validate-dataset-measure-aggregates.ts index f9b7c1614b3..ec169612b5d 100644 --- a/packages/lint/src/validate-dataset-measure-aggregates.ts +++ b/packages/lint/src/validate-dataset-measure-aggregates.ts @@ -350,19 +350,6 @@ interface MemberSite { declarer: string; } -/** - * The last sentence of a measure finding's hint: the door that refuses the - * same pair later, and with what. A dataset's pair is refused when the - * dataset compiles; an authored cube has no compile step, and its pair is - * refused by the analytics door when a query names the measure. - */ -const DATASET_COMPILE_DOOR = - 'The compile leg refuses this same pair with `400 DATASET_INVALID` before any SQL is emitted, ' + - 'so this is the same fix made earlier.'; -const CUBE_QUERY_DOOR = - 'The analytics door refuses this same pair on the cube with `400 INVALID_FIELD` before any SQL is ' + - 'built, so this is the same fix made earlier.'; - /** * [#20890] The finding for a dimension that groups by a JSON-stored column, * in one set of words for a dataset dimension and a cube dimension alike. @@ -391,30 +378,32 @@ function jsonStoredDimensionFinding( ? 'Group by a field that stores one scalar value: store the part of the document you ' + 'group on in a field of its own and point this dimension at that field, or remove ' + 'the dimension.' - : `Filter by one member instead of grouping: a record query on "${leaf.object}" ` + - `with where { "${leaf.field}": { "$contains": VALUE } } counts or lists the records ` + - 'that hold VALUE, one query per member. Point this dimension at a field that stores ' + - 'one value, or remove it.', + : // [#22161] One remedy line; the per-member `$contains` query in full is + // `os explain dimension-json-stored-field-refused`. + 'Point this dimension at a field that stores one value, or remove it; a record query on ' + + `"${leaf.object}" filtering "${leaf.field}" with \`$contains\` counts one member's records`, }; } /** * The finding for a measure whose aggregate the column's declaration cannot * carry ({@link acceptsDeclaration} said no), in one set of words for a - * dataset measure and a cube measure alike. `door` is the hint's last - * sentence ({@link DATASET_COMPILE_DOOR} or {@link CUBE_QUERY_DOOR}). + * dataset measure and a cube measure alike. * * [#22161] One verdict sentence: the pair, the declaration and why it is * refused. `where` names the measure; how backends answer the pair, and each * aggregate's accepted types (the table's rows, written out), are - * `os explain measure-aggregate-field-type-refused`. + * `os explain measure-aggregate-field-type-refused`. One remedy line, too: the + * aggregates this declaration accepts. Why `count` and `count_distinct` reach + * further, the stored-numeric route for a sum or an average, and which door + * refuses the same pair later (a dataset's compile, a cube's query) are the + * same explanation's. */ function refusedMeasureFinding( site: MemberSite, aggregate: string, fieldType: string, shape: ValueShapeFieldDef, - door: string, ): DatasetMeasureAggregateFinding { // [#20890] The row accepts the TYPE and the declaration is what refuses: // a multi-capable field flagged `multiple: true` under `count_distinct`. @@ -435,12 +424,7 @@ function refusedMeasureFinding( hint: `Either point "${aggregate}" at a field of an accepted type, or aggregate ` + `"${site.column}" with one its ${declaredAs(shape)} ${shape.multiple === true ? 'declaration' : 'type'} accepts: ` + - `${aggregatesAccepting(shape).join(', ')}. ` + - `\`count\` accepts every type because it reads no value, and \`count_distinct\` every ` + - `type but the JSON-stored ones — a field declared \`multiple: true\` among them — whose ` + - `values no two backends compare alike; a ` + - `quantity that must be added up or averaged has to be STORED as a ` + - `numeric field (a computed column) and aggregated as one. ${door}`, + `${aggregatesAccepting(shape).join(', ')}.`, }; } @@ -560,7 +544,6 @@ function cubeMemberFindings(cube: AnyRec, cubePath: string, graph: ObjectGraph): aggregate, column.fieldType, column.shape, - CUBE_QUERY_DOOR, ), ); } @@ -664,7 +647,6 @@ export function validateDatasetMeasureAggregates(stack: unknown): DatasetMeasure aggregate, fieldType, shape, - DATASET_COMPILE_DOOR, ), ); }); diff --git a/packages/lint/src/validate-empty-combinators.test.ts b/packages/lint/src/validate-empty-combinators.test.ts index b2b80241347..db90eb2eb7f 100644 --- a/packages/lint/src/validate-empty-combinators.test.ts +++ b/packages/lint/src/validate-empty-combinators.test.ts @@ -111,7 +111,9 @@ describe('validateEmptyCombinators — the literal shapes are refused', () => { expect(findings[0].severity).toBe('error'); expect(findings[0].path).toBe(WIDGET_PATH); expect(findings[0].message).toContain('matches EVERY row'); - expect(findings[0].hint).toContain('DELETE the key'); + expect(findings[0].hint).toContain('Delete the key to express "no filter"'); + // [#22161] Why the absent key is the spelling is `os explain`. + expect(explainRule(FILTER_EMPTY_NODE)!.paragraphs.join('\n')).toContain('DELETE the key'); }); it('refuses a `{}` branch of `$or` by naming the branches it kills', () => { @@ -126,7 +128,10 @@ describe('validateEmptyCombinators — the literal shapes are refused', () => { // Absorption, not narrowing — the distinction #5297 was filed about. expect(findings[0].message).toContain('ABSORBS'); expect(findings[0].message).toContain('matches EVERY row'); - expect(findings[0].hint).toContain('NARROW'); + // [#22161] The one-line `fix:` deletes the branch; why the runtime absorbs + // rather than narrows is `os explain`. + expect(findings[0].hint).toContain('Delete the empty branch'); + expect(explainRule(FILTER_EMPTY_NODE)!.paragraphs.join('\n')).toContain('silently narrow the scope'); }); it('refuses a `{}` branch of `$and` with the AND identity wording', () => { @@ -184,8 +189,8 @@ describe('validateEmptyCombinators — what it leaves alone', () => { }); it('says nothing about the declared match-none spelling it prescribes', () => { - // The hint tells authors to write `{ $in: [] }` when they really do want - // zero rows. A rule that then rejected its own prescription would be worse + // The explanation tells authors to write `{ $in: [] }` when they really do + // want zero rows. A rule that then rejected its own prescription would be worse // than no rule at all. expect(validateEmptyCombinators(widgetStack({ stage: { $in: [] } }))).toEqual([]); }); @@ -446,8 +451,8 @@ describe('[#22161] one-line verdicts — filter-empty-combinator, filter-empty-n // [#22161] Slice 11: the `fix:` line. `filter-empty-combinator` names its // remedy in one line per shape; the declared match-none spelling and the -// programmatic-producer note are its explanation's. (`filter-empty-node` keeps -// its longer root-arm hint: a later slice's.) +// programmatic-producer note are its explanation's. (`filter-empty-node` is +// slice 12's, in the block below.) describe('[#22161] one-line fixes — filter-empty-combinator', () => { it('every `fix:` line of the id the cases above fired is one line of at most 200 characters', () => { const converted = fired.filter((f) => f.rule === FILTER_EMPTY_COMBINATOR); @@ -467,3 +472,27 @@ describe('[#22161] one-line fixes — filter-empty-combinator', () => { expect(text).toContain('A PROGRAMMATIC producer that loops to zero operands keeps the runtime identity unchanged'); }); }); + +// [#22161] Slice 12: the `fix:` line of `filter-empty-node`, one remedy line +// per position; why the absent key is the spelling, the declared match-none +// spelling and why the runtime absorbs are its explanation's. +describe('[#22161] one-line fixes — filter-empty-node', () => { + it('every `fix:` line of the id the cases above fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => f.rule === FILTER_EMPTY_NODE); + // Coverage control: all three positions fired. + for (const opening of ['An EMPTY filter node', 'An EMPTY branch (`{}`) of a `$or`', 'An EMPTY branch (`{}`) of a `$and`']) { + expect(converted.some((f) => f.message.startsWith(opening)), opening).toBe(true); + } + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries what the `fix:` lines no longer say', () => { + const text = explainRule(FILTER_EMPTY_NODE)!.paragraphs.join('\n'); + expect(text).toContain('only the absent key says so to the next reader'); + expect(text).toContain('`{ : { $in: [] } }` is the declared spelling'); + expect(text).toContain('which is why the runtime absorbs'); + }); +}); diff --git a/packages/lint/src/validate-empty-combinators.ts b/packages/lint/src/validate-empty-combinators.ts index ab186319482..c9e9e91bccc 100644 --- a/packages/lint/src/validate-empty-combinators.ts +++ b/packages/lint/src/validate-empty-combinators.ts @@ -199,25 +199,6 @@ function rows(verdict: FilterVerdict): string { return 'carries a real predicate'; } -/** - * The one spelling of "match no row" that is a declared predicate rather than - * an identity nobody can read: an empty `$in` list. - * - * Not invented here — `packages/objectql/src/filter-comparand-shape.ts` names - * `$in: []` / `$nin: []` as "a legitimate, declared predicate — matches nothing - * and matches everything respectively", and deliberately does not refuse them. - */ -const MATCH_NONE_SPELLING = - 'If you really do want a predicate that selects nothing, `{ : { $in: [] } }` is the declared ' + - 'spelling for it (an empty `$in` list matches nothing, on every backend) — it says so where an empty ' + - 'combinator only implies it.'; - -/** The prescription every shape shares: an absent key IS "no filter". */ -const OMIT_THE_KEY = - 'To express "no filter", DELETE the key — an absent `filter` and a filter that reduces to TRUE run ' + - 'identically, and only the absent key says so to the next reader (and to the next AI author that ' + - 'copies this metadata).'; - interface Ctx { where: string; out: EmptyCombinatorFinding[]; @@ -273,9 +254,13 @@ function emitEmptyNode(position: EmptyNodePosition, path: string, ctx: Ctx): voi 'An EMPTY filter node (`{}`) is TRUE, so it ' + `${rows(VERDICT_OF.node)} exactly as if the key were absent: a filter is declared and enforces ` + 'nothing', + // [#22161] One remedy line per position. Why the absent key is the + // spelling, and the declared match-none spelling (`$in: []`, which + // `packages/objectql/src/filter-comparand-shape.ts` names a legitimate + // declared predicate), are `os explain filter-empty-node`. hint: - `${OMIT_THE_KEY} If you meant to constrain something, write the condition into the node. ` + - `${MATCH_NONE_SPELLING}`, + 'Delete the key to express "no filter" (an absent `filter` runs identically), or write the ' + + 'condition into the node', }); return; } @@ -290,9 +275,8 @@ function emitEmptyNode(position: EmptyNodePosition, path: string, ctx: Ctx): voi 'An EMPTY branch (`{}`) of a `$or` is TRUE, and one TRUE disjunct ABSORBS the disjunction ' + `(it ${rows(VERDICT_OF.orWithEmptyBranch)}), so every branch written beside it is dead`, hint: - 'Delete the empty branch — the `$or` then means what it looks like. If it was meant to carry a ' + - 'condition, write it. (A compiler that DROPPED the empty branch instead would silently NARROW ' + - 'the scope to the surviving branches, which is why the runtime absorbs rather than filters.)', + 'Delete the empty branch so the `$or` means what it looks like, or write the condition it was ' + + 'meant to carry', }); return; } @@ -305,9 +289,7 @@ function emitEmptyNode(position: EmptyNodePosition, path: string, ctx: Ctx): voi message: 'An EMPTY branch (`{}`) of a `$and` is TRUE, the AND identity, so it contributes no condition ' + 'and the conjunction means whatever its other branches mean', - hint: - 'Delete the empty branch, or write the condition it was meant to carry. A branch that constrains ' + - 'nothing is indistinguishable from one whose condition was lost in an edit.', + hint: 'Delete the empty branch, or write the condition it was meant to carry', }); } diff --git a/packages/lint/src/validate-object-references.test.ts b/packages/lint/src/validate-object-references.test.ts index 6e47f0c270b..fba3647fa3a 100644 --- a/packages/lint/src/validate-object-references.test.ts +++ b/packages/lint/src/validate-object-references.test.ts @@ -18,7 +18,7 @@ import { explainRule } from './rule-explanations.js'; // suite exercises, not a chosen few. Run the whole file: those cases read what // the cases above fired. (`object-reference-unknown` already prints one // sentence of at most 175 characters here and is not converted.) -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateObjectReferences: typeof validateObjectReferencesUnrecorded = (...args) => { const findings = validateObjectReferencesUnrecorded(...args); fired.push(...findings); @@ -115,8 +115,9 @@ describe('validateObjectReferences — field relationship targets (#16611)', () expect(findings[0].where).toBe('object "crm_lead" · field "zzz_probe"'); expect(findings[0].path).toBe('objects[0].fields.zzz_probe.reference'); expect(findings[0].message).toContain('lookup target "zzz_object_that_does_not_exist"'); - // The hint says what the miss costs at runtime, not just that it is a miss. - expect(findings[0].hint).toContain('record picker'); + // What the miss costs at runtime, not just that it is a miss ([#22161]: + // `os explain`, one line away from the `fix:` line). + expect(explainRule(OBJECT_REFERENCE_UNKNOWN)!.paragraphs.join('\n')).toContain('the record picker has no object to query'); }); it('errors on a master_detail whose reference names an object declared nowhere', () => { @@ -286,9 +287,17 @@ describe('validateObjectReferences — artifact packages[] as resolution context expect(findings[0].severity).toBe('error'); expect(findings[0].rule).toBe(OBJECT_REFERENCE_UNKNOWN); expect(findings[0].path).toBe('objects[0].fields.contract.reference'); - // The remedy lists what the ARTIFACT provides, not only this package's own - // objects — that is the list the author actually has to choose from. - expect(findings[0].hint).toContain('Defined objects: crm_account, crm_order.'); + // The resolution set is what the ARTIFACT provides, not only this package's + // own objects — that is the set the author actually has to choose from. + // [#22161] The verdict's "Did you mean" reads it (a near miss of the + // sibling's object below); the `fix:` line no longer echoes the list. + expect(findings[0].hint).not.toContain('Defined objects'); + const near = { + ...ORDERS_BODY, + objects: [{ name: 'crm_order', fields: { account: { type: 'lookup', reference: 'crm_acount' } } }], + }; + const [miss] = validateObjectReferences(perPackageStack(near, [{ manifest: near }, { manifest: CORE_BODY }])); + expect(miss?.message).toContain('Did you mean "crm_account"?'); }); it('carries the context to every site on the rule, not only the field one', () => { @@ -739,10 +748,12 @@ describe('[#20216] validateObjectReferences — view container `object`', () => // The namespace prescription names the ONE spelling that resolves… expect(f.hint).toContain('manifest.namespace "my_app"'); expect(f.hint).toContain('write "my_app_order_line", not "order_line"'); - // …and the hint still lists every object the stack does declare. - expect(f.hint).toContain('Defined objects: my_app_order, my_app_order_line.'); - // What the miss costs, not only that it is a miss. - expect(f.hint).toContain('getViewsByObject()'); + // [#22161] …as the whole `fix:` line; what the miss costs is `os explain`. + expect(f.hint).toBe( + 'Object names carry the package namespace prefix (manifest.namespace "my_app"): write ' + + '"my_app_order_line", not "order_line".', + ); + expect(explainRule(OBJECT_REFERENCE_UNKNOWN)!.paragraphs.join('\n')).toContain('getViewsByObject()'); }); it('CONTROL — the correctly prefixed container is clean', () => { @@ -765,7 +776,7 @@ describe('[#20216] validateObjectReferences — view container `object`', () => expect(findings[0].path).toBe('views[0].object'); // `my_app_invoice` is not declared, so prefixing is not the fix and is not offered. expect(findings[0].hint).not.toContain('namespace prefix'); - expect(findings[0].hint).toContain('Defined objects: my_app_order, my_app_order_line.'); + expect(findings[0].hint).toContain('Point it at one of this stack\'s objects'); }); it('passes a container over an object a `packages[]` sibling provides (rung ①, artifact scope)', () => { @@ -898,3 +909,52 @@ describe('[#22161] one-line verdicts — object-reference-unregistered-platform' } }); }); + +// [#22161] Slice 12: the `fix:` line, both ids. One remedy line each (the +// namespace prescription, where it applies, IS the line); what a miss costs at +// each site, the ladder and the approval-object example are the explanations' +// (`object-reference-unknown` gains one). +describe('[#22161] one-line fixes — the two object-reference ids', () => { + const IDS = [OBJECT_REFERENCE_UNKNOWN, OBJECT_REFERENCE_UNREGISTERED_PLATFORM]; + + it('every `fix:` line the cases in this file fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => IDS.includes(f.rule)); + // Coverage control: both ids fired, the unknown id with and without the + // namespace prescription. + expect([...new Set(converted.map((f) => f.rule))].sort()).toEqual([...IDS].sort()); + const unknown = converted.filter((f) => f.rule === OBJECT_REFERENCE_UNKNOWN).map((f) => f.hint); + expect(unknown.some((h) => h.startsWith('Point it at one of'))).toBe(true); + expect(unknown.some((h) => h.startsWith('Object names carry the package namespace prefix'))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + expect(f.hint, 'no object list is echoed').not.toContain('Defined objects'); + } + }); + + it('`os explain` names what a miss costs at every site the rule judges', () => { + for (const rule of IDS) { + const entry = explainRule(rule); + expect(entry, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = entry!.paragraphs.join('\n'); + for (const cost of [ + 'the record picker has no object to query', + 'none of its views appears for any object', + 'the picker degrades to a raw record-id text input', + 'so it is left untyped', + 'the dropdown renders an always-empty filter', + 'every one of them queries nothing', + 'so a typo hides it for good', + 'so it is verified only here', + ]) { + expect(text, `${rule} explanation names ${cost}`).toContain(cost); + } + } + expect(explainRule(OBJECT_REFERENCE_UNKNOWN)!.paragraphs.join('\n')).toContain( + 'the platform user object is `sys_user`, not `user`', + ); + expect(explainRule(OBJECT_REFERENCE_UNREGISTERED_PLATFORM)!.paragraphs.join('\n')).toContain( + 'if a third-party package genuinely provides the name, the warning is expected', + ); + }); +}); diff --git a/packages/lint/src/validate-object-references.ts b/packages/lint/src/validate-object-references.ts index ba8ce60b007..cc25a005868 100644 --- a/packages/lint/src/validate-object-references.ts +++ b/packages/lint/src/validate-object-references.ts @@ -217,13 +217,19 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { /** * Resolve one reference through the ladder and record a finding if it fails. * `subject` describes the reference for the message ("record-picker target"). + * `remedy`, when a site has one sharper than the generic line (a view + * container's namespace-prefixed spelling), is the whole `fix:` line. + * + * [#22161] What a miss costs at each site used to close every hint; it is + * the ids' `os explain` entries now (one paragraph, every site), so a hint is + * one remedy line. */ const check = ( target: string | undefined, where: string, path: string, subject: string, - fix: string, + remedy = '', ) => { const name = strName(target); if (!name) return; @@ -246,10 +252,9 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { `this stack does not define it, so it likely resolves to nothing.` + suggestName(name, PLATFORM_NAMES), hint: - `Check the spelling against the object the providing package actually registers ` + - `(e.g. "sys_approval_request", not "sys_approval_process" — the process object ` + - `was removed when approval became a flow node, ADR-0019). If a third-party ` + - `package genuinely provides it, this warning is expected. ${fix}`, + remedy || + 'Check the spelling against the object the providing package registers ("sys_approval_request", ' + + 'not the removed "sys_approval_process"), or ignore this if a third-party package provides it', }); return; } @@ -265,10 +270,13 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { `${subject} "${name}" resolves to no object defined in this stack. ` + `The reference is inert at runtime — nothing reports the miss.` + suggestName(name, resolvable), + // [#22161] One remedy line. The verdict's "Did you mean" names the + // nearest object this stack or its artifact provides, so the whole list + // is not echoed. hint: - `Point it at one of this stack's objects, or at a platform object by its full ` + - `name (the platform user object is "sys_user", not "user"). ${fix}` + - (resolvable.size > 0 ? ` Defined objects: ${[...resolvable].sort().join(', ')}.` : ''), + remedy || + 'Point it at one of this stack\'s objects, or at a platform object by its full name (the platform ' + + 'user object is "sys_user", not "user")', }); }; @@ -328,8 +336,6 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { `object "${objName}" · field "${fieldName}"`, `objects[${oi}].fields.${fieldName}.reference`, `${type} target`, - 'The record picker has no object to query, `$expand` has nothing to resolve, and the ' + - 'form renders a relationship control that can never resolve a value.', ); } } @@ -374,7 +380,7 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { const prefixed = namespace ? `${namespace}_${bound}` : undefined; const namespaceHint = prefixed && !bound.startsWith(`${namespace}_`) && resolvable.has(prefixed) - ? ` Object names carry the package namespace prefix (manifest.namespace "${namespace}"): ` + + ? `Object names carry the package namespace prefix (manifest.namespace "${namespace}"): ` + `write "${prefixed}", not "${bound}".` : ''; check( @@ -382,10 +388,7 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { `view ${strName(view.name) ? `"${strName(view.name)}"` : `#${vi}`}`, `views[${vi}].object`, 'view container object', - 'The runtime indexes a container\'s views by this key (`getViewsByObject()` / ' + - '`GET /meta/view?object=`), so a container bound to an object nothing registers is ' + - 'never found: none of its views appears for any object.' + - namespaceHint, + namespaceHint, ); } @@ -422,14 +425,12 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { where, `${actionPath}.params[${pi}].reference`, 'record-picker target', - 'Without a resolvable target the picker degrades to a raw record-id text input.', ); check( strName(param.objectOverride), where, `${actionPath}.params[${pi}].objectOverride`, 'field-backed param object', - 'The param inherits type/options from a field on this object, so an unknown object leaves it untyped.', ); } }; @@ -474,7 +475,6 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { `dashboard "${dashName}" · filter "${strName(filter.name) ?? `#${fi}`}"`, `dashboards[${di}].globalFilters[${fi}].optionsFrom.object`, 'filter options source', - 'The dropdown fetches its options from this object; an unknown one renders an always-empty filter.', ); } } @@ -499,8 +499,6 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { `dataset "${strName(ds.name) ?? `#${dsi}`}"`, `datasets[${dsi}].object`, 'dataset base object', - 'A dataset is the FROM of every report and dashboard widget bound to it (ADR-0021), so ' + - 'an unknown base object leaves every one of those surfaces querying nothing.', ); } @@ -525,8 +523,6 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { where, `${navPath}.requiresObject`, 'capability gate object', - 'The entry is hidden unless this object is registered, so a typo hides it permanently — ' + - 'and it suppresses the nav cross-reference check that would have caught the target.', ); // A nav target exempted from the `defineStack` throw by `requiresObject` @@ -537,7 +533,6 @@ export function validateObjectReferences(stack: AnyRec): ObjectRefFinding[] { where, `${navPath}.objectName`, 'navigation target', - 'Declaring `requiresObject` exempts this target from the build-time check, so it is only verified here.', ); } diff --git a/packages/lint/src/validate-org-axis-red-lines.test.ts b/packages/lint/src/validate-org-axis-red-lines.test.ts index 13bfd5577de..f873db7c5de 100644 --- a/packages/lint/src/validate-org-axis-red-lines.test.ts +++ b/packages/lint/src/validate-org-axis-red-lines.test.ts @@ -27,7 +27,7 @@ import { explainRule } from './rule-explanations.js'; // file hold each recorded verdict of that id to one line of at most 200 // characters — every firing variant this suite exercises, not a chosen few. // Run the whole file: those cases read what the cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateOrgAxisRedLines: typeof validateOrgAxisRedLinesUnrecorded = (...args) => { const findings = validateOrgAxisRedLinesUnrecorded(...args); fired.push(...findings); @@ -950,3 +950,39 @@ describe('[#22161] one-line verdicts — org-axis-cross-org-bu-grant', () => { } }); }); + +// [#22161] Slice 12: the `fix:` line, both ids. One remedy line each; why +// membership is the only cross-organization route, why Layer 1 cannot widen +// Layer 0, the business-unit tree for an in-organization hierarchy and the +// `public_read` catalog route are the explanations' (the first id gains one). +describe('[#22161] one-line fixes — the two org-axis ids', () => { + const IDS = [ORG_AXIS_PERMISSION_INHERITANCE, ORG_AXIS_CROSS_ORG_BU_GRANT]; + + it('every `fix:` line the cases in this file fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => IDS.includes(f.rule)); + // Coverage control: both ids fired. + expect([...new Set(converted.map((f) => f.rule))].sort()).toEqual([...IDS].sort()); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries what the `fix:` lines no longer say', () => { + const facts: Record = { + [ORG_AXIS_PERMISSION_INHERITANCE]: [ + 'Cross-organization visibility comes from MEMBERSHIP', + '`organization_id IN accessible_org_ids`', + 'a Layer-1 policy cannot widen the engine\'s Layer 0 wall', + '`unit_and_subordinates` sharing, or a depth scope anchored on `sys_user_position`', + ], + [ORG_AXIS_CROSS_ORG_BU_GRANT]: ['wants an organization-wide default of `public_read`, not a business-unit grant'], + }; + for (const rule of IDS) { + const entry = explainRule(rule); + expect(entry, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = entry!.paragraphs.join('\n'); + for (const fact of facts[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + } + }); +}); diff --git a/packages/lint/src/validate-org-axis-red-lines.ts b/packages/lint/src/validate-org-axis-red-lines.ts index 1f3c4278c0b..30085fd6829 100644 --- a/packages/lint/src/validate-org-axis-red-lines.ts +++ b/packages/lint/src/validate-org-axis-red-lines.ts @@ -186,13 +186,15 @@ function isTenancyDisabled(object: AnyRec): boolean { return false; } +/** + * [#22161] One remedy line, both arms. Why membership is the only + * cross-organization route, why a Layer-1 policy cannot widen Layer 0, and the + * business-unit tree for a hierarchy inside one organization are + * `os explain org-axis-permission-inheritance`. + */ const INHERITANCE_HINT = - `Remove the ${ORG_PARENT_FIELD} reference. Cross-organization visibility comes from MEMBERSHIP: ` + - `under the \`group\` tenancy posture the engine's Layer 0 wall is ` + - `\`organization_id IN accessible_org_ids\`, so a user who should see several organizations is made ` + - `a member of them (ADR-0105 D2). A Layer-1 policy cannot widen Layer 0 anyway, so this rule would ` + - `not grant the access it appears to. For a hierarchy INSIDE one organization, use the business-unit ` + - `tree (\`unit_and_subordinates\` sharing, or a depth scope anchored on \`sys_user_position\`).`; + `Remove the ${ORG_PARENT_FIELD} reference: make a user who should see several organizations a ` + + `member of each (Layer 0 reads accessible_org_ids)`; /** * Lint an ObjectStack config for the ADR-0105 D6 organization-axis red lines. @@ -297,11 +299,11 @@ export function validateOrgAxisRedLines(stack: unknown): OrgAxisFinding[] { message: `Recipient \`${recipientType}\` (${reach}) on an object that opted out of tenancy spans ` + `EVERY organization: a cross-org business-unit grant ADR-0105 D6 forbids`, + // [#22161] One remedy line; the `public_read` route for a catalog + // everyone reads is the explanation's. hint: - `Either scope the object to organizations (drop \`tenancy.enabled: false\` so Layer 0 walls it), ` + - `or share it to a \`user\` / \`team\` / \`position\` audience instead — those expand flat, with no ` + - `business-unit tree to resolve. A platform-global catalog that everyone should read wants an OWD ` + - `of \`public_read\`, not a BU grant.`, + 'Scope the object to organizations (drop `tenancy.enabled: false`), or share it to a `user` / ' + + '`team` / `position` audience, which expands flat', }); }); diff --git a/packages/lint/src/validate-predicate-path-refs.test.ts b/packages/lint/src/validate-predicate-path-refs.test.ts index 0d928ed61c5..d08e68b4fa1 100644 --- a/packages/lint/src/validate-predicate-path-refs.test.ts +++ b/packages/lint/src/validate-predicate-path-refs.test.ts @@ -65,7 +65,7 @@ const DemoSchema = z.object({ // this suite exercises (the shipped-corpus block included), not a chosen few. // Run the whole file: those cases read what the cases above fired. const PREDICATE_IDS: readonly string[] = [PREDICATE_PATH_UNRESOLVED, PREDICATE_PATH_UNROOTED, PREDICATE_RHS_PATH_SHAPED]; -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validatePredicatePathRefs: typeof validatePredicatePathRefsUnrecorded = (...args) => { const findings = validatePredicatePathRefsUnrecorded(...args); fired.push(...findings); @@ -883,3 +883,39 @@ describe('[#22161] one-line verdicts — the three predicate ids', () => { for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); }); }); + +// [#22161] Slice 12: the `fix:` line. Each arm names its remedy in one line; +// that every reference resolves against the edited schema, the implicit-row +// note and the subset's reasoning are each id's `os explain` entry. +describe('[#22161] one-line fixes — the three predicate ids', () => { + it('every `fix:` line the cases above fired is one line of at most 200 characters', () => { + // Coverage control: each id fired, the unresolved id on both arms and the + // right-hand id on its dotted (error) and bare-word (warning) arms. + expect([...new Set(fired.map((f) => f.rule))].sort()).toEqual([...PREDICATE_IDS].sort()); + const unresolved = fired.filter((f) => f.rule === PREDICATE_PATH_UNRESOLVED).map((f) => f.hint); + expect(unresolved.some((h) => h.startsWith('Did you mean'))).toBe(true); + expect(unresolved.some((h) => h.startsWith('Reference a key'))).toBe(true); + const rhs = fired.filter((f) => f.rule === PREDICATE_RHS_PATH_SHAPED).map((f) => f.hint); + expect(rhs.some((h) => h.includes('if you meant the path'))).toBe(true); + expect(rhs.some((h) => h.includes('if you meant the field'))).toBe(true); + for (const f of fired) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries what the `fix:` lines no longer say', () => { + const facts: Record = { + [PREDICATE_PATH_UNRESOLVED]: ['Every reference must resolve against the schema the form edits'], + [PREDICATE_PATH_UNROOTED]: ['there is no implicit row scope'], + [PREDICATE_RHS_PATH_SHAPED]: [ + 'so changes no behaviour', + 'FALSE on every row', + 'There is no spelling that compares two paths here', + ], + }; + for (const rule of PREDICATE_IDS) { + for (const fact of facts[rule]) expect(explanationOf(rule), `${rule} explanation names ${fact}`).toContain(fact); + } + }); +}); diff --git a/packages/lint/src/validate-predicate-path-refs.ts b/packages/lint/src/validate-predicate-path-refs.ts index 7fae4b721ab..395acc8ab5b 100644 --- a/packages/lint/src/validate-predicate-path-refs.ts +++ b/packages/lint/src/validate-predicate-path-refs.ts @@ -656,10 +656,13 @@ function checkPredicate( message: `predicate references \`${full}\`, but \`${segment}\` is not a key of \`${container}\`, ` + FAILS_OPEN, + // [#22161] One remedy line: the nearest declared key, or what the + // container declares. That every reference must resolve against + // the edited schema, and where the walk stops, is + // `os explain predicate-path-unresolved`. hint: - `${formatSuggestion(findClosestMatches(segment, step.declared)) - || `\`${container}\` declares: ${step.declared.slice(0, 12).sort().join(', ')}`}` - + ` Every reference must resolve against the schema the form edits.`, + formatSuggestion(findClosestMatches(segment, step.declared)) + || `Reference a key \`${container}\` declares: ${step.declared.slice(0, 12).sort().join(', ')}`, }); break; } @@ -695,10 +698,11 @@ function checkPredicate( message: `predicate names \`${id}\` bare, but it is a key of the edited schema with its \`${ROOT}.\` ` + `root dropped, ${FAILS_OPEN}`, + // [#22161] One remedy line; why the row is spelled `data` at every + // depth is `os explain predicate-path-unrooted`. hint: - `Write \`${ROOT}.${id}\` instead of \`${id}\`. A metadata-editing form binds the row under ` - + `edit as \`${ROOT}\` at every depth — inside a repeater \`${ROOT}\` is the ROW, but it is ` - + `still spelled \`${ROOT}\` (there is no implicit row scope).`, + `Write \`${ROOT}.${id}\` instead of \`${id}\`: the form binds the row under edit as ` + + `\`${ROOT}\` at every depth, a repeater's row included`, }); } } @@ -732,20 +736,15 @@ function checkPredicate( : `predicate compares against the unquoted word \`${text}\` on the RIGHT of \`${op}\`, ` + `which reads as a literal today but is outside the declared subset and breaks under ` + `the real CEL evaluator`, + // [#22161] One remedy line per arm: both spellings, and (for the bare + // word) the in-place rooting it must not become. The supported subset, + // why two paths cannot be compared, and why quoting changes nothing + // today are `os explain predicate-rhs-path-shaped`. hint: dotted - ? `Two sanctioned spellings. (1) If you meant the TEXT, quote it: \`${op} '${text}'\`. ` - + `(2) If you meant the PATH, restructure so the path is on the LEFT and a literal is on ` - + `the right — comparing one path against another is outside the subset this surface ` - + `renders, which is \`path == 'literal'\` / \`path != 'literal'\` and nothing wider. There ` - + `is no third spelling that compares two paths here.` - : `Two sanctioned spellings, and you must pick — they are not the same predicate. ` - + `(1) If you meant the TEXT \`${text}\`, quote it: \`${op} '${text}'\`. That is what this ` - + `renders as today, so it changes no behaviour and is the fix unless you know otherwise. ` - + `(2) If you meant the FIELD \`${ROOT}.${text}\`, move it to the LEFT and put a literal on ` - + `the right, e.g. \`${ROOT}.${text} == 'yes'\`. ⛔ Do NOT simply add the root in place: ` - + `\`${op} ${ROOT}.${text}\` is a path on the RIGHT, which this surface parses as the ` - + `literal string "${ROOT}.${text}" — it is refused by this same rule at \`error\`, and it ` - + `is FALSE on every row. The subset here is \`path == 'literal'\` and nothing wider.`, + ? `If you meant the text, quote it: \`${op} '${text}'\`; if you meant the path, put it on the ` + + `LEFT with a literal on the right (two paths cannot be compared here)` + : `If you meant the text, quote it: \`${op} '${text}'\` (what it does today); if you meant the ` + + `field, write \`${ROOT}.${text} == 'yes'\` with the path on the LEFT. Do NOT simply add the root in place`, }); } } diff --git a/packages/lint/src/validate-readonly-hook-writes.test.ts b/packages/lint/src/validate-readonly-hook-writes.test.ts index a1b5aa541f3..e840e334e6f 100644 --- a/packages/lint/src/validate-readonly-hook-writes.test.ts +++ b/packages/lint/src/validate-readonly-hook-writes.test.ts @@ -671,7 +671,9 @@ describe('validateReadonlyHookWrites - readonlyWhen is a SECOND shape, not the s // hook-derived value on a TRUE readonlyWhen field now LANDS". The hint used // to assert the opposite and thereby rule out the one remedy that works. expect(finding.hint).toContain('beforeUpdate hook'); - expect(finding.hint).toContain('does land, even on a locked record'); + // [#22161] Why it lands is the explanation's, one line away. + const why = explainRule(HOOK_API_UPDATE_READONLY_WHEN_FIELD)!.paragraphs.join('\n'); + expect(why).toContain('does land, even on a locked record'); expect(finding.hint).not.toMatch(/strips even a beforeUpdate-derived value/); // [#14010] The conditional lock is the one place elevation genuinely does // NOT help, so the hint must keep saying so now that the platform HAS a @@ -685,9 +687,12 @@ describe('validateReadonlyHookWrites - readonlyWhen is a SECOND shape, not the s // AND a system context does not waive the conditional lock anyway ("LOCK 2 - // isSystem does NOT exempt a caller-supplied value"). The second reason is // what makes this hint's refusal survive if the first is ever fixed. - expect(finding.hint).toContain('Elevation is not a workaround here'); - expect(finding.hint).toContain('not marshalled into the sandbox'); - expect(finding.hint).toContain('does not waive the conditional lock'); + // [#22161] The `fix:` line refuses both in one clause; the two reasons are + // the explanation's. + expect(finding.hint).toContain("runAs: 'system' and sudo() do not waive this lock"); + expect(why).toContain('elevation is no workaround'); + expect(why).toContain('not marshalled into the sandbox'); + expect(why).toContain('does not waive the conditional lock'); expect(finding.hint).not.toMatch(/write it through ctx\.api\.sudo/); }); @@ -851,7 +856,7 @@ describe('[#22161] one-line verdicts — the rule ids this file shortened', () = // [#22161] Slice 11: the `fix:` line. `hook-api-update-readonly-field` names // its two routes and the sudo() refusal in one line per verb; the reasoning and // the drop-the-field alternatives are its explanation's. -// (`hook-api-update-readonly-when-field` keeps its longer hint: a later slice's.) +// (`hook-api-update-readonly-when-field` is slice 12's, in the block below.) describe('[#22161] one-line fixes — hook-api-update-readonly-field', () => { it('every `fix:` line of the id the cases above fired is one line of at most 200 characters', () => { const converted = firedShortened.filter((f) => f.rule === HOOK_API_UPDATE_READONLY_FIELD); @@ -870,3 +875,25 @@ describe('[#22161] one-line fixes — hook-api-update-readonly-field', () => { expect(text).toContain('drop `readonly: true` from the field'); }); }); + +// [#22161] Slice 12: the `fix:` line of `hook-api-update-readonly-when-field`: +// the two routes and the refusal of elevation, in one line. The reasons and the +// drop-the-field alternative are its explanation's. +describe('[#22161] one-line fixes — hook-api-update-readonly-when-field', () => { + it('every `fix:` line of the id the cases above fired is one line of at most 200 characters', () => { + const converted = firedShortened.filter((f) => f.rule === HOOK_API_UPDATE_READONLY_WHEN_FIELD); + // Coverage control: the id fired. + expect(converted.some((f) => f.hint.startsWith('Derive '))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries the alternative the `fix:` line no longer lists', () => { + const text = explainRule(HOOK_API_UPDATE_READONLY_WHEN_FIELD)!.paragraphs.join('\n'); + expect(text).toContain('otherwise drop the field from the payload'); + expect(text).toContain('The warning never blocks a build'); + expect(text).toContain('a TypeError at run time'); + }); +}); diff --git a/packages/lint/src/validate-readonly-hook-writes.ts b/packages/lint/src/validate-readonly-hook-writes.ts index d3eea726239..81a7d615526 100644 --- a/packages/lint/src/validate-readonly-hook-writes.ts +++ b/packages/lint/src/validate-readonly-hook-writes.ts @@ -485,14 +485,14 @@ export function validateReadonlyHookWrites( message: `body's ${call} writes readonlyWhen field '${w.field}', silently stripped ${READONLY_WHEN_STRIP_SCOPE}` + loc.messageSuffix, + // [#22161] One remedy line: the two routes, and the refusal of + // elevation. Why a derived value lands on a locked record, why no + // system context waives the lock, why sudo() is a TypeError from a + // body, and the drop-the-field alternative are + // `os explain hook-api-update-readonly-when-field`. 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 ` + - `caller-supplied and does land, even on a locked record. Elevation is not a workaround here: ` + - `a system context does not waive the conditional lock (unlike the static readonly strip), so ` + - `neither runAs: 'system' nor ctx.api.sudo() helps - and sudo() is not marshalled into the ` + - `sandbox in any case (calling it from a body is a TypeError at run time). Otherwise ` + - `drop '${w.field}' from this payload. This warning never blocks a build.`, + `Derive '${w.field}' in a beforeUpdate hook on '${objectName}', or target only records whose ` + + `readonlyWhen predicate is FALSE; runAs: 'system' and sudo() do not waive this lock`, }); } } diff --git a/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts index f8e6862b606..a89265ffd7d 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts @@ -373,10 +373,8 @@ describe('[#22161] one-line verdicts', () => { describe('[#22161] one-line fixes', () => { it('every `fix:` line the cases above fired is one line of at most 200 characters', () => { expect(fired.some((f) => f.message.includes('which no comparison class spans'))).toBe(true); - // The three ids this slice converted (the other two RLS ids keep their - // longer hints: a later slice's). - const converted = ['rls-predicate-unenforceable', 'rls-predicate-over-budget', 'rls-predicate-unknown-field']; - for (const f of fired.filter((x) => converted.includes(x.rule))) { + // Every RLS id: slice 11 converted three, slice 12 the other two. + for (const f of fired.filter((x) => x.rule.startsWith('rls-predicate-'))) { expect(f.hint, f.rule).not.toContain('\n'); expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); } diff --git a/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts index 45134ea6c47..cbcc4169fe1 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts @@ -270,10 +270,8 @@ describe('[#22161] one-line verdicts', () => { describe('[#22161] one-line fixes', () => { it('every `fix:` line the cases above fired is one line of at most 200 characters', () => { expect(fired.length).toBeGreaterThan(0); - // The three ids this slice converted (the other two RLS ids keep their - // longer hints: a later slice's). - const converted = ['rls-predicate-unenforceable', 'rls-predicate-over-budget', 'rls-predicate-unknown-field']; - for (const f of fired.filter((x) => converted.includes(x.rule))) { + // Every RLS id: slice 11 converted three, slice 12 the other two. + for (const f of fired.filter((x) => x.rule.startsWith('rls-predicate-'))) { expect(f.hint, f.rule).not.toContain('\n'); expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); } diff --git a/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts index 0069ea1ad2f..d55043f5753 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts @@ -324,10 +324,8 @@ describe('[#22161] one-line verdicts', () => { describe('[#22161] one-line fixes', () => { it('every `fix:` line the cases above fired is one line of at most 200 characters', () => { expect(fired.some((f) => f.message.includes('holds a list'))).toBe(true); - // The three ids this slice converted (the other two RLS ids keep their - // longer hints: a later slice's). - const converted = ['rls-predicate-unenforceable', 'rls-predicate-over-budget', 'rls-predicate-unknown-field']; - for (const f of fired.filter((x) => converted.includes(x.rule))) { + // Every RLS id: slice 11 converted three, slice 12 the other two. + for (const f of fired.filter((x) => x.rule.startsWith('rls-predicate-'))) { expect(f.hint, f.rule).not.toContain('\n'); expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); } diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts index b94b0807d65..012b5bbb53b 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -722,8 +722,11 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not expect(why).toMatch(/unresolved-variable` for it in EVERY position/); expect(why).toMatch(/disappears for every holder of the permission set/); expect(why).not.toMatch(/fail-OPEN/); - expect(f.hint).toMatch(/IRlsMembershipResolver/); - expect(f.hint).toMatch(/never compared with `==`/); + // [#22161] The one-line `fix:` names the `in` route and refuses `==`; how an + // app stages the set is the explanation's. + expect(f.hint).toMatch(/test an app-staged membership set with `in`, never `==`/); + expect(why).toMatch(/IRlsMembershipResolver/); + expect(why).toMatch(/never compared with `==`/); }); it('names the WRITE consequence on a `check` clause, not the read one', () => { @@ -1256,7 +1259,7 @@ describe('[#22161] one-line verdicts — the five ids', () => { // [#22161] Slice 11: the `fix:` line. The three ids this slice converted name // their remedy in one line; what the hints enumerated is each id's `os explain` // entry. (`rls-predicate-unknown-user-variable` and `rls-predicate-unparseable` -// keep their longer hints: a later slice's.) +// are slice 12's, in the block below.) describe('[#22161] one-line fixes — rls-predicate-unenforceable, -over-budget, -unknown-field', () => { const ONE_LINE_FIX_IDS: readonly string[] = [ RLS_PREDICATE_UNENFORCEABLE, @@ -1303,3 +1306,52 @@ describe('[#22161] one-line fixes — rls-predicate-unenforceable, -over-budget, } }); }); + +// [#22161] Slice 12: the `fix:` line of the other two ids. Both name only +// fixed text (and, for the variable, the nearest kernel key), so no author +// text can lengthen them; the bridge's reach, the subquery route, what each +// kernel key holds and how an app stages a membership set are the explanations'. +describe('[#22161] one-line fixes — rls-predicate-unparseable, -unknown-user-variable', () => { + const SLICE_12_IDS: readonly string[] = [RLS_PREDICATE_UNPARSEABLE, RLS_PREDICATE_UNKNOWN_USER_VARIABLE]; + + it('every `fix:` line of the two ids the cases above fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => SLICE_12_IDS.includes(f.rule)); + // Coverage control: both ids fired, the variable id with and without a nearest key. + expect([...new Set(converted.map((f) => f.rule))].sort()).toEqual([...SLICE_12_IDS].sort()); + const variable = converted.filter((f) => f.rule === RLS_PREDICATE_UNKNOWN_USER_VARIABLE).map((f) => f.hint); + expect(variable.some((h) => h.endsWith('never `==`.'))).toBe(true); + expect(variable.some((h) => h.includes('Did you mean'))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('the variable `fix:` line stays inside 200 whatever kernel key it suggests', () => { + // The line holds no author text but the suggestion, so the bound is a + // property of the key set: the longest key is the worst case. + const longest = [...RESERVED_RLS_MEMBERSHIP_KEYS].sort((a, b) => b.length - a.length)[0]!; + const [f] = validateRlsPredicateEnforceability(siteWith('using', `owner_id == current_user.${longest}x`)); + expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_USER_VARIABLE); + expect(f.hint).toContain(`Did you mean "${longest}"?`); + expect(f.hint.length).toBeLessThanOrEqual(200); + }); + + it('`os explain` carries what the `fix:` lines no longer say', () => { + const facts: Record = { + [RLS_PREDICATE_UNPARSEABLE]: [ + 'rather than SQL `AND` / `OR`', + 'A subquery has no CEL spelling at all: RLS cannot join (ADR-0055)', + '(`FIELD in current_user.KEY`, ADR-0105 D11)', + ], + [RLS_PREDICATE_UNKNOWN_USER_VARIABLE]: [ + '`current_user.organization_id` is the tenant', + '`ExecutionContext.rlsMembership`', + 'never as scalars', + ], + }; + for (const rule of SLICE_12_IDS) { + for (const fact of facts[rule]) expect(explanationOf(rule), `${rule} explanation names ${fact}`).toContain(fact); + } + }); +}); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts index a7401dfa2e3..1e997b3fc7c 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -336,10 +336,12 @@ function str(v: unknown): string { * at most 200 characters, and what the hints used to enumerate — the lowerable * subset and its three traps, the rule each holding breaks with the other * rewrites a site admits, the comparison classes with their types, the four - * ways to shrink an oversized predicate, the declared fields of the object — - * is each id's `os explain` entry (`rule-explanations.ts`), held to its source - * by this file's tests. A predicate with several faulty sites gets the first - * site's rewrite and a count of the rest. + * ways to shrink an oversized predicate, the declared fields of the object, + * what each kernel key holds and how an app stages a membership set, the + * legacy SQL bridge's reach and the subquery route — is each id's + * `os explain` entry (`rule-explanations.ts`), held to its source by this + * file's tests. A predicate with several faulty sites gets the first site's + * rewrite and a count of the rest. */ /** Rewrite into the lowerable subset — the shape arm of `rls-predicate-unenforceable`. */ @@ -357,6 +359,14 @@ const OVER_BUDGET_FIX = 'Make the predicate smaller: collapse an `||` chain into `field in [a, b, …]`, test a membership key ' + '`field in current_user.`, or split a top-level `||` across policies (never a top-level `&&`)'; +/** + * `rls-predicate-unparseable`: the SQL-to-CEL translations in one line. What the + * legacy bridge covers, and why a subquery has no CEL spelling, are the explanation's. + */ +const UNPARSEABLE_FIX = + 'Author the predicate in canonical CEL: `&&` / `||` for AND / OR, `!` for NOT, `startsWith` / `endsWith` / ' + + '`contains` for LIKE; pre-resolve a subquery into a membership key'; + /** `rls-predicate-unknown-field`: the verdict's "Did you mean" names the nearest declared column. */ const UNKNOWN_FIELD_FIX = 'Point the predicate at a column the object declares (denormalise a value from another object onto it ' + @@ -1343,15 +1353,12 @@ function referenceFindings( message: `RLS ${clause} reads \`${variablePath}\`, which nothing pre-resolves, in a scalar position no request ` + `can ever fill, ${droppedClause(clause)}`, + // [#22161] One remedy line, naming only the fixed key set and the + // nearest key, so no author text can lengthen it. What each key holds, + // and how an app stages a membership set, are the explanation's. hint: - `Use one of the pre-resolved context values (${listNames(PRERESOLVED_USER_KEYS)})${suggestName( - key, - PRERESOLVED_USER_KEYS, - )} — \`current_user.organization_id\` is the tenant, \`current_user.id\` the acting user, ` + - `\`current_user.email\` their unique address. If "${key}" is meant to be an app-resolved set, it ` + - `must be staged into \`ExecutionContext.rlsMembership\` by an \`IRlsMembershipResolver\` that ` + - `DECLARES the key, and it can then only be tested with \`in\` (\` in ${variablePath}\`), ` + - `never compared with \`==\`: the runtime stages membership sets as arrays and never as scalars.`, + `Read a pre-resolved key (${listNames(PRERESOLVED_USER_KEYS)}), or test an app-staged membership ` + + `set with \`in\`, never \`==\`.${suggestName(key, PRERESOLVED_USER_KEYS)}`, }); } @@ -1555,13 +1562,7 @@ export function validateRlsPredicateEnforceability( message: `RLS ${clause} does not parse as CEL, even after the legacy SQL bridge (${detail}), ` + droppedClause(clause), - hint: - 'Author the predicate in canonical CEL (ADR-0058 D1). The bridge covers only the historic ' + - 'SQL subset — a bare `=` and `IN` — so everything else must already be CEL: combine with ' + - '`&&` / `||` rather than SQL `AND` / `OR`, negate with `!`, and use `startsWith` / ' + - '`endsWith` / `contains` rather than `LIKE`. A subquery has no CEL spelling at all: RLS ' + - 'cannot join (ADR-0055), so pre-resolve the set into a membership key the runtime exposes ' + - '(`field in current_user.`, ADR-0105 D11) or denormalise the value onto this object.', + hint: UNPARSEABLE_FIX, }); continue; } diff --git a/packages/lint/src/validate-rule-schema-formats.test.ts b/packages/lint/src/validate-rule-schema-formats.test.ts index 85c5bf535c5..8ea2cabaaef 100644 --- a/packages/lint/src/validate-rule-schema-formats.test.ts +++ b/packages/lint/src/validate-rule-schema-formats.test.ts @@ -104,10 +104,11 @@ describe('validateRuleSchemaFormats — the dropped keyword goes RED (#5178)', ( expect(explanationOf(VALIDATION_RULE_SCHEMA_UNKNOWN_FORMAT)).toContain('DROPS the keyword'); // The fix, concretely — an author cannot act on "unknown format". expect(f.hint).toContain("Did you mean `format: 'email'`?"); - // …and the full vocabulary, so a name that is merely unfamiliar is - // resolvable without reading ajv-formats' source. expect(f.hint).toContain('date-time'); - expect(f.hint).toContain('uri-reference'); + // …and the full vocabulary, so a name that is merely unfamiliar is + // resolvable without reading ajv-formats' source. [#22161] `os explain` + // carries it; the `fix:` line names the nearest one. + expect(explanationOf(VALIDATION_RULE_SCHEMA_UNKNOWN_FORMAT)).toContain('uri-reference'); }); it('is reachable on a stack the spec fully accepts — a VALUE verdict needs a green safeParse', () => { @@ -448,3 +449,27 @@ describe('[#22161] one-line verdicts — validation-rule-json-schema-unknown-for } }); }); + +// [#22161] Slice 12: the `fix:` line. One remedy line: the nearest registered +// name, the spelling rule and `pattern`; the registered names themselves are +// the id's `os explain` entry, written out there and held to the live set here. +describe('[#22161] one-line fixes — validation-rule-json-schema-unknown-format', () => { + it('every `fix:` line the cases above fired is one line of at most 200 characters', () => { + const converted = (fired as Array<{ rule: string; hint?: string }>).filter( + (f) => f.rule === VALIDATION_RULE_SCHEMA_UNKNOWN_FORMAT, + ); + // Coverage control: both arms fired, with and without a nearest name. + expect(converted.some((f) => f.hint?.startsWith('Did you mean'))).toBe(true); + expect(converted.some((f) => f.hint?.startsWith('Use a registered name'))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint!.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` lists exactly the registered names the rule judges against', () => { + const text = explanationOf(VALIDATION_RULE_SCHEMA_UNKNOWN_FORMAT); + expect(text).toContain(`\`format\` is enforced on every write): ${registeredFormatNames().join(', ')}.`); + expect(text).toContain('express it with `pattern` instead'); + }); +}); diff --git a/packages/lint/src/validate-rule-schema-formats.ts b/packages/lint/src/validate-rule-schema-formats.ts index de884f7522b..d7d4741f3c2 100644 --- a/packages/lint/src/validate-rule-schema-formats.ts +++ b/packages/lint/src/validate-rule-schema-formats.ts @@ -320,13 +320,15 @@ export function validateRuleSchemaFormats(stack: unknown): RuleSchemaFormatFindi message: `\`format: '${use.name}'\` is not a registered format, so ajv drops the keyword and the rule ` + `enforces it on no record: every write is ACCEPTED`, + // [#22161] One remedy line: the nearest registered name, the spelling + // rule, and `pattern` for a constraint no format covers. The registered + // names themselves (the default `ajv-formats` set, written out and held + // to `registeredFormatNames()` by this rule's test) are + // `os explain validation-rule-json-schema-unknown-format`. hint: (suggestion ? `Did you mean \`format: '${suggestion}'\`? ` : '') + - `The registered names are: ${registered.join(', ')} — the default \`ajv-formats\` set, the one ` + - `\`rule-validator.ts\` registers so that a \`format\` is enforced on every write. Names are ` + - `case-sensitive and hyphenated ` + - `(\`date-time\`, not \`datetime\`). If you meant a constraint ajv has no format for, express it ` + - `with \`pattern\` instead — a regex is enforced, an unknown format name is not.`, + 'Use a registered name (case-sensitive and hyphenated: `date-time`, not `datetime`), or express the ' + + 'constraint with `pattern`, which is enforced', }); }