From 7553d500a4640eacc935593ec959cbad83f77c7e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:10:13 +0000 Subject: [PATCH 1/8] fix(lint): one-line fix lines for 15 author-time rule ids; the enumerations move to os explain WIP: hint text and RULE_EXPLANATIONS data only; tests updated text-only. Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- packages/lint/src/rule-explanations.ts | 191 ++++++++++++++++-- .../src/validate-approval-approvers.test.ts | 126 ++++++++---- .../lint/src/validate-approval-approvers.ts | 79 +++----- .../lint/src/validate-component-types.test.ts | 56 ++++- packages/lint/src/validate-component-types.ts | 40 +++- .../src/validate-empty-combinators.test.ts | 35 +++- .../lint/src/validate-empty-combinators.ts | 18 +- .../lint/src/validate-field-consumers.test.ts | 38 +++- packages/lint/src/validate-field-consumers.ts | 17 +- .../validate-flow-trigger-readiness.test.ts | 27 ++- .../src/validate-flow-trigger-readiness.ts | 12 +- .../src/validate-react-page-props.test.ts | 43 +++- .../lint/src/validate-react-page-props.ts | 20 +- .../src/validate-readonly-hook-writes.test.ts | 33 ++- .../lint/src/validate-readonly-hook-writes.ts | 23 +-- ...alidate-retired-permission-residue.test.ts | 48 ++++- .../validate-retired-permission-residue.ts | 30 ++- ...e-enforceability.cross-class-field.test.ts | 40 +++- ...dicate-enforceability.engine-judge.test.ts | 31 ++- ...-enforceability.list-holding-field.test.ts | 19 +- ...idate-rls-predicate-enforceability.test.ts | 135 ++++++++++--- .../validate-rls-predicate-enforceability.ts | 175 +++++++--------- ...e-enforceability.cross-class-field.test.ts | 23 ++- ...-enforceability.list-holding-field.test.ts | 14 +- ...lidate-sharing-rule-enforceability.test.ts | 178 +++++++++++----- .../validate-sharing-rule-enforceability.ts | 88 ++++---- .../validate-visibility-predicates.test.ts | 10 +- .../src/validate-visibility-predicates.ts | 13 +- 28 files changed, 1152 insertions(+), 410 deletions(-) diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index 73c9bf5bb1a..92b2bdd1b42 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, ], }; @@ -1670,7 +1781,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.', ], }; @@ -1994,10 +2106,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.', ], @@ -2035,6 +2150,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 a slot nobody can act on, 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', @@ -2045,7 +2193,7 @@ const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY_EXPLANATION: RuleExplanation = { '`pending_approvers` slate: no user can act, so it waits forever, and under the default ' + '`lockRecord: true` the record stays locked with it, with no in-product recovery. 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 is the same dead end 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 ' + @@ -2060,6 +2208,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 for the node to open on when the ' + + 'groups come back empty.', + MANAGER_ONLY_REMEDY, + MANAGER_ONLY_ROUTES, + 'For either arm, a request that still lands empty is recoverable only by a platform/tenant admin ' + + 'override, which may act on any pending request so that one nobody in its slate can decide never ' + + 'stays stuck.', ], }; @@ -2459,6 +2616,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, ], }; @@ -3153,10 +3317,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 9fa294e12ed..248bf864c7f 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,39 @@ 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 declare onEmptyApprovers: 'fallback' with a fallbackApprovers list", + ); + }); + + 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', + 'recoverable only by a platform/tenant admin override', + 'which may act on any pending request', + '/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); + }); +}); diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 19f0419c54f..b630ef706b5 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,32 @@ 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. What + * recovers a request that lands empty anyway 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 declare onEmptyApprovers: 'fallback' with a fallbackApprovers list"; export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; @@ -549,13 +546,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin `every approver on this node routes to a group (position/team/department), so if ` + `none is staffed the request opens on an empty slate and waits forever` + (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 for the node to open on ` + - `when the groups come back empty. A request that still lands empty is recoverable ` + - `only by a platform/tenant admin override, which may act on any pending request so that ` + - `one nobody in its slate can decide never stays stuck.`, + hint: GROUP_ONLY_FIX, }); } @@ -567,7 +558,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. @@ -601,11 +592,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 an empty slate and waits forever` + (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 is recoverable only by a platform/tenant admin override.`, + 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 94be00fdba1..88dbe305a88 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..580baa8d314 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,22 @@ 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); + for (const f of fired) { + expect(f.hint, f.rule).not.toContain('\n'); + expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` 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..c10dab7205c 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,14 @@ 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); + for (const f of fired) { + 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') { From 7bffceea28abd90b6d8daff4b47b74895e623e8c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:25:28 +0000 Subject: [PATCH 2/8] test(lint): scope the sharing siblings' fix-line bound to the two converted ids Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- ...ate-sharing-rule-enforceability.cross-class-field.test.ts | 5 ++++- ...te-sharing-rule-enforceability.list-holding-field.test.ts | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) 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 580baa8d314..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 @@ -188,7 +188,10 @@ describe('[#22161] one-line verdicts', () => { // 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); - for (const f of fired) { + // 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.list-holding-field.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts index c10dab7205c..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 @@ -286,7 +286,10 @@ describe('[#22161] one-line verdicts', () => { // 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); - for (const f of fired) { + // 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); } From d272244924320ece0015969693841f69cf9a3614 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:28:10 +0000 Subject: [PATCH 3/8] chore(changeset): @objectstack/lint patch for the one-line fix lines Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- .../22161-lint-slice-11-one-line-fix.md | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 .changeset/22161-lint-slice-11-one-line-fix.md 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..6e5582fea95 --- /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. Before, the longest of each ran from 111 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,159 → 198, its group arm 463 → 195); + - 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); + - fields: `field-no-consumers` (629 → at most 199 on the showcase app's `os validate`). +- **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]… (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,159 / 463 → 198 / 195); + - 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. From b4cf329adaceede37db8dbe5ddbb37076ba4464c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 17:18:12 +0000 Subject: [PATCH 4/8] chore(changeset): state hint lengths as measured, and the showcase carrier example verbatim Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- .changeset/22161-lint-slice-11-one-line-fix.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.changeset/22161-lint-slice-11-one-line-fix.md b/.changeset/22161-lint-slice-11-one-line-fix.md index 6e5582fea95..9be78a17190 100644 --- a/.changeset/22161-lint-slice-11-one-line-fix.md +++ b/.changeset/22161-lint-slice-11-one-line-fix.md @@ -6,7 +6,7 @@ fix(lint): 15 author-time rule ids print a `fix:` line of one remedy sentence, a 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. Before, the longest of each ran from 111 to 2,280 characters. The ids: +- **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,159 → 198, its group arm 463 → 195); - RLS predicates: `rls-predicate-unenforceable` (1,056 → 191), `rls-predicate-over-budget` (1,044 → 197) and `rls-predicate-unknown-field` (766 → 154); @@ -15,10 +15,10 @@ Clause-②: no - 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); - - fields: `field-no-consumers` (629 → at most 199 on the showcase app's `os validate`). + - 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]… (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. +- **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,159 / 463 → 198 / 195); From a7cacc2c7bcd72f0591119801c3217ea3f76395d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 18:03:42 +0000 Subject: [PATCH 5/8] test(lint): hold approval-approvers-may-resolve-empty's one-line fixes and explanation to #22824's administrators' slate Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- .../src/validate-approval-approvers.test.ts | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 01a8e076b53..3739cb3a5d3 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -820,7 +820,7 @@ describe('[#22161] one-line fixes — approval-approvers-may-resolve-empty', () 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 declare onEmptyApprovers: 'fallback' with a fallbackApprovers list", + "value: 'owner' }), or name who decides in fallbackApprovers with onEmptyApprovers: 'fallback'", ); }); @@ -828,11 +828,31 @@ describe('[#22161] one-line fixes — approval-approvers-may-resolve-empty', () const text = explainRule(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY)!.paragraphs.join('\n'); for (const fact of [ 'make sure at least one target is always staffed', - 'recoverable only by a platform/tenant admin override', - 'which may act on any pending request', + // #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"); + }); +}); From 2cf8cf4832a378db2c553c0bcde942fb7841dce6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 18:04:39 +0000 Subject: [PATCH 6/8] docs(spec): ApproverType's description points at os explain approval-approvers-may-resolve-empty Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- .changeset/22161-spec-approver-type-describe.md | 10 ++++++++++ packages/spec/src/automation/approval.zod.ts | 17 ++++++++++------- 2 files changed, 20 insertions(+), 7 deletions(-) create mode 100644 .changeset/22161-spec-approver-type-describe.md diff --git a/.changeset/22161-spec-approver-type-describe.md b/.changeset/22161-spec-approver-type-describe.md new file mode 100644 index 00000000000..aa9f6c94f92 --- /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 reads that the request falls to the node's `onEmptyApprovers` policy, which by default opens it on the organization's administrators. +- **Unchanged:** the `ApproverType` values, the schema's shape, and what it accepts or refuses. Only the description text moves. 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; From 70b179f819ca8efd2b55b8ea6b264bf8772fd003 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 18:17:21 +0000 Subject: [PATCH 7/8] docs(spec): regenerate the approval reference page for ApproverType's re-pointed description Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- content/docs/references/automation/approval.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 From 6e2c2f068ce1cbd146cecabcfee138e04b11a4ec Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 19:10:58 +0000 Subject: [PATCH 8/8] docs(changeset): restate the approval rule's fix-line figures against main after the administrators' slate landed The slice changeset's FROM figures for approval-approvers-may-resolve-empty were measured before the merge that brought in the administrators' slate: main's hints are now 2,243 (manager arm) and 440 (group arm) characters, and the group arm's one-line fix is 200 characters after it was restated in that verdict's words. The spec changeset now says plainly that the describe's "the request waits" clause names the onEmptyApprovers policy, and that the default policy opens the request on the organization's administrators. Claude-Session: https://claude.ai/code/session_01KNKBCRDJCu5tGy3TEbvtrF Co-authored-by: Claude --- .changeset/22161-lint-slice-11-one-line-fix.md | 4 ++-- .changeset/22161-spec-approver-type-describe.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/22161-lint-slice-11-one-line-fix.md b/.changeset/22161-lint-slice-11-one-line-fix.md index 9be78a17190..529a3187b6d 100644 --- a/.changeset/22161-lint-slice-11-one-line-fix.md +++ b/.changeset/22161-lint-slice-11-one-line-fix.md @@ -8,7 +8,7 @@ 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,159 → 198, its group arm 463 → 195); + - 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`); @@ -21,7 +21,7 @@ Clause-②: no - **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,159 / 463 → 198 / 195); + - 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). diff --git a/.changeset/22161-spec-approver-type-describe.md b/.changeset/22161-spec-approver-type-describe.md index aa9f6c94f92..8cad97d7446 100644 --- a/.changeset/22161-spec-approver-type-describe.md +++ b/.changeset/22161-spec-approver-type-describe.md @@ -6,5 +6,5 @@ docs(spec): `ApproverType`'s description points at `os explain approval-approver 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 reads that the request falls to the node's `onEmptyApprovers` policy, which by default opens it on the organization's administrators. +- **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.