fix(spec): action-param rejection names the built-in a "differs by one underscore" key meant (#5622)

.changeset/action-param-builtin-near-miss-hint.md

@@ -0,0 +1,39 @@
+---
+"@objectstack/spec": patch
+---
+
+fix(spec): action-param rejection names the built-in a "differs by one underscore" key meant
+
+`validateActionParams` (ADR-0104 D2) rejected every undeclared key with the
+same sentence — `Unknown action param "selectedIds" — not declared on this
+action` — including keys one leading underscore away from a built-in
+(`ACTION_PARAM_BUILTIN_KEYS`: `recordId` / `objectName` / `_selectedIds`).
+That sentence is true, and its only actionable reading is false: the reader's
+next step is to declare the key on the action, and a built-in is precisely the
+key that **cannot** be declared. #5568's reporter walked that road to its end
+on `params.selectedIds`, concluded that REST carried no legal shape for a bulk
+selection at all, and opened a platform issue — while `params._selectedIds`
+was live the whole time.
+
+The `unknown_field` message now appends a near-miss hint when `'_' + key` or
+`key` minus its leading underscore is in the allowed built-in set:
+
+```
+Unknown action param "selectedIds" — not declared on this action. Did you mean
+the built-in "_selectedIds"? Built-in params are never declared on an action —
+an aggregate bulk dispatch (`execution: 'aggregate'`) injects every selected
+record id under it, and a handler reads `ctx.params._selectedIds`.
+```
+
+The origin sentence is per built-in, because the three have three different
+producers: `recordId` / `objectName` are merged into the bag server-side by the
+dispatcher, `_selectedIds` arrives from the renderer's aggregate bulk dispatch.
+A key reached through a custom `builtinKeys` override gets the generic
+"the dispatcher supplies it".
+
+**Message copy only — the verdict does not move.** The key is rejected before
+and after, the accepted set is unchanged, and an unknown key that is *not* a
+near-miss keeps today's message byte for byte (the match is one leading
+underscore, not a similarity score). This is not a second acceptance channel
+for `selectedIds`: the contract still has exactly one spelling,
+`params._selectedIds`.

packages/spec/src/ui/action-params.test.ts

@@ -93,6 +93,145 @@ describe('validateActionParams (ADR-0104 D2)', () => {
   });
 });
 
+/**
+ * [#5622] A rejected key one leading underscore away from a built-in NAMES
+ * that built-in. Message copy only — the verdict does not move.
+ *
+ * ## What the cost was, and why a message is the whole fix
+ *
+ * `Unknown action param "selectedIds" — not declared on this action` is a true
+ * sentence whose only actionable reading is false: the reader's next step is
+ * to declare `selectedIds` on the action, and `_selectedIds` is precisely the
+ * key that CANNOT be declared — the aggregate bulk dispatch injects it
+ * (objectui#3139). #5568's reporter walked that road to its end, concluded
+ * from the dead end that REST carried no legal shape for a selection at all,
+ * and opened a platform issue; `params._selectedIds` had been live the whole
+ * time. The hint ends that at the first call.
+ *
+ * ## The pins below assert the MESSAGE, not the failure
+ *
+ * "Validation failed" is one bit and this defect has two — the key is rejected
+ * both before and after this change, so a test asserting only rejection is
+ * green on the unfixed validator. Every case therefore pins `code` AND the
+ * message content, and the negative case pins the message BYTE FOR BYTE.
+ *
+ * ## Reverse verification — directions predicted BEFORE running
+ *
+ *  - Make `builtinNearMissHint` return `''` unconditionally (i.e. restore
+ *    today's behaviour) → the three hint cases and the accuracy case go RED on
+ *    their message assertions; every `codes(...)` assertion in this file stays
+ *    green, which is the point: the verdict never moved.
+ *  - ⚠️ HONEST NOTE — "an ordinary typo keeps today's message" is a COMPANION,
+ *    not a pin of this change (the #5722 distinction). It asserts the message
+ *    the UNFIXED validator already produced, so it is green on both sides of
+ *    the revert by construction. It is kept because it is the assertion that
+ *    goes red if the hint ever starts firing on distant keys — a fuzzy matcher
+ *    creeping in later is exactly the regression it guards, and that one this
+ *    file cannot otherwise see.
+ *  - Widen the match to fuzzy/Levenshtein → the two "no hint" cases go RED.
+ */
+describe('#5622 — near-miss built-in hint on unknown_field', () => {
+  const declaresFormat: ResolvedActionParam[] = [{ name: 'format', type: 'text' }];
+
+  it('names `_selectedIds` when the caller sent `selectedIds` (the #5568 road)', () => {
+    const issues = validateActionParams(declaresFormat, { format: 'png', selectedIds: ['dev_1', 'dev_2'] });
+
+    expect(issues).toHaveLength(1);
+    expect(issues[0].code).toBe('unknown_field');
+    expect(issues[0].param).toBe('selectedIds');
+    expect(issues[0].message).toContain('Unknown action param "selectedIds" — not declared on this action');
+    expect(issues[0].message).toContain('Did you mean the built-in "_selectedIds"?');
+    // The mechanism half — what makes the hint actionable rather than a
+    // spelling suggestion: it says the key is never declared, and where the
+    // value comes from instead.
+    expect(issues[0].message).toContain('Built-in params are never declared on an action');
+    expect(issues[0].message).toContain('aggregate bulk dispatch');
+    expect(issues[0].message).toContain('ctx.params._selectedIds');
+  });
+
+  it('names `recordId` when the caller sent `_recordId` (the reverse direction)', () => {
+    const issues = validateActionParams(declaresFormat, { format: 'png', _recordId: 'rec_1' });
+
+    expect(issues).toHaveLength(1);
+    expect(issues[0].code).toBe('unknown_field');
+    expect(issues[0].param).toBe('_recordId');
+    expect(issues[0].message).toContain('Did you mean the built-in "recordId"?');
+    expect(issues[0].message).toContain('ctx.params.recordId');
+  });
+
+  it('gives each of the THREE built-ins its own true origin sentence', () => {
+    // The issue proposed one sentence — "injected by an aggregate bulk
+    // dispatch" — which is true of `_selectedIds` and false of the other two:
+    // those are merged in SERVER-side by the dispatcher. Wrong-mechanism copy
+    // is worse than none, so the sentence is per-key.
+    const messageFor = (key: string) => {
+      const issues = validateActionParams(declaresFormat, { format: 'png', [key]: 'v' });
+      expect(issues).toHaveLength(1);
+      return issues[0].message;
+    };
+
+    expect(messageFor('_recordId')).toContain('the dispatcher merges it in from the record-scoped route');
+    expect(messageFor('_objectName')).toContain('the dispatcher merges in the name of the object the action dispatched on');
+    expect(messageFor('selectedIds')).toContain('injects every selected record id under it');
+
+    // ...and no message claims another key's mechanism.
+    expect(messageFor('_recordId')).not.toContain('aggregate bulk dispatch');
+    expect(messageFor('selectedIds')).not.toContain('record-scoped route');
+  });
+
+  it('leaves an ordinary unknown key\'s message EXACTLY as it is today', () => {
+    const issues = validateActionParams(declaresFormat, { format: 'png', bogus: 1 });
+
+    expect(issues).toHaveLength(1);
+    expect(issues[0].code).toBe('unknown_field');
+    // Byte for byte — a hint that leaks onto keys nowhere near a built-in is
+    // noise, and noise is how a reader learns to skip the line that mattered.
+    expect(issues[0].message).toBe('Unknown action param "bogus" — not declared on this action');
+    expect(issues[0].message).not.toContain('Did you mean');
+  });
+
+  it('does NOT fire on keys that are merely NEARBY — the match is one leading underscore, not a similarity score', () => {
+    for (const key of ['selected_ids', 'selectedIDs', 'selectedId', 'recordID', 'record_id', 'objectname']) {
+      const issues = validateActionParams(declaresFormat, { format: 'png', [key]: 'v' });
+      expect(issues).toHaveLength(1);
+      expect(issues[0].code).toBe('unknown_field');
+      expect(issues[0].message).toBe(`Unknown action param "${key}" — not declared on this action`);
+    }
+  });
+
+  it('hints a custom `builtinKeys` entry with the GENERIC origin, never another key\'s mechanism', () => {
+    // The override's members are built-ins by the option's own definition, so
+    // the "never declared" half holds; their producer is unknown to this
+    // module, so the sentence claims nothing more than that.
+    const issues = validateActionParams(
+      declaresFormat,
+      { format: 'png', _ctxToken: 'z' },
+      { builtinKeys: ['ctxToken'] },
+    );
+
+    expect(issues).toHaveLength(1);
+    expect(issues[0].code).toBe('unknown_field');
+    expect(issues[0].message).toContain('Did you mean the built-in "ctxToken"?');
+    expect(issues[0].message).toContain('the dispatcher supplies it.');
+    expect(issues[0].message).not.toContain('ctx.params');
+  });
+
+  it('moves the ACCEPTED SET by nothing — the near-miss is still rejected, the built-in still accepted', () => {
+    // The ruling that scopes this change: a hint, not a second channel. If
+    // `selectedIds` ever starts being accepted, the platform grows a synonym
+    // for a contract that has exactly one spelling.
+    expect(validateActionParams(declaresFormat, { format: 'png', _selectedIds: ['a', 'b'] })).toEqual([]);
+    expect(codes(validateActionParams(declaresFormat, { format: 'png', selectedIds: ['a', 'b'] })))
+      .toEqual(['unknown_field']);
+
+    // And the built-ins themselves are still allowed under their own names —
+    // the hint reads the same `allow` set the verdict does, so a bug there
+    // would show up as a built-in being flagged.
+    expect(validateActionParams(declaresFormat, { format: 'png', recordId: 'r1', objectName: 'o' })).toEqual([]);
+    expect(ACTION_PARAM_BUILTIN_KEYS).toEqual(['recordId', 'objectName', '_selectedIds']);
+  });
+});
+
 /**
  * [#5779] `ActionSession` gains the canonical `positions` key and demotes
  * `roles` to a deprecated alias — the SPEC half of #5613 phase 2, under the

packages/spec/src/ui/action-params.zod.ts

@@ -78,13 +78,78 @@ export interface ActionParamIssue {
  * record id in this key so the handler can produce a single aggregate
  * artifact (zip of QR codes, merged PDF…). Server handlers read it from
  * `ctx.params._selectedIds`; it is never authored as a declared param.
+ *
+ * Rejecting a caller's near-miss spelling of one of these (`selectedIds` for
+ * `_selectedIds`) names the built-in in the rejection message — see
+ * {@link BUILTIN_PARAM_ORIGINS}.
  */
 export const ACTION_PARAM_BUILTIN_KEYS: readonly string[] = ['recordId', 'objectName', '_selectedIds'];
 
+/**
+ * Where each built-in key actually comes from, one true sentence each — the
+ * tail of the near-miss hint below, NOT a second contract. Message copy only:
+ * nothing reads it to decide anything.
+ *
+ * Per-key rather than one generic sentence because the three built-ins have
+ * three different producers, and a hint that explains the wrong one is worse
+ * than no hint: `recordId` / `objectName` are merged in server-side by the
+ * dispatcher (`params: { ...reqParams, recordId, objectName }` —
+ * `runtime/src/domains/actions.ts` and `action-execution.ts`'s
+ * `invokeBusinessAction`), while `_selectedIds` arrives from the CLIENT, in
+ * the request's own `params`, put there by the renderer's aggregate bulk
+ * dispatch. Telling a caller to "send `params.recordId`" would be actively
+ * wrong — the dispatcher's spread overwrites whatever the bag carried under
+ * that key, with `undefined` when the route and body carry no record id.
+ *
+ * A key reached through a custom `builtinKeys` override has no entry and gets
+ * {@link GENERIC_BUILTIN_ORIGIN}, which is true of every member of that set by
+ * the option's own definition (keys the dispatcher merges in, never authored).
+ */
+const BUILTIN_PARAM_ORIGINS: ReadonlyMap<string, string> = new Map([
+  ['recordId', 'the dispatcher merges it in from the record-scoped route (or the request body\'s top-level `recordId`), and a handler reads `ctx.params.recordId`.'],
+  ['objectName', 'the dispatcher merges in the name of the object the action dispatched on, and a handler reads `ctx.params.objectName`.'],
+  ['_selectedIds', 'an aggregate bulk dispatch (`execution: \'aggregate\'`) injects every selected record id under it, and a handler reads `ctx.params._selectedIds`.'],
+]);
+
+/** Fallback origin sentence for a built-in supplied via `opts.builtinKeys`. */
+const GENERIC_BUILTIN_ORIGIN = 'the dispatcher supplies it.';
+
 function isPresent(v: unknown): boolean {
   return v !== undefined && v !== null && !(typeof v === 'string' && v.trim() === '');
 }
 
+/**
+ * The tail appended to an `unknown_field` message when the rejected key is one
+ * leading underscore away from a built-in — `''` when it is not (an ordinary
+ * typo keeps today's message, byte for byte).
+ *
+ * The rejection VERDICT is unchanged in both directions: the key is refused
+ * before and after, and the accepted set does not move. What changes is that
+ * "not declared on this action" no longer sends the reader down the one road
+ * that cannot work — declaring the built-in as a param, which the platform
+ * refuses by construction. #5568's reporter spent that road's full length on
+ * `params.selectedIds` and concluded from the silence that REST had no legal
+ * shape carrying a selection, when `params._selectedIds` was live the whole
+ * time.
+ *
+ * Bidirectional, and exactly one byte wide: `selectedIds` → `_selectedIds`
+ * and `_recordId` → `recordId`. Anything further away gets no hint, because a
+ * guess that is merely nearby ("did you mean X?" for an unrelated key) trains
+ * readers to ignore the line. Candidate order is fixed, so one rejected key
+ * always yields one message (#5240).
+ *
+ * The `key.replace(...)` candidate collapses to `key` itself for a key with no
+ * leading underscore. That can never produce a hint: the caller has already
+ * skipped every key `allow` holds, so `allow.has(key)` is false here by the
+ * loop's own guard — no identity check needed to exclude it.
+ */
+function builtinNearMissHint(key: string, allow: ReadonlySet<string>): string {
+  const near = [`_${key}`, key.replace(/^_/, '')].find((candidate) => allow.has(candidate));
+  if (!near) return '';
+  const origin = BUILTIN_PARAM_ORIGINS.get(near) ?? GENERIC_BUILTIN_ORIGIN;
+  return `. Did you mean the built-in "${near}"? Built-in params are never declared on an action — ${origin}`;
+}
+
 /**
  * Validate a params bag against an action's resolved param declarations.
  * Returns the list of issues (empty = conformant). Does NOT throw — this is a
@@ -96,6 +161,11 @@ function isPresent(v: unknown): boolean {
  * `valueSchemaFor`, so option membership / `multiple` arrays / reference-id
  * shape all ride the one contract), and unknown keys (not declared, not a
  * built-in). A param with no resolvable `type` leaves its value shape open.
+ *
+ * An unknown key one leading underscore away from a built-in is rejected
+ * exactly as before and additionally NAMED with it — see
+ * {@link builtinNearMissHint}. Message copy only; the accepted set is the
+ * declared params plus `allow`, unchanged.
  */
 export function validateActionParams(
   resolved: ResolvedActionParam[],
@@ -134,7 +204,7 @@ export function validateActionParams(
     issues.push({
       param: key,
       code: 'unknown_field',
-      message: `Unknown action param "${key}" — not declared on this action`,
+      message: `Unknown action param "${key}" — not declared on this action${builtinNearMissHint(key, allow)}`,
     });
   }