diff --git a/.changeset/22161-lint-slice-11-one-line-fix.md b/.changeset/22161-lint-slice-11-one-line-fix.md new file mode 100644 index 00000000000..529a3187b6d --- /dev/null +++ b/.changeset/22161-lint-slice-11-one-line-fix.md @@ -0,0 +1,30 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): 15 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 15 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 (the lengths here are the `hint` alone; the CLI prints it after `fix: `). Before, the longest of each ran from 128 to 2,280 characters. The ids: + - sharing rules: `sharing-rule-runtime-variable-condition` (2,280 → 195) and `sharing-rule-unlowerable-condition` (968 → 199); + - approvals: `approval-approvers-may-resolve-empty` (its `manager` arm 2,243 → 198, its group arm 440 → 200); + - RLS predicates: `rls-predicate-unenforceable` (1,056 → 191), `rls-predicate-over-budget` (1,044 → 197) and `rls-predicate-unknown-field` (766 → 154); + - filters: `filter-empty-combinator` (764 → 154); + - permission sets: `permission-retired-lifecycle-residue` (681 → 14: `Delete the key`); + - view visibility: `visibility-predicate-over-budget` (651 → 179); + - hook bodies: `hook-api-update-readonly-field` (647 → 182); + - flows: `flow-time-relative-descriptor-invalid` (342 → 176); + - pages: `component-type-unknown` (229 → 158), `react-chart-aggregate-invalid` (363 → 191) and `react-chart-drilldown-invalid` (128 → 181: longer, because it now carries the remedy instead of a pointer); + - fields: `field-no-consumers` (on `os validate` of the showcase app, 624 → 199). +- **Four fix lines pointed at text the verdict no longer carries.** `component-type-unknown` said "Apply the prescription above", `react-chart-drilldown-invalid` and `react-chart-aggregate-invalid` said "the rejection above carries the fix", and `flow-time-relative-descriptor-invalid` said "satisfies each message above", after earlier releases cut those verdicts to one sentence. Each now names the remedy itself: the retired component type's "Delete the …" sentence cut from `RETIRED_PAGE_COMPONENT_TYPES` (for example `Delete the \`element:form\` component and use the object-bound \`object-form\` block instead`), the schema refusal's rename or the layer a misplaced key belongs to, and the descriptor key the verdict names. +- **Enumerations are cut to the first item and a count.** `field-no-consumers` names the first carrier site and counts the rest (`… together with its 16 carrier site(s): translations[0].en.objects.showcase_account.fields.website (and 15 more)`), where it listed every one. `rls-predicate-unenforceable` gives the first faulty site's rewrite and counts the rest; for a bare `current_user` it names the key slot (`current_user.`) instead of listing every kernel key. `rls-predicate-unknown-field` no longer lists every field of the object: the verdict's "Did you mean" already names the nearest one. `permission-retired-lifecycle-residue` carries the retirement's own "Delete the key" sentence, cut from the tombstone's prescription, not the whole prescription. +- **`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: the lowerable predicate subset and its traps, the comparison classes with the declared types each holds, the rule each `current_user` holding breaks, which mechanism replaces a `current_user` read on which sharing model (an RLS policy narrows only `public_read` / `public_read_write`; it grants nothing on `private`), the graded routes that populate `sys_user.manager_id`, the four ways to shrink an oversized RLS predicate (and why a top-level `&&` must never be split), and the declared "match nothing" filter spelling. The entries already existed; they gain paragraphs. +- **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: + - a `flow` write: `flow-time-relative-descriptor-invalid` (422 `issues[].hint`, 342 → 176), `filter-empty-combinator` (422, 647 / 764 → 134 / 154) and `approval-approvers-may-resolve-empty` (2xx `advisories[].hint` and the `[Protocol] authoring advisory` log line, 2,243 / 440 → 198 / 200); + - a `permission` write: `rls-predicate-unenforceable` (422, 912 / 312 → 186 / 121), `rls-predicate-unknown-field` (422, 708 → 154), `rls-predicate-over-budget` (422, once the platform CEL bounds refuse) and `permission-retired-lifecycle-residue` (2xx advisory, 573 / 681 → 14); + - a `view` write: `visibility-predicate-over-budget` (422, 651 → 179). + + Studio's save-advisory toast prints an advisory's `hint` after its message, so the approval and residue 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:** the two sharing-rule ids, `field-no-consumers`, `component-type-unknown`, the two `react-chart-*` ids and `hook-api-update-readonly-field` run on the CLI only. +- **Unchanged:** every `message` (the verdict), rule id, severity and `path`, and what each rule accepts or refuses. diff --git a/.changeset/22161-spec-approver-type-describe.md b/.changeset/22161-spec-approver-type-describe.md new file mode 100644 index 00000000000..8cad97d7446 --- /dev/null +++ b/.changeset/22161-spec-approver-type-describe.md @@ -0,0 +1,10 @@ +--- +"@objectstack/spec": patch +--- + +docs(spec): `ApproverType`'s description points at `os explain approval-approvers-may-resolve-empty` for the graded `manager` routes + +Clause-②: no + +- **What changes:** the `.describe()` text of `ApproverType` (`@objectstack/spec/automation`), which the generated reference page renders for every approver entry (`content/docs/references/automation/approval.mdx`). It said `os lint` "carries the graded population routes and the full remedy". Since `@objectstack/lint` shortened `approval-approvers-may-resolve-empty`'s fix line to one sentence, the finding names two routes in one line, and `os explain approval-approvers-may-resolve-empty` prints the graded routes and the full remedy. The description now says so. Its "the request waits" clause now says the request falls to the node's `onEmptyApprovers` policy; under the default policy that opens the request on the organization's administrators, so it no longer waits. +- **Unchanged:** the `ApproverType` values, the schema's shape, and what it accepts or refuses. Only the description text moves. diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index 1045d84de36..9e7e0773116 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -53,7 +53,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it, a manager step resolves to nobody and the request falls to the node's `onEmptyApprovers` policy. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty`, and `os explain approval-approvers-may-resolve-empty` carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | | **value** | `string` | optional | User id / membership tier / position / team / department / field — per `type`; for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*` | | **resolveAs** | `Enum<'user' \| 'department' \| 'position' \| 'team'>` | optional | How an `expression` result is expanded into approvers (default 'user') | | **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | @@ -83,7 +83,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it, a manager step resolves to nobody and the request falls to the node's `onEmptyApprovers` policy. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty`, and `os explain approval-approvers-may-resolve-empty` carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | | **value** | `string` | optional | User id / membership tier / position / team / department / field — per `type`; for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*` | | **resolveAs** | `Enum<'user' \| 'department' \| 'position' \| 'team'>` | optional | How an `expression` result is expanded into approvers (default 'user') | | **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | @@ -93,7 +93,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | +| **type** | `Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>` | ✅ | Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it, a manager step resolves to nobody and the request falls to the node's `onEmptyApprovers` policy. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty`, and `os explain approval-approvers-may-resolve-empty` carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. | | **value** | `string` | optional | User id / membership tier / position / team / department / field — per `type`; for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*` | | **resolveAs** | `Enum<'user' \| 'department' \| 'position' \| 'team'>` | optional | How an `expression` result is expanded into approvers (default 'user') | | **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | @@ -124,7 +124,7 @@ const result = ApprovalDecision.parse(data); ## ApproverType -Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it through one of the routes `os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. +Approval step approver type. `manager` is resolved from the directory rather than configured here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a profile field an author edits, so until an operator populates it, a manager step resolves to nobody and the request falls to the node's `onEmptyApprovers` policy. `os lint` reports that at authoring time as `approval-approvers-may-resolve-empty`, and `os explain approval-approvers-may-resolve-empty` carries the graded population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the same remedy in prose. ### Allowed Values diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index d0cebdf05c6..f8c8ac51efa 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -70,9 +70,10 @@ const FIELD_NO_CONSUMERS_EXPLANATION: RuleExplanation = { 'drawn), or a dataset or cube member path the analytics door refuses (a hop or column that does ' + 'not resolve, or a join the dataset\'s `include` does not declare).', 'The verdict on the warning is `inert` when no site of any kind names the field, and ' + - '`carrier-only` when only carriers do — then the warning lists each carrier site, because ' + - 'removing the declaration means cleaning them too. Verdicts are per object: a consumer of the ' + - 'same field name on another object does not cover this declaration.', + '`carrier-only` when only carriers do — then the `fix:` line names the first carrier site and counts ' + + 'the rest, because removing the declaration means cleaning them too (each one names the field). ' + + 'Verdicts are per object: a consumer of the same field name on another object does not cover this ' + + 'declaration.', 'Never reported, because the platform reads them without an authored consumer: a system column ' + 'the registry injects (when the object re-declares it), the record\'s title field, and a ' + '`master_detail` field (the relationship is consumed by being declared). Fields an ' + @@ -715,6 +716,8 @@ const VISIBILITY_PREDICATE_OVER_BUDGET_EXPLANATION: RuleExplanation = { 'far fewer AST nodes (`maxListElements` is 64, so a very large set needs the next option), or ' + 'precompute the heavy part into a formula or roll-up field and test that one field. The finding ' + 'does not echo the predicate; its `path` locates it.', + 'Logic genuinely this large is not element visibility: compute it once on the record rather than ' + + 're-deriving it in every predicate that needs it.', ], }; @@ -1021,6 +1024,10 @@ const HOOK_API_UPDATE_READONLY_FIELD_EXPLANATION: RuleExplanation = { 'channel, not the field, so that correct stamp is never touched. `ctx.api.sudo()` is no way out ' + 'from a body: `sudo()` lives on the in-process scoped context and is not marshalled into the ' + 'sandbox, so calling it is a TypeError at run time.', + 'Seeding a readonly column at create time is a SYSTEM act, so the two routes are the remedy whichever ' + + 'verb the body calls: `runAs: \'system\'` on this hook, or the own-hook stamp — on an update the ' + + 'recommended shape, since a value automation maintains belongs on the record\'s own hook. Otherwise drop ' + + 'the field from the payload, or drop `readonly: true` from the field.', 'The rule gates because both halves are declared in this stack: the field\'s `readonly` and the ' + 'body\'s literal `ctx.api` write. `id` in an update payload is the write\'s address, not a field ' + 'write, and is not judged; a body that does not parse is skipped, and its ' + @@ -1122,6 +1129,22 @@ const CROSS_CLASS_COMPARISON = 'formula field has no stored column a row filter can read. So the platform defines no comparison between ' + 'such columns. The classification is the spec\'s (`crossFieldComparisonVerdict`), the one driver-sql applies.'; +/** + * The comparison classes with the declared types each holds — both rules' + * cross-class arm, which used to print this listing on its `fix:` line. + * `validate-rls-predicate-enforceability.test.ts` holds it to + * `CROSS_FIELD_CLASS_LISTING`, which the rule reads off the spec's class table. + */ +const CROSS_CLASS_LISTING = + 'Compare a field only with a field of the same comparison class: a number only with a number (`number`, ' + + '`currency`, `percent`, `rating`, `slider`, `progress`, `summary`); text only with text (`text`, `textarea`, ' + + '`email`, `url`, `phone`, `password`, `secret`, `markdown`, `html`, `richtext`, `code`, `color`, `signature`, ' + + '`qrcode`, `autonumber`, `select`, `radio`, `lookup`, `master_detail`, `user`, `tree`); a boolean only with a ' + + 'boolean (`boolean`, `toggle`); a date only with a date (`date`); a datetime only with a datetime ' + + '(`datetime`); a time of day only with a time of day (`time`). A file field and a formula field cannot be ' + + 'compared with another column at all. If the two columns do hold comparable values, one of them is declared ' + + 'with the wrong type: fix the declaration, not the predicate.'; + const RLS_PREDICATE_UNPARSEABLE_EXPLANATION: RuleExplanation = { rule: 'rls-predicate-unparseable', covers: 'why a predicate that does not parse is dropped', @@ -1146,6 +1169,15 @@ const RLS_PREDICATE_OVER_BUDGET_EXPLANATION: RuleExplanation = { 'error — on purpose, because the runtime\'s only decision is whether to drop the policy. This rule tells ' + 'the two apart because the fix differs: there is no syntax or dialect error to correct, so the predicate ' + 'must get smaller. The finding names the bound and its value; its `path` locates the predicate.', + 'So make it smaller, or move the work off the predicate. (1) Collapse a long `field == a || field == b || …` ' + + 'chain into a single `field in [a, b, …]`, which is far fewer AST nodes (`maxListElements` is 64, so a very ' + + 'large set needs option 2). (2) Pre-resolve the set into a membership key the runtime exposes and test ' + + '`field in current_user.` (ADR-0105 D11) — one comparison whatever the set size. (3) Denormalise a ' + + 'repeated sub-expression onto this object as a formula/rollup field and test that single column. (4) Split ' + + 'a TOP-LEVEL `||` across several `rowLevelSecurity` policies: applicable policies are OR-ed, so that is ' + + 'equivalent — but never split a top-level `&&` this way, which would WIDEN access rather than preserve it. ' + + 'Logic genuinely this large is not a row filter: move it to a hook or action body ' + + '(`ScriptBody { language: \'js\' }`, the L2 sandboxed surface).', RLS_UNCOMPILABLE_DROP, RLS_DROPPED_USING, RLS_DROPPED_CHECK, @@ -1160,6 +1192,15 @@ const RLS_PREDICATE_UNENFORCEABLE_EXPLANATION: RuleExplanation = { 'function call (`size(…)`, `has(…)`), arithmetic, a ternary, a related-record path, a list literal under ' + '`==` / `!=` or the bare `current_user` root used as a value is outside it, and the compiler refuses it. ' + 'The finding quotes the verdict half of the compiler\'s own refusal.', + 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, and the ' + + 'string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column field paths (ADR-0058 D2), ' + + 'compared against a literal or a `current_user.*` value. Three traps in particular: (1) a function call — ' + + '`size(record.tags) > 0`, `has(record.x)` — is correct in an object VALIDATION rule, which is INTERPRETED, ' + + 'and wrong here, where the predicate is COMPILED to a filter; write the null test as `field != null`. ' + + '(2) A related-record path (`record.account.region`) is a join, which the compiler refuses by design ' + + '(ADR-0055) — denormalise the value onto this object (a formula/rollup field) and test that column. ' + + '(3) Arithmetic on a column (`amount * 2 > 100`) never lowers — precompute it into a field, or compare the ' + + 'column against the literal directly.', RLS_UNCOMPILABLE_DROP, // The two key lists are held equal to the rule's per-key probe types by the rule's test. 'A predicate can pass that shape check and still be refused for the TYPE of the value a `current_user` ' + @@ -1173,13 +1214,33 @@ const RLS_PREDICATE_UNENFORCEABLE_EXPLANATION: RuleExplanation = { 'per-request "DENY (fail closed)" WARN, emitted only when nothing else applicable compiles. A refusal that ' + 'depends on WHICH caller asks — a constant comparison such as `current_user.email == \'ops@acme.com\'` — is ' + 'not reported: no single probe can stand for every request.', + 'Each holding breaks one rule. `==` / `!=` compare ONE value and a string method takes ONE string, so a ' + + 'membership set is tested with `in` — "one of these" is `x in set`, "none of these" is `!(x in set)`. `in` ' + + 'tests membership in a SET, so one value is compared with `==` / `!=`. And `current_user` is the caller ' + + 'context, not a value: name the key that holds what you mean. The `fix:` line rewrites the first site the ' + + 'fault lands on and counts the rest; every site admits the same rewrite. A site also admits a second one: a ' + + 'string method can be passed ONE of the one-value keys above instead of a set, and a set is named by one of ' + + 'the membership keys above, or by an app-staged §7.3.1 set (`field in current_user.`).', 'The same per-request drop, with the same signal, follows a lowered comparand the platform\'s shared filter ' + 'check refuses — a `null` list member or a `null` ordering bound, which no two backends agree on: ' + '`RLSCompiler` runs that check on every compiled policy filter before any backend sees it.', + '`null` has no place inside a list or opposite an ordering operator — the platform refuses both in every ' + + 'filter it is sent, because no two backends agree on what they match. Test for no value with `== null` and ' + + 'for a value with `!= null`. A list that keeps other members either keeps matching a missing value ' + + '(`(field in [...] || field == null)`, the `fix:` line\'s rewrite) or drops the `null` member; an ordering ' + + 'bound of `null` becomes `!= null` (has a value) or `== null` (has none), or a real bound.', RLS_DROPPED_USING, RLS_DROPPED_CHECK, LIST_HOLDING_COMPARISON, + 'A row filter compares one value with one value, so it cannot test membership in a list another column ' + + 'holds: compare with a single-valued column, or with a literal or a `current_user` value — "one of these ' + + 'values" is `record.status in [\'open\', \'pending\']` or `record.owner in current_user.org_user_ids` — or ' + + 'move the condition into a validation rule or a hook.', CROSS_CLASS_COMPARISON, + CROSS_CLASS_LISTING, + 'To keep the rule without a second column, compare with a literal or a `current_user` value, test a file ' + + 'field with `!= null`, or store the value you mean in a field of the right type and compare that — or move ' + + 'the condition into a validation rule or a hook.', 'Such a comparison is not dropped; it is refused where it runs. On a `using`, every read the policy scopes ' + 'is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql refuses the comparison by the columns\' ' + 'declared types), and every by-id update or delete it scopes fails closed (`PERMISSION_DENIED` / 403). On ' + @@ -1195,6 +1256,11 @@ const RLS_PREDICATE_UNENFORCEABLE_EXPLANATION: RuleExplanation = { 'quotes the engine\'s verdict with its code and status. The analytics face hands the read scope to this ' + 'same admission before it runs a query, so every analytics query over the object that this policy scopes ' + 'is refused. Each clause earns one finding: the engine judges only a clause every earlier check left clean.', + 'What the engine refuses, it refuses for a reason the `fix:` line names in one line: a text operator ' + + '(`startsWith` / `endsWith` / `contains`) runs only on a field that holds a string, and a date or datetime ' + + 'field only against a value its storage can read. A `{…}` placeholder is saved-filter syntax, and an RLS ' + + 'predicate does not use it: the predicate is CEL, and the caller\'s values are the `current_user` keys ' + + '(accessible_org_ids, email, id, org_user_ids, organization_id, positions).', ], }; @@ -1224,6 +1290,12 @@ const RLS_PREDICATE_UNKNOWN_FIELD_EXPLANATION: RuleExplanation = { 'write path had no field-existence check at all, so a negated miss was satisfied VACUOUSLY by the ' + 'post-image and PERMITTED exactly the writes the policy was written to refuse, on every driver. Fix the ' + 'name rather than relying on either behaviour.', + 'So point the predicate at a column the object really declares — the verdict\'s "Did you mean" names the ' + + 'nearest one — or delete the policy if the narrowing is gone: a policy naming a column that does not exist ' + + 'is not protection. If the field was RENAMED, this policy has been wrong since that rename, so check which ' + + 'runtime it ran on before you judge the blast radius. If the value was meant to live on another object, ' + + 'RLS cannot join to it (ADR-0055) — denormalise it onto the policy\'s object (a formula/rollup field) and ' + + 'test that column instead.', ], }; @@ -1274,9 +1346,23 @@ const SHARING_RULE_UNLOWERABLE_CONDITION_EXPLANATION: RuleExplanation = { 'A `condition` outside the pushdown subset — a function call such as `has(…)` (correct in an object ' + 'validation, which is interpreted, and wrong here, where the condition is compiled), arithmetic, a ' + 'ternary or a related-record path — does not lower. The finding quotes the compiler\'s own refusal.', + 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, and the ' + + 'string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column `record.` paths ' + + '(ADR-0058 D2). Two traps in particular: `has(record.x)` is correct in an object VALIDATION rule, which is ' + + 'INTERPRETED, and wrong here, where the condition is COMPILED — write the null test as ' + + '`record.x != null`; and a related-record path (`record.account.region`) is a join, which the compiler ' + + 'refuses by design (ADR-0055) — denormalise the value onto this object (a formula/rollup field) and test ' + + 'that column, or share the related object instead.', SHARING_RULE_SKIPPED, LIST_HOLDING_COMPARISON, + 'A row filter compares one value with one value, so it cannot test membership in a list another column ' + + 'holds: compare with a single-valued column, or with a literal — "one of these values" is ' + + '`record.status in [\'open\', \'pending\']` — or keep the value the rule keys on in a single-valued ' + + 'field and compare with that.', CROSS_CLASS_COMPARISON, + CROSS_CLASS_LISTING, + 'To keep the rule without a second column, compare with a literal — "one of these values" is ' + + '`record.status in [\'open\', \'pending\']` — or test a file field with `!= null`.', 'Such a comparison lowers, so the rule IS seeded into `sys_sharing_rule`, but every criteria query it runs ' + 'is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql refuses the comparison by the ' + 'columns\' declared types), and `SharingRuleService` reads a refused query as matching no record. No ' + @@ -1296,7 +1382,29 @@ const SHARING_RULE_RUNTIME_VARIABLE_CONDITION_EXPLANATION: RuleExplanation = { 'compiler refuses a `current_user.*` read.', SHARING_RULE_SKIPPED, 'The fix is a different mechanism, not a different spelling, and which one depends on the anchor object\'s ' + - '`sharingModel`; the `fix:` line names both.', + '`sharingModel`, because the security layers are AND-composed (`getReadFilter` = RLS AND ' + + 'controlled-by-parent AND sharing) and an AND term can only REMOVE rows — it can never add one back.', + 'On `public_read` / `public_read_write` the sharing layer imposes nothing on reads (`buildReadFilter` ' + + 'constrains `private` only), so an RLS policy on a permission set (`rowLevelSecurity[].using`, where ' + + '`current_user.*` IS resolved) is the right mechanism there: it NARROWS an open baseline, the direction ' + + 'AND can express.', + 'On `private` — including a CUSTOM object that declares no `sharingModel`, which resolves to `private` ' + + '(ADR-0090 D1) — that same policy CANNOT work: the sharing layer has already withheld the row, and AND-ing ' + + 'a predicate onto it can only withhold more, so the policy lints clean, passes every gate and grants ' + + 'nothing.', + 'The two doors that widen a `private` object for a non-admin principal both key on WHO OWNS the row or on ' + + 'an explicit share row, never on a property of the record: the ADR-0057 D1 depth scopes (`readScope` / ' + + '`writeScope` on the object permission — `own` | `own_and_reports` | `unit` | `unit_and_below` | `org`), ' + + 'which widen the owner-match to `owner_id IN (…)`; and a `sys_record_share` row, which on this path only a ' + + 'criteria sharing rule writes — this rule. So keep the rule for the part of the predicate that is a ' + + 'property of the RECORD (e.g. `record.stage == \'closed_won\'`) and name the audience through ' + + '`sharedWith`, which is what the `fix:` line says.', + 'If the audience is genuinely record-relative ("the owner\'s manager", "this record\'s approver"), no ' + + 'mechanism expresses it today: `sharedWith` recipients resolve tenant-wide, so reaching for `position` ' + + 'shares every matched row with EVERY holder of that position (the skip-level, the manager two teams over) ' + + '— an over-broad grant, not the narrower one you meant. That is a known gap in the platform, not ' + + 'something a different spelling of this rule can close: leave the grant unauthored rather than widening ' + + 'it, and ask for record-relative sharing recipients.', ], }; @@ -1389,6 +1497,9 @@ const FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID_EXPLANATION: RuleExplanation = { 'layer out: `schedule` is a sibling of `timeRelative` on the START node\'s `config`, not a key ' + 'inside it (omitting it means daily at 08:00 UTC), and `runAs` is a FLOW-level key: a sweep has no ' + 'trigger user, so declare `runAs: \'system\'` beside `nodes` and `edges`.', + 'A descriptor that satisfies `TimeRelativeTriggerSchema` binds, because it is the same schema the ' + + 'trigger safeParses at bind time; an unrecognized key names the declared key it was probably meant to ' + + 'be. The descriptor\'s keys are listed in content/docs/references/automation/time-relative-trigger.mdx.', FLOW_GATE_REACH, ], }; @@ -1672,7 +1783,8 @@ const REACT_CHART_AGGREGATE_INVALID_EXPLANATION: RuleExplanation = { 'Every refusal the schema, the published react-blocks type and objectui\'s renderer agree on is an ' + 'error. An absent `groupBy` is a warning, and it is a tolerance, not a supported shape: the ' + 'aggregate returns one ungrouped row and the chart plots a single point; a single number belongs ' + - 'in an object-metric block.', + 'in an object-metric block. It stays a warning rather than an error only because promoting it is its ' + + 'own step.', ], }; @@ -1996,10 +2108,13 @@ const COMPONENT_TYPE_UNKNOWN_EXPLANATION: RuleExplanation = { 'registered string-arm type — and the verdict names the closest declared spellings. A type ' + 'outside those namespaces is untouched. It was an error from birth because the in-repo page ' + 'corpus was measured clean first. A source-authored (`kind: \'react\'`) page is not walked.', + 'A custom component registered by your own plugin belongs in its own namespace (`my-plugin:…`), so it ' + + 'cannot be mistaken for platform vocabulary: a reserved namespace is closed to it.', 'A RETIRED type (`RETIRED_PAGE_COMPONENT_TYPES`) is reported by exact name, before the namespace ' + 'check, because a retirement can take its namespace with it: `user:profile` was the `user:` ' + - 'namespace\'s only member. The verdict quotes the head of the retirement\'s prescription. The ' + - 'whole prescription — what replaces the type, or that nothing does — is the parse door\'s ' + + 'namespace\'s only member. The verdict quotes the head of the retirement\'s prescription and the ' + + '`fix:` line its "delete the …" sentence; the page cannot validate or publish while the node is ' + + 'present. The whole prescription — what replaces the type, or that nothing does — is the parse door\'s ' + 'refusal of the same name, which `os validate` and `os build` print, and the kept ' + '`ComponentPropsMap` row carries it too: one copy, never restated here.', ], @@ -2037,6 +2152,39 @@ const FILTER_PRESET_COMPARAND_EXPLANATION: RuleExplanation = { // ── Approval approvers (`validate-approval-approvers.ts`) ─────────────────── +/** + * How an operator populates `sys_user.manager_id` — the `manager` arm's + * remedy, which its `fix:` line printed whole until it became one line. The + * measurement behind each graded route is the docblock above `MANAGER_ONLY_FIX` + * in `validate-approval-approvers.ts`; this is the one printed copy. + */ +const MANAGER_ONLY_REMEDY = + 'sys_user.manager_id is not a profile column — the data API\'s managed-update whitelist is ' + + '{name, image, locale} and the column is readonly on the user form, so it is never populated by ' + + 'editing the user in the Console. It has a dedicated admin operation instead: a platform admin ' + + 'uses Set Manager (Setup → Users, from the user\'s row menu or record header) or POSTs ' + + '{ userId, managerId } to /api/v1/auth/admin/set-user-manager, with managerId set to null ' + + 'to clear the link.'; + +/** The routes an operator can actually take, GRADED — the paragraph after {@link MANAGER_ONLY_REMEDY}. */ +const MANAGER_ONLY_ROUTES = + 'That endpoint is the route this platform gives you, and it refuses a link that would make a ' + + 'user their own manager, close a cycle, run past the chain depth cap, or point across an ' + + 'organization boundary. For many users at once, the admin bulk import ' + + '(POST /api/v1/auth/admin/import-users) reads a manager_id column holding the manager\'s ' + + 'email address, or phone number where phone sign-in is enabled — someone in the same file or ' + + 'an existing user — and links each row once every row in the file exists, through those same ' + + 'refusals; a link it cannot make is reported on that row\'s manager result and never fails the ' + + 'user it imported. The column is also written by a seed, or by any other system-context ' + + 'write, which bypasses the managed-update whitelist. SCIM provisioning and directory sync can ' + + 'populate it too, but only through a provisioning path your own deployment supplies: this ' + + 'platform declares the SCIM \'manager\' attribute without projecting it onto the column — and ' + + 'where an identity carries source \'idp_provisioned\' the admin operation refuses, the import\'s ' + + 'manager_id column included, leaving that directory the one surface that authors its ' + + 'manager. Or declare onEmptyApprovers: \'fallback\' on the node, with a ' + + 'fallbackApprovers list: an empty manager rung then opens the request on those people instead ' + + 'of on the organization\'s administrators, which needs no write to the column at all.'; + const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY_EXPLANATION: RuleExplanation = { rule: 'approval-approvers-may-resolve-empty', covers: 'why an all-group or all-manager slate can stall', @@ -2051,7 +2199,7 @@ const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY_EXPLANATION: RuleExplanation = { 'decides instead; only an organization with no administrator leaves the request on an empty ' + '`pending_approvers` slate, for a platform administrator\'s override. An approver of any ' + 'other type on the node is a non-group route, so a mixed slate is not judged.', - 'The `manager` arm is the same dead end with a cause the product cannot repair. ' + + 'The `manager` arm reaches the same empty slate with a cause the flow cannot repair. ' + '`{ type: \'manager\' }` resolves from `sys_user.manager_id` of the user the record names ' + '(the field `value` names, else the owner), and returns nobody where that column is unset; a ' + 'static check cannot read that column, so this does not assert the slate IS empty — it reports ' + @@ -2066,6 +2214,15 @@ const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY_EXPLANATION: RuleExplanation = { 'users are wired.', 'Both arms are `info`: staffing and the manager column are runtime data a linter cannot see, so ' + 'the rule flags the SHAPE, and an `info` finding never blocks a build or a publish.', + 'For the group arm: make sure at least one target is always staffed, or add a guaranteed-staffed ' + + 'approver entry, e.g. { type: \'org_membership_level\', value: \'owner\' }, or declare ' + + 'onEmptyApprovers: \'fallback\' with a fallbackApprovers list to name who decides when the ' + + 'groups come back empty.', + MANAGER_ONLY_REMEDY, + MANAGER_ONLY_ROUTES, + 'For either arm, a slate that still names no one opens on the organization\'s administrators under ' + + 'the default `onEmptyApprovers: \'admin_rescue\'` (the first paragraph), and declaring ' + + '`onEmptyApprovers: \'fallback\'` with a `fallbackApprovers` list names who decides instead.', ], }; @@ -2465,6 +2622,13 @@ const FILTER_EMPTY_COMBINATOR_EXPLANATION: RuleExplanation = { 'disjunction never opens a read scope to the whole table); and `$not: {}` matches NO row, ' + 'the opposite of the "no filter" an empty operand looks like. An error: no reading of the ' + 'literal behaves as written, and a literal empty combinator is not an authoring surface.', + '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) — it says so where an empty combinator only implies it. A PROGRAMMATIC producer that ' + + 'loops to zero operands keeps the runtime identity unchanged — this rule judges only what is written in ' + + 'the metadata.', EMPTY_FILTER_SCOPE, ], }; @@ -3159,10 +3323,13 @@ const PERMISSION_RETIRED_LIFECYCLE_RESIDUE_EXPLANATION: RuleExplanation = { 'the key at its residue value gets a clean parse, no signal, and a line that grants nothing. ' + 'This rule is that missing signal, judged on the raw authored stack before the parse, where ' + 'the residue is still attributable to a line somebody wrote.', - 'A warning: the line is harmless, only dead. The fix line is the retirement\'s own ' + - 'prescription, read from the tombstone on `ObjectPermissionSchema`, so the parse-time refusal ' + - 'and this finding never word it differently. Judged at every CLI door and at the runtime ' + - 'publish gate for a `permission` write.', + 'A warning: the line is harmless, only dead. The fix line is the "Delete the key" sentence of the ' + + 'retirement\'s own prescription, read from the tombstone on `ObjectPermissionSchema` and cut, never ' + + 'retyped, so the parse-time refusal and this finding never word it differently. Deleting the key opens ' + + 'nothing: a dispatched `restore` or `purge` stays denied fail-closed by the permission evaluator\'s ' + + 'destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the ' + + 'operation it gates; `os migrate meta` lists the mechanical edits for existing sources. Judged at every ' + + 'CLI door and at the runtime publish gate for a `permission` write.', ], }; diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 27dc9ddbfeb..3739cb3a5d3 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -23,7 +23,7 @@ import { explainRule } from './rule-explanations.js'; // verdict of those ids to one line of at most 200 characters — so the pin // covers every firing variant this suite exercises, not a chosen few. Run the // whole file: 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 validateApprovalApprovers: typeof validateApprovalApproversUnrecorded = (...args) => { const findings = validateApprovalApproversUnrecorded(...args); fired.push(...findings); @@ -274,56 +274,62 @@ describe('unset-manager dead-end (#16748)', () => { expect(findings[0].message).toContain('locked'); // lockRecord defaults true }); + // [#22161] The graded remedy below was the `fix:` line's until it became one + // line; it is `os explain approval-approvers-may-resolve-empty`'s now, so + // these legs read the explanation, and the one-line `fix:` gets its own pins + // right after them. + const managerRemedy = (): string => explainRule(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY)!.paragraphs.join('\n'); + it('names the REAL remedy — the admin operation, not the Console form', () => { - const [finding] = validateApprovalApprovers(managerOnly()); + const remedy = managerRemedy(); // The prescription an operator can actually carry out. #16678 landed the // write surface, so the first thing the hint owes the reader is the // endpoint, its body and how to CLEAR the link. - expect(finding.hint).toContain('/api/v1/auth/admin/set-user-manager'); - expect(finding.hint).toContain('{ userId, managerId }'); - expect(finding.hint).toContain('managerId set to null'); + expect(remedy).toContain('/api/v1/auth/admin/set-user-manager'); + expect(remedy).toContain('{ userId, managerId }'); + expect(remedy).toContain('managerId set to null'); // The in-product door onto the same endpoint (the `set_user_manager` action // on `sys_user`, label "Set Manager") — an admin reading this in a terminal // is looking for the button, not the HTTP call. - expect(finding.hint).toContain('Set Manager'); - expect(finding.hint).toContain('SCIM'); - expect(finding.hint).toContain('import'); - expect(finding.hint).toContain('directory sync'); + expect(remedy).toContain('Set Manager'); + expect(remedy).toContain('SCIM'); + expect(remedy).toContain('import'); + expect(remedy).toContain('directory sync'); // ⛔ The assertion that went stale the day the endpoint landed. It must be // gone from the string, not merely contradicted further down it: an author // who reads "no product write surface" stops looking for the endpoint. - expect(finding.hint).not.toContain('no product write surface'); + expect(remedy).not.toContain('no product write surface'); // ⛔ And it still must not send them to a surface that cannot write it — // the column stays OUT of ADR-0092 Tier 1 and readonly on the form, so a // dedicated operation is not the same thing as an editable profile column. // The word "Console" appears only inside that denial, never as an // instruction. - expect(finding.hint).toContain('never populated by editing the user in the Console'); - expect(finding.hint).not.toMatch(/[Ee]dit .{0,40}in the Console\b(?!.*NOT)/); + expect(remedy).toContain('never populated by editing the user in the Console'); + expect(remedy).not.toMatch(/[Ee]dit .{0,40}in the Console\b(?!.*NOT)/); // It still offers the escape that needs no operator action at all. - expect(finding.hint).toContain("org_membership_level', value: 'owner'"); + expect(remedy).toContain("org_membership_level', value: 'owner'"); }); it('GRADES the routes — an exact diagnosis whose remedy cannot be carried out is worse than none', () => { // A remedy that names a route with no writer is the #17037 shape. The four // routes are measured against this tree, so the hint must SEPARATE the ones // that work here from the ones that need the deployment's own provisioning. - const [finding] = validateApprovalApprovers(managerOnly()); + const remedy = managerRemedy(); // The route added by #16678, and the refusals that make it safe to point an // operator at: naming an endpoint without naming what it declines is the // same "prescription that cannot be carried out" defect one layer in. - expect(finding.hint).toContain('their own manager'); - expect(finding.hint).toContain('close a cycle'); - expect(finding.hint).toContain('organization boundary'); + expect(remedy).toContain('their own manager'); + expect(remedy).toContain('close a cycle'); + expect(remedy).toContain('organization boundary'); // The pre-existing route with a demonstrated writer: a system-context write // bypasses the managed-update whitelist (`isUserContextWrite` is // `userId && !isSystem`). Kept — the endpoint did not replace it. - expect(finding.hint).toContain('written by a seed, or by any other system-context write'); - expect(finding.hint).toContain('bypasses the managed-update whitelist'); + expect(remedy).toContain('written by a seed, or by any other system-context write'); + expect(remedy).toContain('bypasses the managed-update whitelist'); // The bulk route also WORKS HERE, so it is graded beside the endpoint and // ⛔ never among the routes the deployment must supply: the admin import @@ -333,22 +339,22 @@ describe('unset-manager dead-end (#16748)', () => { // import does not write the column — away from the one supported way to // link a whole file. It must be gone from the string, not merely // contradicted further down it. - expect(finding.hint).toContain('/api/v1/auth/admin/import-users'); - expect(finding.hint).toContain('reads a manager_id column'); - expect(finding.hint).toContain('once every row in the file exists'); - expect(finding.hint).toContain("reported on that row's manager result"); - expect(finding.hint).not.toMatch(/does not write it/); + expect(remedy).toContain('/api/v1/auth/admin/import-users'); + expect(remedy).toContain('reads a manager_id column'); + expect(remedy).toContain('once every row in the file exists'); + expect(remedy).toContain("reported on that row's manager result"); + expect(remedy).not.toMatch(/does not write it/); // Placement is the grade: ahead of the clause that hands the remaining // routes to the deployment. - const importAt = finding.hint.indexOf('/api/v1/auth/admin/import-users'); - const deploymentAt = finding.hint.indexOf('a provisioning path your own deployment supplies'); + const importAt = remedy.indexOf('/api/v1/auth/admin/import-users'); + const deploymentAt = remedy.indexOf('a provisioning path your own deployment supplies'); expect(importAt).toBeGreaterThan(-1); expect(deploymentAt).toBeGreaterThan(importAt); // ⛔ The one that is NOT this repo's to offer must be marked as the // deployment's own, and the hint must say WHY rather than merely hedging. - expect(finding.hint).toContain('a provisioning path your own deployment supplies'); - expect(finding.hint).toContain("declares the SCIM 'manager' attribute without projecting it"); + expect(remedy).toContain('a provisioning path your own deployment supplies'); + expect(remedy).toContain("declares the SCIM 'manager' attribute without projecting it"); // ⛔ And they must not be deleted: a deployment running a real directory // sync may well populate the column, and the defect was presenting all @@ -356,9 +362,19 @@ describe('unset-manager dead-end (#16748)', () => { // PRECEDENCE — the endpoint refuses an `idp_provisioned` identity, and so // does the import's `manager_id` column, which runs the same derivation — // so the hint must not read as "use the endpoint instead of your directory". - expect(finding.hint).toContain('SCIM provisioning and directory sync can populate it'); - expect(finding.hint).toContain("source 'idp_provisioned' the admin operation refuses"); - expect(finding.hint).toContain("the import's manager_id column included"); + expect(remedy).toContain('SCIM provisioning and directory sync can populate it'); + expect(remedy).toContain("source 'idp_provisioned' the admin operation refuses"); + expect(remedy).toContain("the import's manager_id column included"); + }); + + it('the one-line `fix:` names the first route and the escape, and never the Console form', () => { + const [finding] = validateApprovalApprovers(managerOnly()); + expect(finding.hint).toBe( + "Set every submitter's sys_user.manager_id (Setup → Users → Set Manager, or the admin user import), or add " + + "an approver that cannot resolve empty, e.g. { type: 'org_membership_level', value: 'owner' }", + ); + expect(finding.hint).not.toContain('no product write surface'); + expect(finding.hint).not.toMatch(/[Ee]dit .{0,40}in the Console\b/); }); it('does not claim a runtime fact it did not read', () => { @@ -372,15 +388,15 @@ describe('unset-manager dead-end (#16748)', () => { }); it("names the node-level escape — onEmptyApprovers: 'fallback' with fallbackApprovers", () => { - const [finding] = validateApprovalApprovers(managerOnly()); - expect(finding.hint).toContain("onEmptyApprovers: 'fallback'"); - expect(finding.hint).toContain('fallbackApprovers'); + const remedy = managerRemedy(); + expect(remedy).toContain("onEmptyApprovers: 'fallback'"); + expect(remedy).toContain('fallbackApprovers'); // It is offered as a route that needs NO write to the column — the point // of naming it beside an endpoint the operator may not be able to reach. - expect(finding.hint).toContain('needs no write to the column'); + expect(remedy).toContain('needs no write to the column'); // ⛔ And the pre-existing routes are not displaced by it. - expect(finding.hint).toContain('/api/v1/auth/admin/set-user-manager'); - expect(finding.hint).toContain("org_membership_level', value: 'owner'"); + expect(remedy).toContain('/api/v1/auth/admin/set-user-manager'); + expect(remedy).toContain("org_membership_level', value: 'owner'"); }); // ⛔ The load-bearing negative: declaring the policy must NOT silence the @@ -784,3 +800,59 @@ describe('[#22161] one-line verdicts — approval-approvers-may-resolve-empty, a } }); }); + +// [#22161] Slice 11: the `fix:` line. Both empty-slate arms name their remedy +// in one line; the graded routes and the admin-override recovery are the +// explanation's. (The other `approval-*` hints over 200 are a later slice's.) +describe('[#22161] one-line fixes — approval-approvers-may-resolve-empty', () => { + it('every `fix:` line of the id the cases above fired is one line of at most 200 characters', () => { + const emptySlate = fired.filter((f) => f.rule === APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY); + // Coverage control: both arms fired, so the bound cannot pass over one. + expect(emptySlate.some((f) => f.message.includes('routes to a group'))).toBe(true); + expect(emptySlate.some((f) => f.message.includes("{ type: 'manager' }"))).toBe(true); + for (const f of emptySlate) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('the group arm names its three routes in one line', () => { + const [group] = validateApprovalApprovers(stackWithApprovers([{ type: 'position', value: 'exec' }])); + expect(group.hint).toBe( + "Staff at least one target, add an approver that cannot resolve empty (e.g. { type: 'org_membership_level', " + + "value: 'owner' }), or name who decides in fallbackApprovers with onEmptyApprovers: 'fallback'", + ); + }); + + it('`os explain` carries what the `fix:` lines no longer say', () => { + const text = explainRule(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY)!.paragraphs.join('\n'); + for (const fact of [ + 'make sure at least one target is always staffed', + // #22824: a slate that names no one opens on the organization's + // administrators; `fallback` names who decides instead. + "a slate that still names no one opens on the organization's administrators", + 'names who decides instead', + "instead of on the organization's administrators, which needs no write to the column at all", + '/api/v1/auth/admin/set-user-manager', + '/api/v1/auth/admin/import-users', + 'needs no write to the column at all', + ]) expect(text, `explanation names ${fact}`).toContain(fact); + }); +}); + +// [#22161] Slice 11, after #22824 landed beside it: no sentence the rule prints +// or explains may still describe the empty-slate outcome #22824 retired (the +// request waiting on a slate nobody can act on, decided only by an override +// for an organization that HAS administrators). +describe('[#22161] one-line fixes — approval-approvers-may-resolve-empty, against #22824', () => { + it('neither fix line nor the explanation describes the retired empty-slate outcome', () => { + const [group] = validateApprovalApprovers(stackWithApprovers([{ type: 'position', value: 'exec' }])); + const [manager] = validateApprovalApprovers(stackWithApprovers([{ type: 'manager' }])); + const text = explainRule(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY)!.paragraphs.join('\n'); + for (const printed of [group.hint, manager.hint, text]) { + expect(printed).not.toMatch(/waits forever|recoverable only|nobody can act on|no in-product recovery/); + } + // The override survives only as the no-administrator case. + expect(text).toContain("only an organization with no administrator leaves the request on an empty"); + }); +}); diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index cb2ff830d5b..fe077083510 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -87,7 +87,7 @@ const RESERVED_OUTPUT_KEYS = new Set(['decision', 'requestId']); * group-gated. * * ⛔ `manager` stays OUT of this set and is judged by its own arm below - * ({@link MANAGER_ONLY_REMEDY}), for two reasons that are not interchangeable: + * ({@link MANAGER_ONLY_FIX}), for two reasons that are not interchangeable: * the group message ("routes to a group (position/team/department)") would be * false about it, and the REMEDY differs — an unstaffed position is staffed * in-product, an unset `sys_user.manager_id` is not (#16748). Folding `manager` @@ -108,6 +108,13 @@ const STAYS_LOCKED = ', and (lockRecord) the record stays locked'; /** * How an operator actually populates `sys_user.manager_id` (#16748 / #16678). * + * [#22161] The graded routes below are printed by + * `os explain approval-approvers-may-resolve-empty`: their text is the + * `MANAGER_ONLY_REMEDY` and `MANAGER_ONLY_ROUTES` paragraphs of that id's entry + * in `rule-explanations.ts`, the one copy, and this block stays the + * measurement behind each grade. The `manager` arm's `fix:` line + * ({@link MANAGER_ONLY_FIX}) names the first route and the escape in one line. + * * ⚠️ STILL NOT "edit the user in the Console". The column is `readonly: true` * and stays out of ADR-0092's Tier 1, so `getManagedUpdateWhitelist('sys_user')` * is still exactly `{name, image, locale}` and the standard edit form still @@ -191,42 +198,34 @@ const STAYS_LOCKED = ', and (lockRecord) the record stays locked'; * generated `content/docs/references/automation/approval.mdx`, once per shape * that reuses the approver entry — so a stale copy there multiplies) and the * `manager` callout in `content/docs/automation/approvals.mdx`. Neither - * RESTATES the remedy — both point back here, which is why there is still - * exactly one copy to edit. + * RESTATES the remedy — both point back at this rule, which is why there is + * still exactly one copy to edit: since #22161 that copy is the explanation's + * two paragraphs, which the finding reaches through its `rule:` line's + * `os explain` pointer rather than on its `fix:` line. + */ +// ⛔ The tracker ids stay in the comments above and never in the strings below +// or in the explanation: `check:doc-authoring` Rule 3 — a runtime string reaches +// authors, operators and generated surfaces, none of whom can resolve `#NNNN`. +// The reader who can resolve it is reading this source. +/** + * [#22161] The `manager` arm's `fix:` line: the first route the docblock above + * grades (the admin operation, or its bulk import) and the escape that needs + * no write to the column, in one line. Every route, graded, is the + * explanation's (see above). */ -// ⛔ The tracker ids stay in the comments above and never in this string: -// `check:doc-authoring` Rule 3 — a runtime string reaches authors, operators and -// generated surfaces, none of whom can resolve `#NNNN`. The reader who can -// resolve it is reading this source. -const MANAGER_ONLY_REMEDY = - `sys_user.manager_id is not a profile column — the data API's managed-update whitelist is ` + - `{name, image, locale} and the column is readonly on the user form, so it is never populated by ` + - `editing the user in the Console. It has a dedicated admin operation instead: a platform admin ` + - `uses Set Manager (Setup → Users, from the user's row menu or record header) or POSTs ` + - `{ userId, managerId } to /api/v1/auth/admin/set-user-manager, with managerId set to null ` + - `to clear the link.`; +const MANAGER_ONLY_FIX = + "Set every submitter's sys_user.manager_id (Setup → Users → Set Manager, or the admin user import), or add " + + "an approver that cannot resolve empty, e.g. { type: 'org_membership_level', value: 'owner' }"; /** - * The routes an operator can actually take, GRADED — the measurement behind - * each grade is in {@link MANAGER_ONLY_REMEDY}'s docblock. + * [#22161] The group arm's `fix:` line: its three routes in one line, the + * third in #22824's words — `fallback` NAMES who decides, rather than leave a + * slate that names no one to the organization's administrators (the verdict's + * own outcome). What happens when even that names no one is the explanation's. */ -const MANAGER_ONLY_ROUTES = - `That endpoint is the route this platform gives you, and it refuses a link that would make a ` + - `user their own manager, close a cycle, run past the chain depth cap, or point across an ` + - `organization boundary. For many users at once, the admin bulk import ` + - `(POST /api/v1/auth/admin/import-users) reads a manager_id column holding the manager's ` + - `email address, or phone number where phone sign-in is enabled — someone in the same file or ` + - `an existing user — and links each row once every row in the file exists, through those same ` + - `refusals; a link it cannot make is reported on that row's manager result and never fails the ` + - `user it imported. The column is also written by a seed, or by any other system-context ` + - `write, which bypasses the managed-update whitelist. SCIM provisioning and directory sync can ` + - `populate it too, but only through a provisioning path your own deployment supplies: this ` + - `platform declares the SCIM 'manager' attribute without projecting it onto the column — and ` + - `where an identity carries source 'idp_provisioned' the admin operation refuses, the import's ` + - `manager_id column included, leaving that directory the one surface that authors its ` + - `manager. Or declare onEmptyApprovers: 'fallback' on the node, with a ` + - `fallbackApprovers list: an empty manager rung then opens the request on those people instead ` + - `of on a slot nobody can act on, which needs no write to the column at all.`; +const GROUP_ONLY_FIX = + "Staff at least one target, add an approver that cannot resolve empty (e.g. { type: 'org_membership_level', " + + "value: 'owner' }), or name who decides in fallbackApprovers with onEmptyApprovers: 'fallback'"; export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; @@ -545,18 +544,12 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin where, path: `${nodePath}.config.approvers`, // [#22161] One verdict sentence; why staffing is unreadable here and - // what recovers a stuck request is `os explain` text. + // who decides an empty slate is `os explain` text. message: `every approver on this node routes to a group (position/team/department), so if ` + `none is staffed the request opens on the organization's administrators` + (locks ? STAYS_LOCKED : ''), - hint: - `Make sure at least one target is always staffed, or add a guaranteed-staffed ` + - `approver entry, e.g. { type: 'org_membership_level', value: 'owner' }, or declare ` + - `onEmptyApprovers: 'fallback' with a fallbackApprovers list to name who decides ` + - `when the groups come back empty. A request that still lands empty opens on the ` + - `organization's administrators; only an organization with no administrator leaves it ` + - `to a platform administrator's override.`, + hint: GROUP_ONLY_FIX, }); } @@ -568,7 +561,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin // the whole slate is manager rungs that is the same empty-slate dead-end // the #3424 arm above flags for positions — with a strictly WORSE cause. // An unstaffed position is an operator's to fix in-product; an unset - // `manager_id` is not (see {@link MANAGER_ONLY_REMEDY}). So the rule that + // `manager_id` is not (see {@link MANAGER_ONLY_FIX}). So the rule that // exists for the recoverable rung was, until this arm, silent on the // unrecoverable one — and `manager` is the canonical first rung of a // tiered ladder, so the silent case was also the common one. @@ -602,12 +595,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin `every approver on this node is { type: 'manager' }, so where sys_user.manager_id ` + `is unset the request opens on the organization's administrators` + (locks ? STAYS_LOCKED : ''), - hint: - `${MANAGER_ONLY_REMEDY} ${MANAGER_ONLY_ROUTES} Populate it for everyone who submits ` + - `this request, or take the other escape that needs none of that: add a second approver ` + - `entry which cannot resolve empty, e.g. { type: 'org_membership_level', value: 'owner' }. ` + - `A request that still lands empty opens on the organization's administrators; to name who ` + - `decides instead, declare onEmptyApprovers: 'fallback' with a fallbackApprovers list.`, + hint: MANAGER_ONLY_FIX, }); } diff --git a/packages/lint/src/validate-component-types.test.ts b/packages/lint/src/validate-component-types.test.ts index aa5ae4163a8..999b0c83c54 100644 --- a/packages/lint/src/validate-component-types.test.ts +++ b/packages/lint/src/validate-component-types.test.ts @@ -15,6 +15,7 @@ import { import { validateComponentTypes as validateComponentTypesUnrecorded, retiredTypeHead, + retiredTypeRemedy, COMPONENT_TYPE_UNKNOWN, } from './validate-component-types.js'; import { explainRule } from './rule-explanations.js'; @@ -25,7 +26,7 @@ import { explainRule } from './rule-explanations.js'; // 200 characters — so the pin covers every firing variant this suite exercises // (every retired type included), 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 validateComponentTypes: typeof validateComponentTypesUnrecorded = (...args) => { const findings = validateComponentTypesUnrecorded(...args); fired.push(...findings); @@ -194,7 +195,12 @@ describe('reports an EXACT retired component type, relaying the spec prescriptio expect(f.message).toBe(retiredVerdict(type)); expect(prescription.startsWith(retiredTypeHead(prescription))).toBe(true); expect(retiredTypeHead(prescription)).toContain(`\`${type}\``); - expect(f.hint).toContain(type); + // [#22161] The `fix:` line is the remedy itself, cut from the same entry — + // never "the prescription above", which the verdict no longer quotes. + expect(f.hint).toBe(retiredTypeRemedy(prescription)); + expect(f.hint).toMatch(/^Delete the /); + expect(prescription.toLowerCase()).toContain(f.hint.toLowerCase()); + expect(f.hint).not.toMatch(/\babove\b/); }); /** @@ -358,3 +364,49 @@ describe('[#22161] one-line verdicts — component-type-unknown', () => { } }); }); + +// [#22161] Slice 11: the `fix:` line. The retired arm's hint said "Apply the +// prescription above" after the verdict had been cut to the prescription's +// head; it now carries the remedy, cut (never rewritten) from the same entry. +describe('[#22161] one-line fixes — component-type-unknown', () => { + it('every `fix:` line the cases above fired is one line of at most 200 characters', () => { + // Coverage control: the retired arm fired for every retired type, and the + // own-namespace arm fired, so the bound cannot pass over an arm. + for (const type of RETIRED_PAGE_COMPONENT_TYPES.keys()) { + expect(fired.some((f) => f.hint === retiredTypeRemedy(RETIRED_PAGE_COMPONENT_TYPES.get(type)!)), type).toBe(true); + } + expect(fired.some((f) => f.hint.includes('give it its own namespace'))).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('each retired type\'s remedy, as the map words it', () => { + expect(retiredTypeRemedy(RETIRED_PAGE_COMPONENT_TYPES.get('user:profile')!)).toBe( + 'Delete the component node and let the shell render the profile', + ); + expect(retiredTypeRemedy(RETIRED_PAGE_COMPONENT_TYPES.get('element:form')!)).toBe( + 'Delete the `element:form` component and use the object-bound `object-form` block instead', + ); + }); + + it('retiredTypeRemedy cuts the "delete the" sentence, at its dash only past 200, and never rewrites', () => { + expect(retiredTypeRemedy('`a:b` is gone — x. Delete the `a:b` node — use `a:c`. Run it.')) + .toBe('Delete the `a:b` node — use `a:c`'); + expect(retiredTypeRemedy(`\`a:b\` is gone: delete the node and ${'x'.repeat(200)} — then y.`)) + .toBe(`Delete the node and ${'x'.repeat(200)}`); + expect(retiredTypeRemedy('`a:b` is gone.')).toMatch(/^Delete the component node/); + }); + + it('the own-namespace arm reads as one line and keeps the example name', () => { + const [f] = validateComponentTypes(page([{ type: 'record:zzzz_qqqq_wwww' }])); + expect(f.hint).toBe( + 'Use a declared component type, or — for a custom component registered by your own plugin — give it ' + + 'its own namespace (e.g. `my-plugin:zzzz_qqqq_wwww`)', + ); + expect(explainRule(COMPONENT_TYPE_UNKNOWN)!.paragraphs.join('\n')).toContain( + 'cannot be mistaken for platform vocabulary', + ); + }); +}); diff --git a/packages/lint/src/validate-component-types.ts b/packages/lint/src/validate-component-types.ts index 36c2e25147c..f6e002a49cc 100644 --- a/packages/lint/src/validate-component-types.ts +++ b/packages/lint/src/validate-component-types.ts @@ -140,6 +140,32 @@ export function retiredTypeHead(prescription: string): string { return stop > 0 ? prescription.slice(0, stop) : prescription.replace(/\.$/, ''); } +/** + * [#22161] A retired type's REMEDY, cut from its prescription for the `fix:` + * line: the sentence that says what to do — every entry has one that opens + * "delete the …" (`Delete the \`element:form\` component and use the + * object-bound \`object-form\` block instead`) — up to its end, and up to its + * first ` — ` as well when the whole sentence would run past 200 characters. + * Like {@link retiredTypeHead} it cuts and never rewrites (the first letter is + * capitalised, nothing else), so the `fix:` line is a piece of + * `RETIRED_PAGE_COMPONENT_TYPES`' own text and no second copy of the guidance + * exists. It replaces a hint that said "Apply the prescription above" after + * the verdict had been cut to the prescription's head. An entry with no such + * sentence gets the one remedy every retirement shares. + */ +export function retiredTypeRemedy(prescription: string): string { + const at = prescription.search(/\bdelete the /i); + if (at < 0) return 'Delete the component node: the type is retired and the parse refuses it by name'; + let remedy = prescription.slice(at); + const stop = remedy.indexOf('. '); + remedy = stop >= 0 ? remedy.slice(0, stop) : remedy.replace(/\.$/, ''); + if (remedy.length > 200) { + const dash = remedy.indexOf(' — '); + if (dash > 0) remedy = remedy.slice(0, dash); + } + return remedy.charAt(0).toUpperCase() + remedy.slice(1); +} + export function validateComponentTypes(stack: AnyRec): ComponentTypeFinding[] { const findings: ComponentTypeFinding[] = []; if (!isRec(stack)) return findings; @@ -165,10 +191,9 @@ export function validateComponentTypes(stack: AnyRec): ComponentTypeFinding[] { where: `page "${pageName}" · ${type}`, path: `${path}.type`, message: `${retiredTypeHead(prescription)}, so the parse refuses this node by name (\`os validate\` prints the prescription)`, - hint: - `Apply the prescription above: \`${type}\` is a retired component type, refused by ` + - 'name at the parse door (`PageComponentSchema.type`), so this page cannot validate or ' + - 'publish while the node is present.', + // [#22161] The remedy itself, cut from the same entry ("above" pointed + // at the prescription the verdict no longer quotes). + hint: retiredTypeRemedy(prescription), }); continue; } @@ -189,11 +214,12 @@ export function validateComponentTypes(stack: AnyRec): ComponentTypeFinding[] { `\`${type}\` is not a declared type of the reserved \`${type.slice(0, type.indexOf(':'))}:\` ` + 'namespace, so the page draws a placeholder in its place.' + (suggestion ? ` ${suggestion}` : ''), + // [#22161] One line; why a custom component needs its own namespace is + // the explanation's. hint: suggestions.length ? `Rename \`${type}\` → \`${suggestions[0]}\`.` - : `Use a declared component type from the standard vocabulary, or — for a custom component ` + - `registered by your own plugin — give it its own namespace (e.g. \`my-plugin:${type.slice(type.indexOf(':') + 1)}\`) ` + - 'so it cannot be mistaken for platform vocabulary.', + : `Use a declared component type, or — for a custom component registered by your own plugin — ` + + `give it its own namespace (e.g. \`my-plugin:${type.slice(type.indexOf(':') + 1)}\`)`, }); } } diff --git a/packages/lint/src/validate-empty-combinators.test.ts b/packages/lint/src/validate-empty-combinators.test.ts index 0a64fc159f2..b2b80241347 100644 --- a/packages/lint/src/validate-empty-combinators.test.ts +++ b/packages/lint/src/validate-empty-combinators.test.ts @@ -36,7 +36,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 validateEmptyCombinators: typeof validateEmptyCombinatorsUnrecorded = (...args) => { const findings = validateEmptyCombinatorsUnrecorded(...args); fired.push(...findings); @@ -71,7 +71,10 @@ describe('validateEmptyCombinators — the literal shapes are refused', () => { expect(findings[0].path).toBe(`${WIDGET_PATH}.$and`); // Per-shape: the AND identity matches everything. expect(findings[0].message).toContain('matches EVERY row'); - expect(findings[0].hint).toContain('DELETE the key'); + // [#22161] One remedy line; why the absent key is the spelling is the + // explanation's. + expect(findings[0].hint).toContain('delete the key'); + expect(explainRule(FILTER_EMPTY_COMBINATOR)!.paragraphs.join('\n')).toContain('DELETE the key'); }); it('refuses `$or: []` and says it is the OPPOSITE of "no filter"', () => { @@ -84,7 +87,9 @@ describe('validateEmptyCombinators — the literal shapes are refused', () => { // The half a generic message gets wrong: OR identity is match-NONE. expect(findings[0].message).toContain('matches NO row'); expect(findings[0].message).not.toContain('matches EVERY row'); - expect(findings[0].hint).toContain('OPPOSITE'); + // [#22161] The verdict says it is the opposite; the one-line `fix:` says + // what emptying the array does instead of relaxing it. + expect(findings[0].hint).toContain('closes the filter instead of relaxing it'); }); it('refuses `$not: {}` and prescribes putting the negated condition inside', () => { @@ -438,3 +443,27 @@ describe('[#22161] one-line verdicts — filter-empty-combinator, filter-empty-n for (const key of FILTER_KEYS) expect(scope, key).toContain(`\`${key}\``); }); }); + +// [#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.) +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); + // Coverage control: all three shapes fired. + for (const shape of ['$and', '$or', '$not']) { + expect(converted.some((f) => f.message.startsWith(`\`${shape}`)), shape).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_COMBINATOR)!.paragraphs.join('\n'); + expect(text).toContain('`{ : { $in: [] } }` is the declared spelling'); + expect(text).toContain('A PROGRAMMATIC producer that loops to zero operands keeps the runtime identity unchanged'); + }); +}); diff --git a/packages/lint/src/validate-empty-combinators.ts b/packages/lint/src/validate-empty-combinators.ts index 53f5f4d0f1b..ab186319482 100644 --- a/packages/lint/src/validate-empty-combinators.ts +++ b/packages/lint/src/validate-empty-combinators.ts @@ -238,15 +238,17 @@ function emitEmptyCombinator(key: '$and' | '$or' | '$not', path: string, ctx: Ct : '`$not: {}` negates an EMPTY node, which is TRUE, so it ' + `${rows(VERDICT_OF.$not)}: the opposite of the "no filter" an empty operand looks like`; + // [#22161] One remedy line per shape. The declared match-none spelling and + // what a programmatic producer keeps are `os explain filter-empty-combinator`. const hint = key === '$and' - ? `${OMIT_THE_KEY} To express a constraint, put the conditions in the array. ${MATCH_NONE_SPELLING}` + ? 'To express "no filter", delete the key (an absent `filter` runs identically); to express a ' + + 'constraint, put the conditions in the array' : key === '$or' - ? 'If you meant "no filter", this is its OPPOSITE: emptying the array does not relax the filter, ' + - `it closes it. ${OMIT_THE_KEY} If you meant to offer alternatives, put the branches in the ` + - `array. ${MATCH_NONE_SPELLING}` - : 'Put the condition you are negating inside `$not` (`{ $not: { status: \'closed\' } }`). ' + - `${OMIT_THE_KEY} ${MATCH_NONE_SPELLING}`; + ? 'Emptying the array closes the filter instead of relaxing it: delete the key to express "no filter", ' + + 'or put the branches in the array to offer alternatives' + : 'Put the condition you are negating inside `$not` (`{ $not: { status: \'closed\' } }`), or delete ' + + 'the key to express "no filter"'; ctx.out.push({ severity: 'error', @@ -254,9 +256,7 @@ function emitEmptyCombinator(key: '$and' | '$or' | '$not', path: string, ctx: Ct where: ctx.where, path, message, - hint: - `${hint} A PROGRAMMATIC producer that loops to zero operands keeps the runtime identity ` + - 'unchanged — this rule judges only what is written in the metadata.', + hint, }); } diff --git a/packages/lint/src/validate-field-consumers.test.ts b/packages/lint/src/validate-field-consumers.test.ts index a5436cc4e16..f5a65726615 100644 --- a/packages/lint/src/validate-field-consumers.test.ts +++ b/packages/lint/src/validate-field-consumers.test.ts @@ -17,7 +17,7 @@ import { explainRule } from './rule-explanations.js'; // in this file hold each recorded 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 validateFieldConsumers: typeof validateFieldConsumersUnrecorded = (...args) => { const findings = validateFieldConsumersUnrecorded(...args); fired.push(...findings); @@ -1589,3 +1589,39 @@ describe('[#22161] one-line verdicts — field-no-consumers, the same-name claus } }); }); + +// [#22161] Slice 11: the `fix:` line names the first carrier site and counts +// the rest, the shape the verdict's same-name clause uses. It used to list every +// site (16 of them, 629 characters, on the showcase's `showcase_account.website`). +describe('[#22161] one-line fixes — field-no-consumers, the carrier sites bounded', () => { + /** One object whose `description` only seeds carry, `n` records deep. */ + const seededOnly = (n: number): AnyRec => ({ + objects: [{ name: 'obj_0', fields: { name: { type: 'text' }, description: { type: 'textarea' } } }], + views: [{ name: 'v', object: 'obj_0', list: { type: 'grid', data: { provider: 'object', object: 'obj_0' }, columns: ['name'] } }], + data: [{ object: 'obj_0', records: Array.from({ length: n }, (_, i) => ({ name: `r${i}`, description: `d${i}` })) }], + }); + + it('names the first carrier site and counts the rest, however many there are', () => { + const f = validateFieldConsumers(seededOnly(12)).find((x) => x.field === 'description'); + expect(f?.verdict).toBe('carrier-only'); + expect(f!.carriers.length).toBeGreaterThan(1); + expect(f!.hint).toBe( + 'add it to a view column or a form section, or remove the declaration together with its ' + + `${f!.carriers.length} carrier site(s): ${f!.carriers[0]} (and ${f!.carriers.length - 1} more)`, + ); + // The finding still holds every site for a programmatic caller. + expect(new Set(f!.carriers).size).toBe(f!.carriers.length); + }); + + 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 === FIELD_NO_CONSUMERS); + // Coverage control: the inert fix, a one-site carrier fix and a counted one. + expect(converted.some((f) => f.hint === 'add it to a view column or a form section, or remove the declaration')).toBe(true); + expect(converted.some((f) => / 1 carrier site\(s\): \S+$/.test(f.hint))).toBe(true); + expect(converted.some((f) => / carrier site\(s\): \S+ \(and \d+ more\)$/.test(f.hint))).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); + } + }); +}); diff --git a/packages/lint/src/validate-field-consumers.ts b/packages/lint/src/validate-field-consumers.ts index 07796cba7ec..7cf2aab4208 100644 --- a/packages/lint/src/validate-field-consumers.ts +++ b/packages/lint/src/validate-field-consumers.ts @@ -1222,8 +1222,16 @@ function creditAnalyticsColumns(ledger: ConsumerLedger, stack: AnyRec): void { } } -function listPaths(paths: readonly string[]): string { - return paths.join(', '); +/** + * [#22161] The carrier sites as the `fix:` line names them: the first, then a + * count of the rest — the shape the verdict's `sharedClause` already uses for + * the other objects. A field named in every translation, seed and grant used + * to list them all on one line (16 sites, 629 characters, on the showcase). + * The finding's `carriers` list still holds every site for a caller of this + * function. + */ +function firstPathAndCount(paths: readonly string[]): string { + return `${paths[0]}${paths.length > 1 ? ` (and ${paths.length - 1} more)` : ''}`; } /** @@ -1301,7 +1309,8 @@ export function validateFieldConsumers(stack: AnyRec): FieldConsumerFinding[] { // sites a removal must clean, and the other objects whose consumers of the // same name do not count — the first one named and the rest counted, since // a field name common to every object (`description`, `name`) would - // otherwise list them all on one line. + // otherwise list them all on one line. The `fix:` line names the carrier + // sites the same way (#22161, slice 11). const verdictTag = verdict === 'carrier-only' ? `carrier-only: ${carriers.length} site(s) name it without reading it` @@ -1321,7 +1330,7 @@ export function validateFieldConsumers(stack: AnyRec): FieldConsumerFinding[] { hint: `add it to a view column or a form section, or remove the declaration` + (carriers.length > 0 - ? ` together with its ${carriers.length} carrier site(s): ${listPaths(carriers)}` + ? ` together with its ${carriers.length} carrier site(s): ${firstPathAndCount(carriers)}` : ''), object, field, diff --git a/packages/lint/src/validate-flow-trigger-readiness.test.ts b/packages/lint/src/validate-flow-trigger-readiness.test.ts index de7258806b4..e5e34d0572c 100644 --- a/packages/lint/src/validate-flow-trigger-readiness.test.ts +++ b/packages/lint/src/validate-flow-trigger-readiness.test.ts @@ -31,7 +31,7 @@ const NEVER_FIRE_IDS: readonly string[] = [ FLOW_API_TRIGGER_SECRET_MISSING, FLOW_TRIGGER_UNKNOWN_EVENT, ]; -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateFlowTriggerReadiness: typeof validateFlowTriggerReadinessUnrecorded = (...args) => { const findings = validateFlowTriggerReadinessUnrecorded(...args); fired.push(...findings); @@ -1714,3 +1714,28 @@ describe('[#22161] one-line verdicts — the never-fire family', () => { expect(explanationOf(FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID)).toContain('`runAs` is a FLOW-level key'); }); }); + +// [#22161] Slice 11: the `fix:` line. `flow-time-relative-descriptor-invalid`'s +// hint said "satisfies each message above"; the verdict quotes ONE issue and +// counts the rest, so the hint now names the remedy in one line, and why a +// descriptor the schema accepts binds is the explanation's. +describe('[#22161] one-line fixes — flow-time-relative-descriptor-invalid', () => { + 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 === FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID); + expect(converted.length).toBeGreaterThan(0); + for (const f of converted) { + expect(f.hint).toBe( + 'Correct config.timeRelative to TimeRelativeTriggerSchema, starting with the key the verdict names, ' + + 'and re-run until the schema accepts it: a descriptor the schema accepts binds', + ); + expect(f.hint.length).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries what the `fix:` line no longer says', () => { + const text = explanationOf(FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID); + expect(text).toContain('the same schema the trigger safeParses at bind time'); + expect(text).toContain('an unrecognized key names the declared key it was probably meant to be'); + expect(text).toContain('content/docs/references/automation/time-relative-trigger.mdx'); + }); +}); diff --git a/packages/lint/src/validate-flow-trigger-readiness.ts b/packages/lint/src/validate-flow-trigger-readiness.ts index 4164b9c057d..1263ccae5b7 100644 --- a/packages/lint/src/validate-flow-trigger-readiness.ts +++ b/packages/lint/src/validate-flow-trigger-readiness.ts @@ -538,13 +538,13 @@ export function validateFlowTriggerReadiness(stack: AnyRec): FlowTriggerReadines // whole list, and why the sweep is never installed is // `os explain flow-time-relative-descriptor-invalid`. message: `config.timeRelative never binds, so the flow never runs: ${descriptorRefusal(parsed.error.issues)}`, - // [#22161] `hint` is the CLI's `fix:` line: the instruction first, the - // reason it is sufficient after it. + // [#22161] `hint` is the CLI's `fix:` line, one remedy line: it said + // "satisfies each message above", and the verdict quotes ONE issue + // and counts the rest. Why a descriptor the schema accepts binds, + // and where its keys are documented, is the explanation's. hint: - `Correct config.timeRelative until it satisfies each message above: they are ` + - `TimeRelativeTriggerSchema's own, the same schema the trigger safeParses at bind time, so a ` + - `descriptor that satisfies them binds. An unrecognized key names the declared key it was ` + - `probably meant to be; see content/docs/references/automation/time-relative-trigger.mdx.`, + 'Correct config.timeRelative to TimeRelativeTriggerSchema, starting with the key the verdict names, ' + + 'and re-run until the schema accepts it: a descriptor the schema accepts binds', }); } } diff --git a/packages/lint/src/validate-react-page-props.test.ts b/packages/lint/src/validate-react-page-props.test.ts index c9d61c26eb2..d8ae7bd1375 100644 --- a/packages/lint/src/validate-react-page-props.test.ts +++ b/packages/lint/src/validate-react-page-props.test.ts @@ -41,7 +41,7 @@ const ONE_LINE_IDS: readonly string[] = [ REACT_BLOCK_NEEDS_RECORD_CONTEXT, REACT_PAGE_SOURCE_UNPARSEABLE, ]; -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateReactPageProps: typeof validateReactPagePropsUnrecorded = (...args) => { const findings = validateReactPagePropsUnrecorded(...args); fired.push(...findings); @@ -1252,8 +1252,11 @@ describe('validateReactPageProps — PARSED (#5020)', () expect(hit.length).toBe(1); expect(hit[0].severity).toBe('warning'); expect(hit[0].message).toContain('aggregate.groupBy is not set'); - expect(hit[0].hint).toContain('groupBy stays required, and a single number belongs in an object-metric block instead'); + // [#22161] One line now; the ruling stays on it, and why the finding is + // still only a warning is the explanation's. + expect(hit[0].hint).toContain('a single number belongs in an object-metric block instead'); expect(hit[0].hint, 'the hint must carry the RULING, not an open question').toContain('NOT a supported'); + expect(explanationOf(REACT_CHART_AGGREGATE_INVALID)).toContain('promoting it is its own step'); expect( validateReactPageProps(agg(`{ function: 'count' }`)).filter((x) => x.severity === 'error'), 'nothing about this aggregate may gate the build', @@ -1706,3 +1709,39 @@ describe('[#22161] one-line verdicts — the five react-page ids', () => { } }); }); + +// [#22161] Slice 11: the `fix:` line. The two chart ids' hints said "the +// rejection above carries the fix" after the verdict had been cut to the +// schema refusal's head; each now names the remedy itself, in one line, and +// the absent-`groupBy` arm's ruling fits one line too. +describe('[#22161] one-line fixes — react-chart-drilldown-invalid, react-chart-aggregate-invalid', () => { + const FIX_IDS: readonly string[] = [REACT_CHART_DRILLDOWN_INVALID, REACT_CHART_AGGREGATE_INVALID]; + + 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) => FIX_IDS.includes(f.rule)); + // Coverage control: both ids fired on their schema-refusal arm, and the + // aggregate id on its absent-groupBy arm, so the bound cannot pass over one. + for (const id of FIX_IDS) { + expect(converted.some((f) => f.rule === id && f.hint.startsWith('Correct the ')), `${id} refusal arm`).toBe(true); + } + expect(converted.some((f) => f.hint.startsWith('Add aggregate.groupBy'))).toBe(true); + for (const f of converted) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint, f.rule).not.toMatch(/\babove\b/); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('each schema-refusal arm names its remedy', () => { + const drill = fired.find((f) => f.rule === REACT_CHART_DRILLDOWN_INVALID && f.hint.startsWith('Correct the ')); + expect(drill?.hint).toBe( + 'Correct the drillDown key or value the verdict names to ChartDrillDownSchema (@objectstack/spec/ui): use ' + + 'the rename it suggests, or delete a key that is real only on another surface', + ); + const agg = fired.find((f) => f.rule === REACT_CHART_AGGREGATE_INVALID && f.hint.startsWith('Correct the ')); + expect(agg?.hint).toBe( + 'Correct the aggregate key or value the verdict names to ChartAggregateSchema (@objectstack/spec/ui): use ' + + 'the rename it suggests, or move a key that is real one layer out to that layer', + ); + }); +}); diff --git a/packages/lint/src/validate-react-page-props.ts b/packages/lint/src/validate-react-page-props.ts index 9f1728087ba..5e00b833203 100644 --- a/packages/lint/src/validate-react-page-props.ts +++ b/packages/lint/src/validate-react-page-props.ts @@ -377,7 +377,12 @@ function checkChartDrillDown( // its rename. What the schema adds after that is // `os explain react-chart-drilldown-invalid`'s. `${at}: ${schemaRefusalHead(issue.message)}`, - 'The drill config is declared by ChartDrillDownSchema (@objectstack/spec/ui) — the rejection above carries the fix.', + // [#22161] The remedy itself, in one line: it used to say "the + // rejection above carries the fix", and the verdict above no longer + // carries the schema's prescription for a key real on another surface + // (the explanation names each such key). + 'Correct the drillDown key or value the verdict names to ChartDrillDownSchema (@objectstack/spec/ui): use ' + + 'the rename it suggests, or delete a key that is real only on another surface', ); } } @@ -460,9 +465,10 @@ function checkChartAggregate( 'warning', REACT_CHART_AGGREGATE_INVALID, 'aggregate.groupBy is not set, so the aggregate returns ONE ungrouped row and the chart plots a single point.', - 'Add aggregate.groupBy (a field name, or { field, dateGranularity } to bucket dates) to give the chart a category axis. ' + - 'An ungrouped single-value chart is NOT a supported shape — groupBy stays required, and a single number belongs in an object-metric block instead. ' + - 'This stays a warning rather than an error only because promoting it is its own step.', + // [#22161] One line, the ruling kept; why it is still only a warning is + // the explanation's. + 'Add aggregate.groupBy (a field name, or { field, dateGranularity } to bucket dates): an ungrouped chart is ' + + 'NOT a supported shape, and a single number belongs in an object-metric block instead', ); } @@ -482,7 +488,11 @@ function checkChartAggregate( issue.code === 'invalid_union' ? `${at} matches no accepted form: ${unionRefusal(issue as LintZodIssue, raw)}` : `${at}: ${schemaRefusalHead(describeIssue(issue, raw))}`, - 'The aggregate is declared by ChartAggregateSchema (@objectstack/spec/ui) — the rejection above carries the fix.', + // [#22161] The remedy itself, in one line (it said "the rejection above + // carries the fix"; the verdict no longer carries the schema's + // prescription for a key real one layer out — the explanation does). + 'Correct the aggregate key or value the verdict names to ChartAggregateSchema (@objectstack/spec/ui): use ' + + 'the rename it suggests, or move a key that is real one layer out to that layer', ); } } diff --git a/packages/lint/src/validate-readonly-hook-writes.test.ts b/packages/lint/src/validate-readonly-hook-writes.test.ts index b058595a9dc..a1b5aa541f3 100644 --- a/packages/lint/src/validate-readonly-hook-writes.test.ts +++ b/packages/lint/src/validate-readonly-hook-writes.test.ts @@ -30,7 +30,7 @@ import { explainRule } from './rule-explanations.js'; // the lowered-handler suffix included, not a chosen few. Run the whole file: // that case reads what the cases above fired. const SHORTENED_RULE_IDS: readonly string[] = [HOOK_API_UPDATE_READONLY_FIELD, HOOK_API_UPDATE_READONLY_WHEN_FIELD]; -const firedShortened: Array<{ rule: string; message: string }> = []; +const firedShortened: Array<{ rule: string; message: string; hint: string }> = []; const validateReadonlyHookWrites: typeof validateReadonlyHookWritesUnrecorded = (...args) => { const findings = validateReadonlyHookWritesUnrecorded(...args); for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f); @@ -100,7 +100,9 @@ describe('validateReadonlyHookWrites - RED: a ctx.api write to a readonly field' // under the default onError:'abort'). Substring-matching 'sudo' is not // enough to tell "offers it" from "warns against it": the corrected hint // still contains the word. Pin the DIRECTION. - expect(findings[0].hint).toMatch(/sudo\(\) is NOT an option|not marshalled into the sandbox/); + // [#22161] The one-line `fix:` keeps the direction; the explanation says why. + expect(findings[0].hint).toMatch(/sudo\(\) is no option from a body/); + expect(explainRule(HOOK_API_UPDATE_READONLY_FIELD)?.paragraphs.join('\n')).toContain('not marshalled into the sandbox'); expect(findings[0].hint).not.toMatch(/make the elevation explicit|write it through ctx\.api\.sudo/); }); @@ -320,7 +322,9 @@ describe('validateReadonlyHookWrites - RED: insert() of a static-readonly field expect(findings[0].hint).toContain("runAs: 'system'"); expect(findings[0].hint).toContain('beforeInsert'); expect(findings[0].hint).toContain('ctx.input.last_activity_date'); - expect(findings[0].hint).toMatch(/sudo\(\) is NOT an option|not marshalled into the sandbox/); + // [#22161] The one-line `fix:` keeps the direction; the explanation says why. + expect(findings[0].hint).toMatch(/sudo\(\) is no option from a body/); + expect(explainRule(HOOK_API_UPDATE_READONLY_FIELD)?.paragraphs.join('\n')).toContain('not marshalled into the sandbox'); expect(findings[0].hint).not.toMatch(/make the elevation explicit|write it through ctx\.api\.sudo/); }); @@ -843,3 +847,26 @@ describe('[#22161] one-line verdicts — the rule ids this file shortened', () = for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); }); }); + +// [#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.) +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); + // Coverage control: the update and the create arm both fired. + expect(converted.some((f) => f.hint.startsWith('Stamp it on the record'))).toBe(true); + expect(converted.some((f) => f.hint.startsWith("Declare runAs: 'system'"))).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 alternatives the `fix:` line no longer lists', () => { + const text = explainRule(HOOK_API_UPDATE_READONLY_FIELD)!.paragraphs.join('\n'); + expect(text).toContain('Seeding a readonly column at create time is a SYSTEM act'); + expect(text).toContain('drop `readonly: true` from the field'); + }); +}); diff --git a/packages/lint/src/validate-readonly-hook-writes.ts b/packages/lint/src/validate-readonly-hook-writes.ts index c9c3e63a3cf..d3eea726239 100644 --- a/packages/lint/src/validate-readonly-hook-writes.ts +++ b/packages/lint/src/validate-readonly-hook-writes.ts @@ -459,21 +459,16 @@ export function validateReadonlyHookWrites( `body's ${call} writes readonly field '${w.field}', silently stripped on a non-system trigger` + (isCreate ? `, ${READONLY_INSERT_STRIP_OUTCOME}` : '') + loc.messageSuffix, + // [#22161] One remedy line per verb: the two routes and the + // refusal of sudo(). Why runAs: 'system' lands the write, why the + // own-hook stamp survives, why sudo() is a TypeError from a body, + // and the drop-the-field / drop-readonly alternatives are + // `os explain hook-api-update-readonly-field`. hint: isCreate - ? `Seeding a readonly column at create time is a SYSTEM act. To keep writing it from here, ` + - `declare runAs: 'system' on this hook: the strip skips a system context, so the write lands, ` + - `and the triggering user is still stamped on the record. Or stamp it on '${objectName}''s OWN ` + - `beforeInsert hook - ctx.input.${w.field} = ... is a server value and survives the strip. ` + - `Note that ctx.api.sudo() is NOT an option from a body: sudo() lives on the in-process ` + - `ScopedContext and is not marshalled into the sandbox, so calling it here is a TypeError at ` + - `run time. Otherwise drop '${w.field}' from this payload, or drop readonly:true from the field.` - : `If automation is meant to maintain '${w.field}', stamp it on the record's OWN hook - ` + - `ctx.input.${w.field} = ... in beforeInsert/beforeUpdate survives the strip, and is the ` + - `recommended shape. To keep writing it CROSS-OBJECT from here, declare runAs: 'system' on ` + - `this hook: the strip skips a system context, so the write lands, and the triggering user ` + - `is still stamped on the record. Note that ctx.api.sudo() is NOT an option from a body: ` + - `sudo() lives on the in-process ScopedContext and is not marshalled into the sandbox, so ` + - `calling it here is a TypeError at run time. Otherwise drop readonly:true from '${w.field}'.`, + ? `Declare runAs: 'system' on this hook, or stamp it in the beforeInsert hook of '${objectName}' ` + + `(ctx.input.${w.field} = ...); ctx.api.sudo() is no option from a body` + : `Stamp it on the record's OWN hook (ctx.input.${w.field} = ... in beforeInsert/beforeUpdate), or ` + + `declare runAs: 'system' on this hook; ctx.api.sudo() is no option from a body`, }); } else if (meta.readonlyWhen && !isCreate) { reported.add(dedupeKey); diff --git a/packages/lint/src/validate-retired-permission-residue.test.ts b/packages/lint/src/validate-retired-permission-residue.test.ts index 859d2065a8f..2b15e9ac451 100644 --- a/packages/lint/src/validate-retired-permission-residue.test.ts +++ b/packages/lint/src/validate-retired-permission-residue.test.ts @@ -13,6 +13,7 @@ import { normalizeStackInput } from '@objectstack/spec'; import { PERMISSION_RETIRED_LIFECYCLE_RESIDUE, retiredKeyPrescription, + retiredKeyRemedy, validateRetiredPermissionResidue as validateRetiredPermissionResidueUnrecorded, } from './validate-retired-permission-residue.js'; import { AUTHORING_COMMANDS, authoringRulesFor, runAuthoringRules } from './authoring-rules.js'; @@ -24,7 +25,7 @@ import { explainRule } from './rule-explanations.js'; // recorded 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 validateRetiredPermissionResidue: typeof validateRetiredPermissionResidueUnrecorded = (...args) => { const findings = validateRetiredPermissionResidueUnrecorded(...args); fired.push(...findings); @@ -107,11 +108,16 @@ describe('validateRetiredPermissionResidue (#17425)', () => { (ObjectPermissionSchema as unknown as { shape: Record }) .shape.allowRestore.description, ).replace(/^\[REMOVED\]\s*/, ''); - expect(finding.hint).toBe(fromSchema); - // Anti-vacuity: an empty derivation would make the assertion above true - // and the rule silent. The prescription's two load-bearing clauses. - expect(finding.hint).toContain('Delete the key'); - expect(finding.hint).toContain('os migrate meta --from 17'); + // [#22161] The `fix:` line is the prescription's remedy sentence, CUT + // from it — never retyped — so it is a piece of the schema's own text. + expect(finding.hint).toBe(retiredKeyRemedy(fromSchema)); + expect(fromSchema).toContain(finding.hint); + // Anti-vacuity: an empty derivation would make the assertions above true + // and the rule silent. The prescription's two load-bearing clauses, and + // the remedy the `fix:` line keeps. + expect(fromSchema).toContain('Delete the key'); + expect(fromSchema).toContain('os migrate meta --from 17'); + expect(finding.hint).toBe('Delete the key'); }); it('the prescription resolves for every key the rule knows about', () => { @@ -236,3 +242,33 @@ describe('[#22161] one-line verdicts — permission-retired-lifecycle-residue', } }); }); + +// [#22161] Slice 11: the `fix:` line. The finding used to carry the whole +// retirement prescription (573 and 681 characters); it carries the remedy +// sentence, cut from it, and the explanation says what deleting the key leaves. +describe('[#22161] one-line fixes — permission-retired-lifecycle-residue', () => { + 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 === PERMISSION_RETIRED_LIFECYCLE_RESIDUE); + expect(converted.some((f) => f.message.startsWith('sets `allowRestore: false`'))).toBe(true); + expect(converted.some((f) => f.message.startsWith('sets `allowPurge: false`'))).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('retiredKeyRemedy cuts the "Delete the key" sentence, at its dash only past 200, and never invents', () => { + expect(retiredKeyRemedy('`k` was removed. Delete the key — x. Run it.')).toBe('Delete the key — x'); + expect(retiredKeyRemedy(`\`k\` was removed. Delete the key — ${'x'.repeat(200)}. Run it.`)).toBe('Delete the key'); + // No remedy sentence: the prescription is returned whole, never replaced. + expect(retiredKeyRemedy('`k` was removed.')).toBe('`k` was removed.'); + }); + + it('`os explain` says what deleting the key leaves, and names no tracker id', () => { + const text = explainRule(PERMISSION_RETIRED_LIFECYCLE_RESIDUE)!.paragraphs.join('\n'); + expect(text).toContain('stays denied fail-closed by the permission evaluator\'s destructive-operation backstop'); + expect(text).toContain('the M2 lifecycle initiative'); + expect(text).not.toMatch(/os migrate meta --from/); + expect(text).not.toMatch(/#\d{3,}/); + }); +}); diff --git a/packages/lint/src/validate-retired-permission-residue.ts b/packages/lint/src/validate-retired-permission-residue.ts index 9348c9845e9..52140529de2 100644 --- a/packages/lint/src/validate-retired-permission-residue.ts +++ b/packages/lint/src/validate-retired-permission-residue.ts @@ -51,7 +51,8 @@ * pinned class-wide by `retired-key-migrate-sentence.test.ts` over in * `packages/spec`. A copy here would be a second wording free to drift from the * parse-time one the same author sees through the other door, so the hint is - * resolved from `ObjectPermissionSchema`'s own shape at call time. An + * resolved from `ObjectPermissionSchema`'s own shape at call time — [#22161] + * and cut to its remedy sentence ({@link retiredKeyRemedy}), never retyped. An * unresolvable prescription yields NO finding rather than a hint this module * invented — the same refusal to invent that `lintLivenessProperties` applies * to an unreadable ledger (which since #19276 it also REPORTS once, rather than @@ -109,6 +110,27 @@ export function retiredKeyPrescription(key: string): string | null { return prescription.length > 0 ? prescription : null; } +/** + * [#22161] The `fix:` line, cut from the retirement's own prescription: its + * "Delete the key" sentence up to its end, and up to its first ` — ` when the + * whole sentence would run past 200 characters (both keys' do: the clause after + * the dash, on what stays denied, is the explanation's). It cuts and never + * rewrites, so the single-wording discipline above holds; a prescription with + * no such sentence is returned whole rather than replaced by an invented line. + */ +export function retiredKeyRemedy(prescription: string): string { + const at = prescription.search(/\bdelete the key\b/i); + if (at < 0) return prescription; + let remedy = prescription.slice(at); + const stop = remedy.indexOf('. '); + remedy = stop >= 0 ? remedy.slice(0, stop) : remedy.replace(/\.$/, ''); + if (remedy.length > 200) { + const dash = remedy.indexOf(' — '); + if (dash > 0) remedy = remedy.slice(0, dash); + } + return remedy.charAt(0).toUpperCase() + remedy.slice(1); +} + /** * Flag every authored object-permission entry carrying a retired lifecycle * key at its inert residue value. Advisory only — returns findings, never @@ -131,8 +153,8 @@ export function validateRetiredPermissionResidue(stack: AnyRec): RetiredPermissi // compares. `0`/`''`/`null` are NOT this value and are refused at the // parse with the prescription already attached. if (entry[key] !== residue) continue; - const hint = retiredKeyPrescription(key); - if (hint === null) continue; + const prescription = retiredKeyPrescription(key); + if (prescription === null) continue; findings.push({ rule: PERMISSION_RETIRED_LIFECYCLE_RESIDUE, severity: 'warning', @@ -144,7 +166,7 @@ export function validateRetiredPermissionResidue(stack: AnyRec): RetiredPermissi message: `sets \`${key}: ${JSON.stringify(residue)}\`, the retired default of a key removed in ` + '@objectstack/spec 17, which the parse strips in silence, so this line grants nothing', - hint, + hint: retiredKeyRemedy(prescription), }); } } 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 4d1c477f34e..f8e6862b606 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 @@ -27,6 +27,7 @@ import { validateRlsPredicateEnforceability as validateRlsPredicateEnforceabilityUnrecorded, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNKNOWN_FIELD, + CROSS_FIELD_CLASS_LISTING, } from './validate-rls-predicate-enforceability.js'; import { runAuthoringRules } from './authoring-rules.js'; import { runRuntimeAuthoringRules } from './runtime-gate.js'; @@ -37,7 +38,7 @@ import { explainRule } from './rule-explanations.js'; // fired, and the last case in this file holds each recorded verdict to one // line of at most 200 characters. Run the whole file: that case reads what the // cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); fired.push(...findings); @@ -142,7 +143,7 @@ describe('validateRlsPredicateEnforceability — a field compared with a field o `RLS ${clause} compares \`record.${left} ${quoted} record.${right}\`, which no comparison class spans ` + `(${why}), ${REFUSED[clause]}`, ); - expect(finding.hint).toMatch(/^Compare a field only with a field of the same comparison class: /); + expect(finding.hint).toMatch(/^Compare a field only with a field of its comparison class, or with a literal or a `current_user` value/); }); } } @@ -174,15 +175,21 @@ describe('validateRlsPredicateEnforceability — a field compared with a field o expect(text).toContain('across two classes every insert or update it judges'); }); - it('the prescription names every class with the declared types it holds, read from the spec', () => { + it('the explanation names every class with the declared types it holds, read from the spec', () => { + // [#22161] The listing was the `fix:` line's, computed from the spec's + // class table; the explanation writes it out (it imports nothing), so it is + // held here to the rule's own computed listing, and the `fix:` is one line. const [finding] = validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.status != record.amount' })); - expect(finding.hint).toContain('a number only with a number (`number`, `currency`, `percent`, `rating`, `slider`, `progress`, `summary`)'); - expect(finding.hint).toContain('a boolean only with a boolean (`boolean`, `toggle`)'); - expect(finding.hint).toContain('a date only with a date (`date`)'); - expect(finding.hint).toContain('a datetime only with a datetime (`datetime`)'); - expect(finding.hint).toContain('a time of day only with a time of day (`time`)'); + expect(finding.hint.length).toBeLessThanOrEqual(200); + const text = explanationOf(RLS_PREDICATE_UNENFORCEABLE); + expect(text).toContain(`Compare a field only with a field of the same comparison class: ${CROSS_FIELD_CLASS_LISTING}.`); + expect(text).toContain('a number only with a number (`number`, `currency`, `percent`, `rating`, `slider`, `progress`, `summary`)'); + expect(text).toContain('a boolean only with a boolean (`boolean`, `toggle`)'); + expect(text).toContain('a date only with a date (`date`)'); + expect(text).toContain('a datetime only with a datetime (`datetime`)'); + expect(text).toContain('a time of day only with a time of day (`time`)'); for (const t of ['text', 'autonumber', 'select', 'radio', 'lookup', 'master_detail', 'user', 'tree']) { - expect(finding.hint).toMatch(new RegExp(`text only with text \\([^)]*\`${t}\``)); + expect(text).toMatch(new RegExp(`text only with text \\([^)]*\`${t}\``)); } }); }); @@ -360,3 +367,18 @@ describe('[#22161] one-line verdicts', () => { } }); }); + +// [#22161] Slice 11: the `fix:` line of every finding this file fired is one +// remedy sentence; what it stopped saying is the id's `os explain` entry. +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))) { + 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 eedab8fecea..45134ea6c47 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 @@ -29,7 +29,7 @@ import { explainRule } from './rule-explanations.js'; // fired, and the last case in this file holds each recorded verdict to one // line of at most 200 characters. Run the whole file: that case reads what the // cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); fired.push(...findings); @@ -133,10 +133,18 @@ describe('validateRlsPredicateEnforceability — the engine judge is an INPUT (# return validateRlsPredicateEnforceability(stackWith({ using: 'owner == current_user.id' }), { judgeFilter })[0] .hint; }; - expect(hintFor('FILTER_TOKEN_UNKNOWN')).toMatch(/Write `owner == current_user\.id`/); - expect(hintFor('FILTER_TOKEN_UNRESOLVED')).toMatch(/Write `owner == current_user\.id`/); + // [#22161] One line each; the `current_user` keys and what each operator + // runs on are the explanation's. + expect(hintFor('FILTER_TOKEN_UNKNOWN')).toMatch(/`owner == current_user\.id`, not `owner == '\{current_user_id\}'`/); + expect(hintFor('FILTER_TOKEN_UNRESOLVED')).toMatch(/`owner == current_user\.id`, not `owner == '\{current_user_id\}'`/); expect(hintFor('INVALID_FIELD')).toMatch(/Denormalise the value/); - expect(hintFor('INVALID_FILTER')).toMatch(/a text operator .* only on a field that holds a string/); + expect(hintFor('INVALID_FILTER')).toMatch(/a text operator only on a field that holds a string/); + for (const code of ['FILTER_TOKEN_UNKNOWN', 'INVALID_FIELD', 'INVALID_FILTER']) { + expect(hintFor(code).length, code).toBeLessThanOrEqual(200); + } + expect(explainRule(RLS_PREDICATE_UNENFORCEABLE)?.paragraphs.join('\n')).toContain( + 'the caller\'s values are the `current_user` keys (accessible_org_ids, email, id, org_user_ids, organization_id, positions)', + ); }); it('judges ONLY the read scope: `using` on a `select` / `all` policy — never a `check`, never another operation', () => { @@ -256,3 +264,18 @@ describe('[#22161] one-line verdicts', () => { } }); }); + +// [#22161] Slice 11: the `fix:` line of every finding this file fired is one +// remedy sentence; what it stopped saying is the id's `os explain` entry. +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))) { + 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 1d7165418bf..0069ea1ad2f 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 @@ -36,7 +36,7 @@ import { explainRule } from './rule-explanations.js'; // fired, and the last case in this file holds each recorded verdict to one // line of at most 200 characters. Run the whole file: that case reads what the // cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); fired.push(...findings); @@ -122,7 +122,7 @@ describe('validateRlsPredicateEnforceability — a field compared with a json / `RLS ${clause} compares \`record.${left} ${quoted} record.${right}\`, where \`${field}\` holds a list ` + `or an object, ${REFUSED[clause]}`, ); - expect(finding.hint).toMatch(/^A field compared with a `json` or `multiple` field has no row-filter form/); + expect(finding.hint).toMatch(/^Compare with a single-valued column, a literal or a `current_user` value/); }); } } @@ -318,3 +318,18 @@ describe('[#22161] one-line verdicts', () => { } }); }); + +// [#22161] Slice 11: the `fix:` line of every finding this file fired is one +// remedy sentence; what it stopped saying is the id's `os explain` entry. +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))) { + 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 54772b062a6..b94b0807d65 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -29,7 +29,7 @@ const RLS_RULE_IDS: readonly string[] = [ RLS_PREDICATE_UNKNOWN_FIELD, RLS_PREDICATE_UNKNOWN_USER_VARIABLE, ]; -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); fired.push(...findings); @@ -86,9 +86,11 @@ describe('validateRlsPredicateEnforceability — predicates the runtime can only expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/DROPS a policy/); expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/RLS_DENY_FILTER/); expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/ZERO rows/); - // …and prescribe the fix that works on THIS surface. - expect(findings[0].hint).toMatch(/field != null/); - expect(findings[0].hint).toMatch(/INTERPRETED/); + // …and prescribe the fix that works on THIS surface: the `fix:` line in + // one line, and why `has()` is wrong here in the explanation (#22161). + expect(findings[0].hint).toMatch(/record\.x != null/); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/field != null/); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/INTERPRETED/); }); it.each([ @@ -421,20 +423,26 @@ describe('validateRlsPredicateEnforceability — a bounds overrun is its own id it('prescribes shrinking, and never sends the author to check their dialect', () => { const [f] = atGa(() => validateRlsPredicateEnforceability(policyWith('using', OVER_BUDGET.maxAstNodes))); - // The real remedies. + // [#22161] The `fix:` line names the remedies in one line; the four + // options in full are the explanation's. + const why = explanationOf(RLS_PREDICATE_OVER_BUDGET); expect(f.hint).toMatch(/field in \[a, b, …\]/); expect(f.hint).toMatch(/current_user\./); - expect(f.hint).toMatch(/[Dd]enormalise/); - expect(f.hint).toMatch(/hook or action body/); + expect(why).toMatch(/[Dd]enormalise/); + expect(why).toMatch(/hook or action body/); // Splitting is only sound on a top-level `||`; policies are OR-ed, so - // splitting an `&&` would WIDEN access. Saying so is the point of the hint. - expect(f.hint).toMatch(/never split a top-level `&&`/); - expect(f.hint).toMatch(/WIDEN access/); + // splitting an `&&` would WIDEN access. Saying so is the point of the + // hint: the one-line `fix:` keeps the warning, the explanation says why. + expect(f.hint).toMatch(/split a top-level `\|\|` across policies \(never a top-level `&&`\)/); + expect(why).toMatch(/never split a top-level `&&`/); + expect(why).toMatch(/WIDEN access/); // …and explicitly NOT the SQL-vs-CEL prose this class used to get. - expect(f.hint).toMatch(/no syntax or dialect error/); - expect(f.hint).not.toMatch(/canonical CEL \(ADR-0058 D1\)/); - expect(f.hint).not.toMatch(/rather than SQL `AND` \/ `OR`/); - expect(f.hint).not.toMatch(/LIKE/); + expect(why).toMatch(/no syntax or dialect error/); + for (const text of [f.hint, why]) { + expect(text).not.toMatch(/canonical CEL \(ADR-0058 D1\)/); + expect(text).not.toMatch(/rather than SQL `AND` \/ `OR`/); + expect(text).not.toMatch(/LIKE/); + } }); it('names the bound that was actually blown, not a hard-coded one', () => { @@ -692,9 +700,14 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not // …and the miss itself, with the platform's own "did you mean". expect(f.message).toMatch(/"is_private_nope" is not a field on object "crm_opportunity"/); expect(f.message).toMatch(/Did you mean "is_private"\?/); - // The hint lists the columns that DO exist and names the rename story. - expect(f.hint).toMatch(/Fields on "crm_opportunity": amount, assigned_to_id/); - expect(f.hint).toMatch(/RENAMED/); + // [#22161] The `fix:` line names the remedy in one line — the verdict's + // "Did you mean" names the nearest column — and the explanation tells the + // rename story. + expect(f.hint).toBe( + 'Point the predicate at a column the object declares (denormalise a value from another object onto it ' + + 'first), or delete the policy if the narrowing is gone', + ); + expect(why).toMatch(/RENAMED/); }); it('says the same for an unresolved `current_user.*`, and prescribes the §7.3.1 route', () => { @@ -890,10 +903,28 @@ describe('validateRlsPredicateEnforceability — the refusals the SHAPE check ca 'record.assigned_to_id in current_user.id', 'replace `record.assigned_to_id in current_user.id` with `record.assigned_to_id == current_user.id`', ], - ['record.assigned_to_id in current_user', '`record.assigned_to_id in current_user.org_user_ids`'], - ['record.assigned_to_id.startsWith(current_user)', '`record.assigned_to_id.startsWith(current_user.id)`'], - ['record.assigned_to_id.contains(current_user)', '`record.assigned_to_id.contains(current_user.email)`'], - ['record.assigned_to_id.endsWith(current_user)', '`record.assigned_to_id.endsWith(current_user.organization_id)`'], + // [#22161] The one-line `fix:` names the key SLOT for the bare root; the + // explanation lists the kernel keys by what each holds. + [ + 'record.assigned_to_id in current_user', + 'replace `record.assigned_to_id in current_user` with the membership key that holds the set: ' + + '`record.assigned_to_id in current_user.`', + ], + [ + 'record.assigned_to_id.startsWith(current_user)', + 'replace `record.assigned_to_id.startsWith(current_user)` with the key that holds the string: ' + + '`record.assigned_to_id.startsWith(current_user.)`', + ], + [ + 'record.assigned_to_id.contains(current_user)', + 'replace `record.assigned_to_id.contains(current_user)` with the key that holds the string: ' + + '`record.assigned_to_id.contains(current_user.)`', + ], + [ + 'record.assigned_to_id.endsWith(current_user)', + 'replace `record.assigned_to_id.endsWith(current_user)` with the key that holds the string: ' + + '`record.assigned_to_id.endsWith(current_user.)`', + ], [ 'record.assigned_to_id.startsWith(current_user.org_user_ids)', 'the membership test `record.assigned_to_id in current_user.org_user_ids`', @@ -963,13 +994,18 @@ describe('validateRlsPredicateEnforceability — the refusals the SHAPE check ca ); }); - it('rewrites EVERY site the fault lands in, in one finding', () => { + it('rewrites the first site the fault lands in and counts the rest, in one finding', () => { + // [#22161] One line: the first site's rewrite, then `(and N more)`; every + // site admits the same rewrite, which the explanation says. const findings = validateRlsPredicateEnforceability( siteWith('using', 'record.assigned_to_id != current_user.org_user_ids && record.owner_id != current_user.org_user_ids'), ); expect(findings.map((f) => f.rule)).toEqual([RLS_PREDICATE_UNENFORCEABLE]); - expect(findings[0].hint).toContain('`!(record.assigned_to_id in current_user.org_user_ids)`'); - expect(findings[0].hint).toContain('`!(record.owner_id in current_user.org_user_ids)`'); + expect(findings[0].hint).toBe( + 'replace `record.assigned_to_id != current_user.org_user_ids` with ' + + '`!(record.assigned_to_id in current_user.org_user_ids)` (and 1 more)', + ); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toContain('every site admits the same rewrite'); }); it('CONTROLS — the literal list keeps its one shape finding, and every spelling the hints point at is clean', () => { @@ -1216,3 +1252,54 @@ 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.) +describe('[#22161] one-line fixes — rls-predicate-unenforceable, -over-budget, -unknown-field', () => { + const ONE_LINE_FIX_IDS: readonly string[] = [ + RLS_PREDICATE_UNENFORCEABLE, + RLS_PREDICATE_OVER_BUDGET, + RLS_PREDICATE_UNKNOWN_FIELD, + ]; + + it('every `fix:` line of the three ids the cases above fired is one line of at most 200 characters', () => { + const converted = fired.filter((f) => ONE_LINE_FIX_IDS.includes(f.rule)); + // Coverage control: each id fired, and the unenforceable id on its shape, + // type-fault and null-comparand arms, so the bound cannot pass over one. + expect([...new Set(converted.map((f) => f.rule))].sort()).toEqual([...ONE_LINE_FIX_IDS].sort()); + const unenforceable = converted.filter((f) => f.rule === RLS_PREDICATE_UNENFORCEABLE).map((f) => f.hint); + expect(unenforceable.some((h) => h.startsWith('Rewrite the predicate inside the lowerable subset'))).toBe(true); + expect(unenforceable.some((h) => / with `!\(record\.\w+ in current_user\.\w+\)`/.test(h))).toBe(true); + expect(unenforceable.some((h) => h.includes('== null'))).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 = { + [RLS_PREDICATE_UNENFORCEABLE]: [ + 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`', + '(3) Arithmetic on a column (`amount * 2 > 100`) never lowers', + '"one of these" is `x in set`, "none of these" is `!(x in set)`', + 'an app-staged §7.3.1 set', + '`null` has no place inside a list or opposite an ordering operator', + 'or move the condition into a validation rule or a hook', + ], + [RLS_PREDICATE_OVER_BUDGET]: [ + '(1) Collapse a long', + '(ADR-0105 D11)', + '(3) Denormalise a repeated sub-expression', + 'never split a top-level `&&` this way, which would WIDEN access', + '`ScriptBody { language: \'js\' }`', + ], + [RLS_PREDICATE_UNKNOWN_FIELD]: ['RENAMED', 'RLS cannot join to it (ADR-0055)', 'the verdict\'s "Did you mean"'], + }; + for (const rule of ONE_LINE_FIX_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 36a07968cfa..a7401dfa2e3 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -331,10 +331,41 @@ function str(v: unknown): string { return typeof v === 'string' ? v : ''; } -const PUSHDOWN_SUBSET = - 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, ' + - 'and the string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column field paths ' + - '(ADR-0058 D2), compared against a literal or a `current_user.*` value.'; +/* + * [#22161] The `fix:` lines. Each finding's `hint` is one remedy sentence of + * 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. + */ + +/** Rewrite into the lowerable subset — the shape arm of `rls-predicate-unenforceable`. */ +const LOWERABLE_REWRITE = + 'Rewrite the predicate inside the lowerable subset (comparisons, `in`, `&&` `||` `!`, null tests and string ' + + 'methods over single-column fields); write `has(record.x)` as `record.x != null`'; + +/** A refused comparand with no `null` in it — the shared-face arm's other half. */ +const COMPARAND_REWRITE = + 'Rewrite the comparand the verdict names inside the lowerable subset: a literal or a `current_user.*` value ' + + 'compared with a single-column field'; + +/** `rls-predicate-over-budget`: the four ways to shrink, in one line; the `&&` warning is never dropped. */ +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-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 ' + + 'first), or delete the policy if the narrowing is gone'; + +/** `x (and N more)` — the first of several faulty sites' rewrites, and a count of the rest. */ +function firstAndCount(items: readonly string[]): string { + return `${items[0]}${items.length > 1 ? ` (and ${items.length - 1} more)` : ''}`; +} /** * The overrun behind a `parse-error`, or `null` when the refusal was a genuine @@ -812,13 +843,6 @@ function attributeTypeFault(bridged: string, probe: UserProbe): TypeFault | null return null; } -/** The kernel keys that hold `holding` on every request, from their declared types. */ -function kernelKeysHolding(holding: 'list' | 'one value'): string[] { - return [...PRERESOLVED_USER_KEYS] - .filter((key) => Array.isArray(kernelKeyProbe(key)) === (holding === 'list')) - .sort(); -} - /** The lowered string-method operators, back to the CEL method an author writes. */ const STRING_METHOD_OF: ReadonlyMap = new Map([ ['$startsWith', 'startsWith'], @@ -826,41 +850,35 @@ const STRING_METHOD_OF: ReadonlyMap = new Map([ ['$contains', 'contains'], ]); -/** The pasteable rewrite for one lowered site of a type fault. */ +/** + * The pasteable rewrite for one lowered site of a type fault — [#22161] the + * primary one only, for the one-line `fix:`. The alternatives a site also + * admits (pass one value instead of the set to a string method; name a + * membership key, or an app-staged §7.3.1 set, for a set) are the + * explanation's, beside the kernel keys by what each holds. + */ function typeFaultRewrite(fault: TypeFault, site: LoweredSite): string { const f = `record.${site.field}`; const v = fault.variable; const method = STRING_METHOD_OF.get(site.op); - const oneValueKeys = kernelKeysHolding('one value'); if (fault.holds === 'list') { if (site.op === '==') return `replace \`${f} == ${v}\` with \`${f} in ${v}\``; if (site.op === '$ne') return `replace \`${f} != ${v}\` with \`!(${f} in ${v})\``; - if (method) { - return ( - `replace \`${f}.${method}(${v})\` with the membership test \`${f} in ${v}\`, or pass one value: ` + - oneValueKeys.map((key) => `\`${f}.${method}(current_user.${key})\``).join(' / ') - ); - } + if (method) return `replace \`${f}.${method}(${v})\` with the membership test \`${f} in ${v}\``; } if (fault.holds === 'one value' && site.op === '$in') { - return ( - `replace \`${f} in ${v}\` with \`${f} == ${v}\` (and \`!(${f} in ${v})\` with \`${f} != ${v}\`); for a ` + - `set, name a membership key: ` + - kernelKeysHolding('list').map((key) => `\`${f} in current_user.${key}\``).join(' / ') - ); + return `replace \`${f} in ${v}\` with \`${f} == ${v}\``; } if (fault.holds === 'context object') { + // Which key is the author's to name: the explanation lists the kernel + // keys by what each holds, so the rewrite names the slot, not a guess. if (site.op === '$in') { - return ( - `replace \`${f} in current_user\` with the key that holds the set: ` + - kernelKeysHolding('list').map((key) => `\`${f} in current_user.${key}\``).join(' / ') + - `, or an app-staged §7.3.1 set \`${f} in current_user.\`` - ); + return `replace \`${f} in current_user\` with the membership key that holds the set: \`${f} in current_user.\``; } if (method) { return ( `replace \`${f}.${method}(current_user)\` with the key that holds the string: ` + - oneValueKeys.map((key) => `\`${f}.${method}(current_user.${key})\``).join(' / ') + `\`${f}.${method}(current_user.)\`` ); } } @@ -879,14 +897,9 @@ const HOLDING_VERDICT: Readonly str '`current_user` alone is the whole caller context object, used where one value or one set belongs', }; -/** The one-sentence rule each holding breaks, leading its hint. */ -const HOLDING_RULE: Readonly> = { - list: - '`==` / `!=` compare ONE value and a string method takes ONE string; a membership set is tested with `in` — ' + - '"one of these" is `x in set`, "none of these" is `!(x in set)`.', - 'one value': '`in` tests membership in a SET; one value is compared with `==` / `!=`.', - 'context object': '`current_user` is the caller context, not a value: name the key that holds what you mean.', -}; +// [#22161] The one-sentence rule each holding breaks led the hint until the +// hint became one line; it is the `rls-predicate-unenforceable` explanation's +// now, which `os explain` prints. /** * The verdict of the shared filter faces on this predicate's LOWERED filter — @@ -949,7 +962,7 @@ function nullComparandRewrites(filter: Record): string[] { rest.length === 0 ? `replace \`${written}\` with \`${f} == null\`` : `replace \`${written}\` with \`(${f} in [${rest.map(celLiteral).join(', ')}] || ${f} == null)\` ` + - `to keep matching a missing value, or drop the member: \`${f} in [${rest.map(celLiteral).join(', ')}]\``, + 'to keep matching a missing value', ); } else { rewrites.push( @@ -1178,11 +1191,13 @@ export function crossClassVerdict(comparisons: readonly CrossClassComparison[]): ); } -/** The prescription both rules share, ahead of their per-surface alternatives. */ -export const CROSS_CLASS_REMEDY = - `Compare a field only with a field of the same comparison class: ${CROSS_FIELD_CLASS_LISTING}. ` + - 'A file field and a formula field cannot be compared with another column at all. If the two columns do ' + - 'hold comparable values, one of them is declared with the wrong type: fix the declaration, not the predicate.'; +/* + * [#22161] The prescription both rules shared, ahead of their per-surface + * alternatives (`CROSS_CLASS_REMEDY`), listed every class with its types on + * the `fix:` line. The listing is the explanation's now — written out there + * and held to {@link CROSS_FIELD_CLASS_LISTING} by the rules' tests — and each + * rule's `fix:` line is one sentence naming its own alternatives. + */ /** * The engine's judge-only filter admission, as the caller hands it in @@ -1257,10 +1272,10 @@ function engineRefusal( /** The prescription for an engine refusal, keyed by the engine's `code` (never by its prose). */ function engineRefusalHint(code: string): string { if (code === 'FILTER_TOKEN_UNKNOWN' || code === 'FILTER_TOKEN_UNRESOLVED') { + // [#22161] One line; the `current_user` keys are the explanation's list. return ( - 'A `{…}` placeholder is saved-filter syntax, and an RLS predicate does not use it: the predicate ' + - `is CEL, and the caller's values are the \`current_user\` keys (${listNames(PRERESOLVED_USER_KEYS)}). ` + - 'Write `owner == current_user.id`, not `owner == \'{current_user_id}\'`.' + "Write the caller's value as a `current_user` key, never a `{…}` placeholder: `owner == current_user.id`, " + + "not `owner == '{current_user_id}'`" ); } if (code === 'INVALID_FIELD') { @@ -1270,9 +1285,8 @@ function engineRefusalHint(code: string): string { ); } return ( - 'Rewrite the comparison the engine names so it can run: a text operator (`startsWith` / `endsWith` / ' + - '`contains`) only on a field that holds a string, and a date or datetime field only against a value ' + - 'its storage can read. ' + PUSHDOWN_SUBSET + 'Rewrite the comparison the engine names: a text operator only on a field that holds a string, and a date ' + + 'or datetime field only against a value its storage can read' ); } @@ -1312,14 +1326,10 @@ function referenceFindings( where, path, message: `${account.message} ${droppedSentence(clause)}`, - hint: - `${account.detail} Point the predicate at a column the object really declares, or delete the ` + - `policy if the narrowing is gone — a policy naming a column that does not exist is not ` + - `protection either way: it is an outage in one position and an open door in the other. If the ` + - `field was RENAMED, this policy has been wrong since that rename — check WHICH way before you ` + - `judge the blast radius, because a negated or non-leading miss has been granting, not denying. ` + - `If the value was meant to live on another object, RLS cannot join to it (ADR-0055) — ` + - `denormalise it onto "${object}" (a formula/rollup field) and test that column instead.`, + // [#22161] One line: the verdict's "Did you mean" names the nearest + // declared column, so the object's whole field list (`account.detail`) + // and the rename / join reasoning are the explanation's. + hint: UNKNOWN_FIELD_FIX, }); } @@ -1356,9 +1366,7 @@ function referenceFindings( where, path, message: `RLS ${clause}: ${HOLDING_VERDICT[fault.holds](fault.variable)}, ${droppedClause(clause)}`, - hint: - `${HOLDING_RULE[fault.holds]} In this predicate: ` + - `${fault.sites.map((site) => typeFaultRewrite(fault, site)).join('; ')}.`, + hint: firstAndCount(fault.sites.map((site) => typeFaultRewrite(fault, site))), }); } @@ -1374,12 +1382,7 @@ function referenceFindings( // compiled policy filter, `using` and `check` alike, so the policy is // dropped on every request, exactly as a TYPE fault is. message: `RLS ${clause} lowers to a refused comparand (${firstSentence(faced)}), ${droppedClause(clause)}`, - hint: - rewrites.length > 0 - ? '`null` has no place inside a list or opposite an ordering operator — the platform refuses both in ' + - 'every filter it is sent, because no two backends agree on what they match. Test for no value with ' + - `\`== null\` and for a value with \`!= null\`: ${rewrites.join('; ')}.` - : 'Rewrite the comparand the check names inside the lowerable subset. ' + PUSHDOWN_SUBSET, + hint: rewrites.length > 0 ? firstAndCount(rewrites) : COMPARAND_REWRITE, }); } @@ -1395,11 +1398,8 @@ function referenceFindings( path, message: `RLS ${clause} ${listHoldingVerdict(listComparisons)}, ${REFUSED_COMPARISON[clause]}`, hint: - 'A field compared with a `json` or `multiple` field has no row-filter form: a row filter compares ' + - 'one value with one value, and cannot test membership in a list another column holds. Compare ' + - 'with a single-valued column, or with a literal or a `current_user` value — "one of these ' + - "values\" is `record.status in ['open', 'pending']` or `record.owner in current_user.org_user_ids` " + - '— or move the condition into a validation rule or a hook.', + 'Compare with a single-valued column, a literal or a `current_user` value ' + + '(`record.owner in current_user.org_user_ids`), or move the condition into a validation rule or a hook', }); } @@ -1416,9 +1416,8 @@ function referenceFindings( path, message: `RLS ${clause} ${crossClassVerdict(crossClass)}, ${REFUSED_COMPARISON[clause]}`, hint: - `${CROSS_CLASS_REMEDY} To keep the rule without a second column, compare with a literal or a ` + - '`current_user` value, test a file field with `!= null`, or store the value you mean in a field of the ' + - 'right type and compare that — or move the condition into a validation rule or a hook.', + 'Compare a field only with a field of its comparison class, or with a literal or a `current_user` value; ' + + 'if both columns do hold comparable values, fix the declaration', }); } @@ -1540,19 +1539,9 @@ export function validateRlsPredicateEnforceability( message: `RLS ${clause} is valid CEL but overruns the parse bound ${bound}${budget}${measured} ` + `(${overrun.summary}), ${droppedClause(clause)}`, - hint: - `There is no syntax or dialect error to correct here — the predicate is well-formed CEL and ` + - `is simply too large for ${bound}${budget}, so the fix is to make it smaller or to move the ` + - `work off the predicate. (1) Collapse a long \`field == a || field == b || …\` chain into a ` + - `single \`field in [a, b, …]\`, which is far fewer AST nodes (\`maxListElements\` is 64, so a ` + - `very large set needs option 2). (2) Pre-resolve the set into a membership key the runtime ` + - `exposes and test \`field in current_user.\` (ADR-0105 D11) — one comparison whatever ` + - `the set size. (3) Denormalise a repeated sub-expression onto this object as a ` + - `formula/rollup field and test that single column. (4) Split a TOP-LEVEL \`||\` across ` + - `several \`rowLevelSecurity\` policies: applicable policies are OR-ed, so that is ` + - `equivalent — but never split a top-level \`&&\` this way, which would WIDEN access rather ` + - `than preserve it. Logic genuinely this large is not a row filter: move it to a hook or ` + - `action body (\`ScriptBody { language: 'js' }\`, the L2 sandboxed surface).`, + // [#22161] One line; the four options in full, and why a top-level + // `&&` must never be split, are the explanation's. + hint: OVER_BUDGET_FIX, }); continue; } @@ -1583,15 +1572,7 @@ export function validateRlsPredicateEnforceability( where, path, message: `RLS ${clause} is not lowerable (${refusalVerdict(detail)}), ${droppedClause(clause)}`, - hint: - 'Rewrite the predicate inside the lowerable subset. ' + PUSHDOWN_SUBSET + ' Three traps in ' + - 'particular: (1) a function call — `size(record.tags) > 0`, `has(record.x)` — is correct in an ' + - 'object VALIDATION rule, which is INTERPRETED, and wrong here, where the predicate is ' + - 'COMPILED to a filter; write the null test as `field != null`. (2) A related-record path ' + - '(`record.account.region`) is a join, which the compiler refuses by design (ADR-0055) — ' + - 'denormalise the value onto this object (a formula/rollup field) and test that column. ' + - '(3) Arithmetic on a column (`amount * 2 > 100`) never lowers — precompute it into a field, ' + - 'or compare the column against the literal directly.', + hint: LOWERABLE_REWRITE, }); } }); diff --git a/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts index 7f300b99e81..4a5481fe021 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts @@ -23,6 +23,7 @@ import { SHARING_RULE_UNLOWERABLE_CONDITION, } from './validate-sharing-rule-enforceability.js'; import { runAuthoringRules } from './authoring-rules.js'; +import { CROSS_FIELD_CLASS_LISTING } from './validate-rls-predicate-enforceability.js'; import { explainRule } from './rule-explanations.js'; // [#22161] Each finding is one verdict sentence; the reasoning it used to @@ -30,7 +31,7 @@ import { explainRule } from './rule-explanations.js'; // fired, and the last case in this file holds each recorded verdict to one // line of at most 200 characters. Run the whole file: that case reads what the // cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateSharingRuleEnforceability: typeof validateSharingRuleEnforceabilityUnrecorded = (...args) => { const findings = validateSharingRuleEnforceabilityUnrecorded(...args); fired.push(...findings); @@ -108,7 +109,7 @@ describe('validateSharingRuleEnforceability — a field compared with a field of `condition compares \`record.${left} ${quoted} record.${right}\`, which no comparison class spans ` + `(${why}), so its criteria query is refused (INVALID_FILTER / 400) and it grants nothing`, ); - expect(finding.hint).toMatch(/^Compare a field only with a field of the same comparison class: /); + expect(finding.hint).toMatch(/^Compare a field only with a field of its comparison class, or with a literal/); }); } } @@ -182,4 +183,25 @@ describe('[#22161] one-line verdicts', () => { expect(text).toContain('so the rule IS seeded into `sys_sharing_rule`, but every criteria query it runs is refused'); expect(text).toContain('The rule is declared and grants nothing.'); }); + + // [#22161] Slice 11: the `fix:` line, one remedy sentence; what it stopped + // saying is `os explain sharing-rule-unlowerable-condition`. + 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 two condition ids this slice converted; the anchor ids keep their + // longer hints (a later slice's). + const converted = ['sharing-rule-unlowerable-condition', 'sharing-rule-runtime-variable-condition']; + for (const f of fired.filter((x) => converted.includes(x.rule))) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` lists the comparison classes the `fix:` line stopped listing, as the spec defines them', () => { + // The listing used to be the hint's, computed from the spec's class table; + // the explanation writes it out (it imports nothing), so it is held here + // to the rule's own computed listing. + const text = explainRule(SHARING_RULE_UNLOWERABLE_CONDITION)?.paragraphs.join('\n') ?? ''; + expect(text).toContain(`Compare a field only with a field of the same comparison class: ${CROSS_FIELD_CLASS_LISTING}.`); + }); }); diff --git a/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts index 7e6b42ed2a7..a7219129651 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts @@ -35,7 +35,7 @@ import { explainRule } from './rule-explanations.js'; // fired, and the last case in this file holds each recorded verdict to one // line of at most 200 characters. Run the whole file: that case reads what the // cases above fired. -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateSharingRuleEnforceability: typeof validateSharingRuleEnforceabilityUnrecorded = (...args) => { const findings = validateSharingRuleEnforceabilityUnrecorded(...args); fired.push(...findings); @@ -116,7 +116,7 @@ describe('validateSharingRuleEnforceability — a condition comparing a field wi `condition compares \`record.${left} ${quoted} record.${right}\`, where \`${field}\` holds a list or an ` + `object, ${CONSEQUENCE}`, ); - expect(findings[0].hint).toMatch(/^A field compared with a `json` or `multiple` field has no row-filter form/); + expect(findings[0].hint).toMatch(/^Compare with a single-valued column or a literal/); }); } } @@ -281,4 +281,17 @@ describe('[#22161] one-line verdicts', () => { expect(text).toContain('`SharingRuleService` reads a refused query as matching no record'); expect(text).toContain('at boot or on any later write'); }); + + // [#22161] Slice 11: the `fix:` line, one remedy sentence; what it stopped + // saying is `os explain sharing-rule-unlowerable-condition`. + 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 or an object'))).toBe(true); + // The two condition ids this slice converted; the anchor ids keep their + // longer hints (a later slice's). + const converted = ['sharing-rule-unlowerable-condition', 'sharing-rule-runtime-variable-condition']; + for (const f of fired.filter((x) => converted.includes(x.rule))) { + 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-sharing-rule-enforceability.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.test.ts index 93702127001..faa68b2b8f5 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.test.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.test.ts @@ -29,7 +29,7 @@ const SHARING_RULE_IDS: readonly string[] = [ SHARING_RULE_OBJECT_NOT_SHAREABLE, SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT, ]; -const fired: Array<{ rule: string; message: string }> = []; +const fired: Array<{ rule: string; message: string; hint: string }> = []; const validateSharingRuleEnforceability: typeof validateSharingRuleEnforceabilityUnrecorded = (...args) => { const findings = validateSharingRuleEnforceabilityUnrecorded(...args); fired.push(...findings); @@ -78,9 +78,10 @@ describe('validateSharingRuleEnforceability — the declared-but-never-read case // …and `os explain` says how. expect(explanationOf(SHARING_RULE_UNLOWERABLE_CONDITION)).toMatch(/the rule is SKIPPED/); expect(explanationOf(SHARING_RULE_UNLOWERABLE_CONDITION)).toMatch(/never written to `sys_sharing_rule`/); - // …and prescribe the fix that works on THIS surface. + // …and prescribe the fix that works on THIS surface: the `fix:` line in + // one line, and why `has()` is wrong here in the explanation (#22161). expect(findings[0].hint).toMatch(/record\.x != null/); - expect(findings[0].hint).toMatch(/INTERPRETED/); + expect(explanationOf(SHARING_RULE_UNLOWERABLE_CONDITION)).toMatch(/INTERPRETED/); }); it.each([ @@ -101,8 +102,10 @@ describe('validateSharingRuleEnforceability — the declared-but-never-read case path: 'sharingRules[0].condition', }); expect(findings[0].message).toMatch(/current_user\.id/); - // Points at the surface where `current_user.*` IS resolved. - expect(findings[0].hint).toMatch(/rowLevelSecurity\[\]\.using/); + // The `fix:` line names the route that holds on every sharing model; the + // explanation points at the surface where `current_user.*` IS resolved. + expect(findings[0].hint).toMatch(/`sharedWith`/); + expect(explanationOf(SHARING_RULE_RUNTIME_VARIABLE_CONDITION)).toMatch(/rowLevelSecurity\[\]\.using/); }); it('reports the parsed tier identically — the envelope `ExpressionInputSchema` produces', () => { @@ -463,9 +466,15 @@ describe('validateSharingRuleEnforceability — an anchor a gate WOULD consult s // security boundary. // // These legs pin the split. The failure they exist to catch is a REGRESSION TO -// ADVICE THAT CANNOT WORK, which is invisible to every other gate: the hint is +// ADVICE THAT CANNOT WORK, which is invisible to every other gate: the text is // prose, so nothing else in CI reads a word of it. -describe('SHARING_RULE_RUNTIME_VARIABLE_CONDITION — the hint qualifies its RLS remedy by sharing model', () => { +// +// [#22161] The split now lives in the id's `os explain` entry, and the hint is +// one remedy line that holds on every model. So the legs read the explanation, +// and the hint gets the guard that matters on a one-line `fix:`: it recommends +// no RLS at all — every RLS sentence anywhere must name the models it holds +// for, and the hint, which has no room to, names none. +describe('SHARING_RULE_RUNTIME_VARIABLE_CONDITION — the explanation qualifies its RLS remedy by sharing model', () => { const hintOf = (): string => { const findings = validateSharingRuleEnforceability( ruleWith("record.visibility == 'manager' && record.owner_id == current_user.id"), @@ -474,14 +483,12 @@ describe('SHARING_RULE_RUNTIME_VARIABLE_CONDITION — the hint qualifies its RLS expect(findings[0].rule).toBe(SHARING_RULE_RUNTIME_VARIABLE_CONDITION); return findings[0].hint; }; + const explanation = (): string => explanationOf(SHARING_RULE_RUNTIME_VARIABLE_CONDITION); - it('keeps the WHY verbatim — the most useful part of the diagnostic', () => { - // Byte-exact. This sentence explains why the condition is refused at all, - // and it was correct before this card and after it. - expect(hintOf()).toContain( - 'A criteria sharing rule is MATERIALISED: the seeder compiles ONE static `criteria_json` per ' + - 'rule and the evaluator writes `sys_record_share` rows from it, so there is no "current user" ' + - 'for the condition to read.', + it('keeps the WHY — the most useful part of the diagnostic — in the explanation', () => { + expect(explanation()).toContain( + 'A criteria sharing rule is MATERIALISED: the seeder compiles one static `criteria_json` per rule and the ' + + 'evaluator writes `sys_record_share` rows from it, so there is no "current user"', ); }); @@ -489,7 +496,7 @@ describe('SHARING_RULE_RUNTIME_VARIABLE_CONDITION — the hint qualifies its RLS // The load-bearing leg. `rowLevelSecurity[].using` may still be // recommended, but only inside a sentence that says WHERE it works; the // shipped defect was exactly this recommendation with no model attached. - const sentences = hintOf().split(/(?<=\.)\s+/); + const sentences = explanation().split(/(?<=\.)\s+/); const rlsSentences = sentences.filter((s) => s.includes('rowLevelSecurity[].using')); expect(rlsSentences.length).toBeGreaterThan(0); rlsSentences.forEach((s) => { @@ -497,79 +504,93 @@ describe('SHARING_RULE_RUNTIME_VARIABLE_CONDITION — the hint qualifies its RLS }); }); - it('states the `private` case: AND-composed layers mean RLS cannot widen', () => { + it('the one-line `fix:` recommends no RLS — it names the route that holds on every model', () => { const hint = hintOf(); - expect(hint).toMatch(/AND-composed/); - expect(hint).toMatch(/can only REMOVE rows/); + expect(hint).not.toMatch(/rowLevelSecurity|\bRLS\b/); + expect(hint).toMatch(/`sharedWith`/); + expect(hint).toMatch(/sharingModel/); + }); + + it('states the `private` case: AND-composed layers mean RLS cannot widen', () => { + const text = explanation(); + expect(text).toMatch(/AND-composed/); + expect(text).toMatch(/can only REMOVE rows/); // The consequence an author must be told, because it is silent. - expect(hint).toMatch(/lints clean/); - expect(hint).toMatch(/grants nothing/); + expect(text).toMatch(/lints clean/); + expect(text).toMatch(/grants nothing/); }); it('names the doors that DO widen a private object, and what they key on', () => { - const hint = hintOf(); + const text = explanation(); // Widen by OWNER (ADR-0057 D1 depth) … - expect(hint).toMatch(/readScope/); - expect(hint).toMatch(/owner_id IN/); + expect(text).toMatch(/readScope/); + expect(text).toMatch(/owner_id IN/); // … or by an explicit share row, which only a criteria rule writes. - expect(hint).toMatch(/sys_record_share/); + expect(text).toMatch(/sys_record_share/); // And the property that makes both unusable as a per-record predicate. - expect(hint).toMatch(/never on a property of the record/); + expect(text).toMatch(/never on a property of the record/); }); it('mentions `position` only as an over-broad-grant WARNING, never as the remedy', () => { // The neighbouring temptation is worse than doing nothing: `position` // shares every matched row with every holder of that position tenant-wide. - const hint = hintOf(); - expect(hint).toMatch(/position/); - expect(hint).toMatch(/over-broad grant/); - expect(hint).toMatch(/EVERY holder/); + const text = explanation(); + expect(text).toMatch(/position/); + expect(text).toMatch(/over-broad grant/); + expect(text).toMatch(/EVERY holder/); + expect(hintOf()).not.toMatch(/position/); }); it('names the open capability gap rather than inventing a mechanism', () => { - const hint = hintOf(); + const text = explanation(); // Described in WORDS, never as a tracker id: this string reaches authors // and generated surfaces, and none of them can resolve `#NNNN` // (`check:doc-authoring`, maintainer ruling 2026-08-12). The `#14103` // anchor lives in an adjacent source comment instead. - expect(hint).toMatch(/known gap in the platform/); - expect(hint).toMatch(/leave the grant unauthored rather than widening it/); - expect(hint).not.toMatch(/#\d{3,}/); + expect(text).toMatch(/known gap in the platform/); + expect(text).toMatch(/leave the grant unauthored rather than widening it/); + expect(text).not.toMatch(/#\d{3,}/); + expect(hintOf()).not.toMatch(/#\d{3,}/); }); }); -// The vocabulary leg. Without it the hint and its expectations above move -// together under a rename and nothing goes red — the failure mode the doctor -// tenancy-hint pin was written to close. Every model and depth the hint names -// is checked against the SPEC-owned enum that declares it. -describe('the hint names REAL sharing-model and depth vocabulary', () => { - const hint = (): string => { - const f = validateSharingRuleEnforceability(ruleWith('record.owner_id == current_user.id')); - return f[0].hint; - }; +// The vocabulary leg. Without it the explanation and its expectations above +// move together under a rename and nothing goes red — the failure mode the +// doctor tenancy-hint pin was written to close. Every model and depth the +// explanation names is checked against the SPEC-owned enum that declares it. +describe('the explanation names REAL sharing-model and depth vocabulary', () => { + const text = (): string => explanationOf(SHARING_RULE_RUNTIME_VARIABLE_CONDITION); - const MODELS_NAMED_BY_THE_HINT = ['private', 'public_read', 'public_read_write'] as const; - const DEPTHS_NAMED_BY_THE_HINT = ['own', 'own_and_reports', 'unit', 'unit_and_below', 'org'] as const; + const MODELS_NAMED_BY_THE_EXPLANATION = ['private', 'public_read', 'public_read_write'] as const; + const DEPTHS_NAMED_BY_THE_EXPLANATION = ['own', 'own_and_reports', 'unit', 'unit_and_below', 'org'] as const; it('every sharing model it names is an `OWDModel` value the spec still declares', () => { const declared = new Set(OWDModel.options); - MODELS_NAMED_BY_THE_HINT.forEach((m) => { + MODELS_NAMED_BY_THE_EXPLANATION.forEach((m) => { expect(declared.has(m)).toBe(true); - expect(hint()).toContain(`\`${m}\``); + expect(text()).toContain(`\`${m}\``); }); }); it('every depth scope it names is an `ObjectAccessScope` value the spec still declares', () => { const declared = new Set(ObjectAccessScopeSchema.options); - DEPTHS_NAMED_BY_THE_HINT.forEach((d) => { + DEPTHS_NAMED_BY_THE_EXPLANATION.forEach((d) => { expect(declared.has(d)).toBe(true); - expect(hint()).toContain(`\`${d}\``); + expect(text()).toContain(`\`${d}\``); }); }); it('`position` is a real `ShareRecipientType` — the warning is about a live trap', () => { expect(new Set(ShareRecipientType.options).has('position')).toBe(true); }); + + it('the one-line `fix:` names the `sharingModel` key the explanation splits on', () => { + // A one-line remedy cannot carry the vocabulary; it carries the pointer + // to it, and the key it points at is the spec's. + const f = validateSharingRuleEnforceability(ruleWith('record.owner_id == current_user.id')); + expect(f[0].hint).toContain('sharingModel'); + expect(text()).toContain('`sharingModel`'); + }); }); /** @@ -648,3 +669,64 @@ describe('[#22161] one-line verdicts — the four ids', () => { for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); }); }); + +// [#22161] Slice 11: the `fix:` line. The two condition ids' hints are one +// remedy sentence each; what they stopped saying — the lowerable subset and its +// two traps, the comparison classes with their types, which mechanism replaces +// a `current_user` read on which sharing model — is the explanation's. +// (`sharing-rule-object-not-shareable` and `-controlled-by-parent` keep their +// longer hints: a later slice's.) +describe('[#22161] one-line fixes — the two condition ids', () => { + const ONE_LINE_FIX_IDS: readonly string[] = [ + SHARING_RULE_UNLOWERABLE_CONDITION, + SHARING_RULE_RUNTIME_VARIABLE_CONDITION, + ]; + + 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) => ONE_LINE_FIX_IDS.includes(f.rule)); + // Coverage control: both ids fired, so the bound below cannot pass over an + // empty record. The unlowerable id's list-holding and cross-class arms are + // fired, and their `fix:` lines bounded, by this file's two siblings + // (`*.list-holding-field.test.ts`, `*.cross-class-field.test.ts`). + expect([...new Set(converted.map((f) => f.rule))].sort()).toEqual([...ONE_LINE_FIX_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('each arm names its remedy in one line', () => { + const hintFor = (condition: string): string => validateSharingRuleEnforceability(ruleWith(condition))[0].hint; + expect(hintFor("has(record.owner_id) && record.stage == 'closed_won'")).toBe( + 'Rewrite `condition` inside the lowerable subset: comparisons, `in`, `&&` `||` `!`, null tests and string ' + + 'methods over single-column `record.` paths; write `has(record.x)` as `record.x != null`', + ); + expect(hintFor('record.owner_id == current_user.id')).toBe( + "Keep only the record's own properties in `condition` (e.g. `record.stage == 'closed_won'`) and name the " + + "audience in `sharedWith`; what replaces `current_user` depends on the object's sharingModel", + ); + }); + + // What each `fix:` line stopped saying, which `os explain RULE_ID` now prints. + const MOVED_FROM_FIX: Record = { + [SHARING_RULE_UNLOWERABLE_CONDITION]: [ + 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`', + '`startsWith` / `endsWith` / `contains`', + 'denormalise the value onto this object (a formula/rollup field)', + 'cannot test membership in a list another column holds', + 'a number only with a number (`number`', + 'or test a file field with `!= null`', + ], + [SHARING_RULE_RUNTIME_VARIABLE_CONDITION]: [ + '`getReadFilter` = RLS AND controlled-by-parent AND sharing', + '`buildReadFilter` constrains `private` only', + 'ADR-0090 D1', + 'ADR-0057 D1 depth scopes', + 'ask for record-relative sharing recipients', + ], + }; + + it.each([...ONE_LINE_FIX_IDS])('`os explain %s` carries what its `fix:` line no longer says', (rule) => { + for (const fact of MOVED_FROM_FIX[rule]) expect(explanationOf(rule), `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-sharing-rule-enforceability.ts b/packages/lint/src/validate-sharing-rule-enforceability.ts index f3c366fe7d8..1fe9b86c5d0 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.ts @@ -99,9 +99,12 @@ * where the sharing layer imposes nothing on reads, and structurally the * WRONG one on `private`, where the sharing layer has already withheld the * row and an AND term can only withhold more (an RLS "widener" there lints - * clean and grants nothing). The hint carries that split rather than the - * unqualified "use RLS" it shipped with, and the tests pin the `private` - * half so it cannot regress to advice that cannot work. + * clean and grants nothing). The id's `os explain` entry carries that split + * rather than the unqualified "use RLS" the hint shipped with; the one-line + * hint names the route that holds on every model (keep the record's own + * properties, name the audience in `sharedWith`) and recommends no RLS at + * all, and the tests pin both, so neither can regress to advice that + * cannot work. * - `parse-error` → deliberately NOT reported here. CEL syntax is * `validateStackExpressions`' surface and it already gates this same field * (`stack.sharingRules[].condition`). Reporting it twice, in two @@ -183,7 +186,6 @@ import { compileCelToFilter } from '@objectstack/formula'; import { referenceCarrierOf } from '@objectstack/spec/data'; import { indexObjectGraph, recordsOf, type ObjectGraph } from './object-graph.js'; import { - CROSS_CLASS_REMEDY, crossClassComparisons, crossClassVerdict, listHoldingComparisons, @@ -250,10 +252,28 @@ function toCompilerInput(condition: unknown): string | { source?: string } | nul return null; } -const PUSHDOWN_SUBSET = - 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, ' + - 'and the string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column `record.` ' + - 'paths (ADR-0058 D2).'; +/* + * [#22161] The `fix:` lines. Each finding's `hint` is one remedy sentence of + * at most 200 characters; the subset it rewrites into, the comparison classes + * with the types each holds, and which mechanism replaces a `current_user` + * read on which sharing model are each id's `os explain` entry + * (`rule-explanations.ts`), held to their sources by this file's test. + */ + +/** Rewrite into the lowerable subset — the compiler's `unsupported` arm. */ +const LOWERABLE_REWRITE = + 'Rewrite `condition` inside the lowerable subset: comparisons, `in`, `&&` `||` `!`, null tests and string ' + + 'methods over single-column `record.` paths; write `has(record.x)` as `record.x != null`'; + +/** + * Keep the record's own properties, name the audience — the `unresolved-variable` + * arm. ⛔ It names no RLS: an RLS policy is the remedy on a `public_read` / + * `public_read_write` anchor only, and grants nothing on `private` (this file's + * header), so the model-qualified split is the explanation's, never this line's. + */ +const RUNTIME_VARIABLE_REMEDY = + "Keep only the record's own properties in `condition` (e.g. `record.stage == 'closed_won'`) and name the " + + "audience in `sharedWith`; what replaces `current_user` depends on the object's sharingModel"; /* * [#22161] The verdict clauses. Every finding of the four ids prints one @@ -291,10 +311,8 @@ function listHoldingFinding( path: at.path, message: `condition ${listHoldingVerdict(comparisons)}, ${CRITERIA_QUERY_REFUSED}`, hint: - 'A field compared with a `json` or `multiple` field has no row-filter form: a row filter compares one ' + - 'value with one value, and cannot test membership in a list another column holds. Compare with a ' + - "single-valued column, or with a literal — \"one of these values\" is `record.status in ['open', " + - "'pending']` — or keep the value the rule keys on in a single-valued field and compare with that.", + "Compare with a single-valued column or a literal (`record.status in ['open', 'pending']`), or keep the " + + 'value the rule keys on in a single-valued field and compare with that', }; } @@ -321,8 +339,8 @@ function crossClassFinding( path: at.path, message: `condition ${crossClassVerdict(comparisons)}, ${CRITERIA_QUERY_REFUSED}`, hint: - `${CROSS_CLASS_REMEDY} To keep the rule without a second column, compare with a literal — "one of these ` + - "values\" is `record.status in ['open', 'pending']` — or test a file field with `!= null`.", + 'Compare a field only with a field of its comparison class, or with a literal; if both columns do hold ' + + 'comparable values, one is declared with the wrong type: fix the declaration', }; } @@ -597,8 +615,10 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn if (result.reason === 'parse-error') return; if (result.reason === 'unresolved-variable') { - // The record-relative-recipient gap the hint's last sentence describes is - // tracked at #14103. The id lives HERE and not in the string: a runtime + // The record-relative-recipient gap the explanation's last paragraph + // describes (it was the hint's last sentence until the hint became one + // line, #22161) is tracked at #14103. The id lives HERE and not in the + // string: a runtime // message reaches authors and generated surfaces, and none of them can // resolve `#NNNN` (`check:doc-authoring`, maintainer ruling 2026-08-12). // The reader who CAN resolve it is reading this source. @@ -608,33 +628,7 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn where, path, message: `condition reads a runtime variable (${result.detail}), ${NEVER_SEEDED}`, - hint: - 'A criteria sharing rule is MATERIALISED: the seeder compiles ONE static `criteria_json` per ' + - 'rule and the evaluator writes `sys_record_share` rows from it, so there is no "current user" ' + - 'for the condition to read. WHICH mechanism replaces it depends on the anchor object\'s ' + - '`sharingModel`, because the security layers are AND-composed (`getReadFilter` = RLS AND ' + - 'controlled-by-parent AND sharing) and an AND term can only REMOVE rows — it can never add ' + - 'one back. On `public_read` / `public_read_write` the sharing layer imposes nothing on reads ' + - '(`buildReadFilter` constrains `private` only), so an RLS policy on a permission set ' + - '(`rowLevelSecurity[].using`, where `current_user.*` IS resolved) is the right mechanism: it ' + - 'NARROWS an open baseline, the direction AND can express. On `private` — including a CUSTOM ' + - 'object that declares no `sharingModel`, which resolves to `private` (ADR-0090 D1) — that ' + - 'same policy CANNOT work: the sharing layer has already withheld the row, and AND-ing an RLS ' + - 'predicate onto it can only withhold more, so the policy lints clean, passes every gate and ' + - 'grants nothing. The two doors that widen a `private` object for a non-admin principal both ' + - 'key on WHO OWNS the row or on an explicit share row, never on a property of the record: the ' + - 'ADR-0057 D1 depth scopes (`readScope` / `writeScope` on the object permission — `own` | ' + - '`own_and_reports` | `unit` | `unit_and_below` | `org`), which widen the owner-match to ' + - '`owner_id IN (…)`; and a `sys_record_share` row, which on this path only a criteria sharing ' + - 'rule writes — i.e. THIS rule. So keep this rule for the part of the predicate that is a ' + - 'property of the RECORD (e.g. `record.stage == \'closed_won\'`) and name the audience ' + - 'through `sharedWith`. If the audience is genuinely record-relative ("the owner\'s manager", ' + - '"this record\'s approver"), no mechanism expresses it today — `sharedWith` recipients ' + - 'resolve tenant-wide, so reaching for `position` shares every matched row with EVERY holder ' + - 'of that position (the skip-level, the manager two teams over): an over-broad grant, not the ' + - 'narrower one you meant. That is a known gap in the platform, not something a different ' + - 'spelling of this rule can close — leave the grant unauthored rather than widening it, and ' + - 'ask for record-relative sharing recipients.', + hint: RUNTIME_VARIABLE_REMEDY, }); return; } @@ -645,13 +639,7 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn where, path, message: `condition is not lowerable (${refusalVerdict(result.detail)}), ${NEVER_SEEDED}`, - hint: - 'Rewrite the predicate inside the lowerable subset. ' + PUSHDOWN_SUBSET + ' Two traps in ' + - 'particular: (1) `has(record.x)` is correct in an object VALIDATION rule, which is INTERPRETED, ' + - 'and wrong here, where the condition is COMPILED — write the null test as `record.x != null`; ' + - '(2) a related-record path (`record.account.region`) is a join, which the compiler refuses by ' + - 'design (ADR-0055) — denormalise the value onto this object (a formula/rollup field) and test ' + - 'that column, or share the related object instead.', + hint: LOWERABLE_REWRITE, }); }); diff --git a/packages/lint/src/validate-visibility-predicates.test.ts b/packages/lint/src/validate-visibility-predicates.test.ts index b8e1d8031e2..e03eaae039e 100644 --- a/packages/lint/src/validate-visibility-predicates.test.ts +++ b/packages/lint/src/validate-visibility-predicates.test.ts @@ -1522,8 +1522,13 @@ describe('visibility-predicate-over-budget (#7217)', () => { // ⛔ The defect itself: the dialect prescription must not reach this class. expect(findings[0].hint).not.toMatch(/bare CEL/); expect(findings[0].hint).not.toContain('`===`'); - expect(findings[0].hint).toContain('SIZE fault, not a dialect mistake'); + // [#22161] One remedy line; that it is a SIZE fault is the explanation's. + expect(findings[0].hint).toMatch(/^Make the predicate smaller/); + expect(explainRule(VISIBILITY_PREDICATE_OVER_BUDGET)!.paragraphs.join('\n')).toContain( + 'SIZE fault, not a dialect mistake', + ); expect(findings[0].hint).toContain("`record.f in ['a', 'b', …]`"); + expect(findings[0].hint.length).toBeLessThanOrEqual(200); }); it('the SAME predicate under budget is clean — paired so it cannot pass vacuously', () => { @@ -1546,7 +1551,8 @@ describe('visibility-predicate-over-budget (#7217)', () => { const findings = overBudgetFindings(formStack(source as string)); expect(findings).toHaveLength(1); expect(findings[0].message).toContain(`\`${limit}\` budget (platform limit ${value})`); - expect(findings[0].hint).toContain('SIZE fault, not a dialect mistake'); + expect(findings[0].hint).toMatch(/^Make the predicate smaller/); + expect(findings[0].hint.length).toBeLessThanOrEqual(200); }); it('a genuine dialect fault keeps the #6253 id, message and hint verbatim', () => { diff --git a/packages/lint/src/validate-visibility-predicates.ts b/packages/lint/src/validate-visibility-predicates.ts index 4fa26e43146..45490d08baf 100644 --- a/packages/lint/src/validate-visibility-predicates.ts +++ b/packages/lint/src/validate-visibility-predicates.ts @@ -1071,15 +1071,12 @@ function checkElement( `visibility predicate is syntactically valid CEL but overruns ${bound}` + `${refusal.overrun.limit && refusal.overrun.limitValue !== null ? '' : ` (${refusal.overrun.summary})`}, ` + NEVER_EVALUATES, + // [#22161] One remedy line; that it is a SIZE fault and not a dialect + // mistake, and why logic this large belongs on the record, are + // `os explain visibility-predicate-over-budget`. hint: - `There is no syntax or dialect error to correct here — this is a SIZE fault, not a dialect ` + - `mistake, so re-spelling the predicate will not fix it. Make it smaller, or move the work ` + - `off the predicate: (1) collapse a long \`${root}.f == 'a' || ${root}.f == 'b' || …\` chain ` + - `into a single \`${root}.f in ['a', 'b', …]\`, which is far fewer AST nodes ` + - `(\`maxListElements\` is 64, so a very large set needs option 2); (2) precompute the heavy ` + - `part into a formula/rollup field on the object and test that one field instead. Logic ` + - `genuinely this large is not element visibility — compute it once on the record rather ` + - `than re-deriving it in every predicate that needs it.`, + `Make the predicate smaller: collapse an \`||\` chain into \`${root}.f in ['a', 'b', …]\`, or precompute ` + + 'the heavy part into a formula/rollup field on the object and test that one field', }); } if (source && refusal?.kind === 'syntax') { diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 206fc815f6e..40ce97b6408 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -80,8 +80,10 @@ export const ApproverType = z.enum([ // // ⛔ It POINTS at the remedy, it does not restate it. The remedy's single // authoritative copy is `MANAGER_ONLY_REMEDY` / `MANAGER_ONLY_ROUTES` in - // `packages/lint/src/validate-approval-approvers.ts`, mirrored for readers by - // the Approvals guide; a third copy here would be one more line to keep in + // `packages/lint/src/rule-explanations.ts` — the explanation + // `os explain approval-approvers-may-resolve-empty` prints, since the + // finding's `fix:` line became one sentence (#22161) — mirrored for readers + // by the Approvals guide; a third copy here would be one more line to keep in // step, and a pointer cannot drift into disagreement with what it points at. // ⛔ It said "that column has no product write surface" until the write // surface landed — the exact staleness the pointer discipline above exists to @@ -98,11 +100,12 @@ export const ApproverType = z.enum([ .describe( 'Approval step approver type. `manager` is resolved from the directory rather than configured ' + "here: it reads the submitter's `sys_user.manager_id` at runtime, and that column is not a " + - 'profile field an author edits, so until an operator populates it through one of the routes ' + - '`os lint` names, a manager step resolves to nobody and the request waits. `os lint` reports ' + - 'that at authoring time as `approval-approvers-may-resolve-empty` and carries the graded ' + - 'population routes and the full remedy, the node-level `onEmptyApprovers` fallback included; ' + - 'the Approvals guide states the same remedy in prose.', + 'profile field an author edits, so until an operator populates it, a manager step resolves to ' + + "nobody and the request falls to the node's `onEmptyApprovers` policy. `os lint` reports that at " + + 'authoring time as `approval-approvers-may-resolve-empty`, and ' + + '`os explain approval-approvers-may-resolve-empty` carries the graded population routes and the ' + + 'full remedy, the node-level `onEmptyApprovers` fallback included; the Approvals guide states the ' + + 'same remedy in prose.', ); export type ApproverType = z.input;