From 277cc048a4b5c613bf53d8d37916f9803656413f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 14:46:03 +0000 Subject: [PATCH] =?UTF-8?q?fix(spec):=20=E5=8F=82=E6=95=B0=E9=97=A8?= =?UTF-8?q?=E6=8B=92=E7=BB=9D=E3=80=8C=E5=B7=AE=E4=B8=80=E4=B8=AA=E4=B8=8B?= =?UTF-8?q?=E5=88=92=E7=BA=BF=E3=80=8D=E7=9A=84=E5=86=85=E5=BB=BA=E9=94=AE?= =?UTF-8?q?=E6=97=B6=E6=8C=87=E5=87=BA=E9=82=A3=E4=B8=AA=E5=86=85=E5=BB=BA?= =?UTF-8?q?=E9=94=AE=20(#5622)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateActionParams`(ADR-0104 D2)对每个未声明键都给同一句 `Unknown action param "selectedIds" — not declared on this action`。这句话本身是真的, 但它唯一可操作的读法是假的:读者的下一步是去 action 上把这个键声明成 param,而内建键 恰恰是**不能**被声明的那一个。#5568 的报告者把这条路走到了尽头,由此判定 REST 没有任何 合法形状能携带选择集并开了平台单 —— 而 `params._selectedIds` 一直是通的。 `unknown_field` 的消息现在在尾部追加一条指路:当 `'_' + key` 或去掉前导下划线的 `key` 命中允许的内建键集合时,消息点名那个内建键,并给出一句「它从哪来」。三个内建键有三个不同 的生产者,所以这句话按键区分:`recordId` / `objectName` 由 dispatcher 在服务端合入 (`params: { ...reqParams, recordId, objectName }`),`_selectedIds` 由渲染器的聚合批量 派发从客户端带入。走 `builtinKeys` 覆盖进来的键拿通用句。 **纯消息层改动 —— 判定不变。** 该键改动前后一样被拒,接受集合一个字节都没动,不构成 near-miss 的未知键消息与今天逐字节一致(匹配的是一个前导下划线,不是相似度)。这不是给 `selectedIds` 另开一条接受通道:契约仍然只有 `params._selectedIds` 一种拼写。 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JTSZAjgtL3oR6YcpNDhW3T --- .../action-param-builtin-near-miss-hint.md | 39 +++++ packages/spec/src/ui/action-params.test.ts | 139 ++++++++++++++++++ packages/spec/src/ui/action-params.zod.ts | 72 ++++++++- 3 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 .changeset/action-param-builtin-near-miss-hint.md diff --git a/.changeset/action-param-builtin-near-miss-hint.md b/.changeset/action-param-builtin-near-miss-hint.md new file mode 100644 index 0000000000..f1403c5bd6 --- /dev/null +++ b/.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`. diff --git a/packages/spec/src/ui/action-params.test.ts b/packages/spec/src/ui/action-params.test.ts index f7c9ebd0cd..0f25e4319c 100644 --- a/packages/spec/src/ui/action-params.test.ts +++ b/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 diff --git a/packages/spec/src/ui/action-params.zod.ts b/packages/spec/src/ui/action-params.zod.ts index a745851555..1a59935062 100644 --- a/packages/spec/src/ui/action-params.zod.ts +++ b/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 = 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 { + 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)}`, }); }