diff --git a/.changeset/settings-declared-step-grid-enforced.md b/.changeset/settings-declared-step-grid-enforced.md new file mode 100644 index 0000000000..ab1d609a24 --- /dev/null +++ b/.changeset/settings-declared-step-grid-enforced.md @@ -0,0 +1,57 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): 写入路径与 env 路径执行 settings 声明的 `step` 网格 (#6199) + +`step` 是 `SpecifierSchema` 五个值约束里的**第五个**,也是最后一个只声明不执行的。 +#5932(PR #6201)补齐 `min`/`max`/`minLength`/`maxLength` 之后,`step` 在 +`packages/services/service-settings/src/` 里仍是**零读取点**:superRefine 不校验它, +写入路径不读它,env 路径不读它。 + +**为什么判定为「值约束」而不是「纯 UI 提示」。** issue 提了两种读法,定论取自 schema +自己的写法:`step` 与 `min`/`max` 声明在**同一段** `/** number / slider: numeric +bounds and step. */` 注释之下,即它是按「界」被作者写下的,而 #5932 的裁决(声明了 +的界就必须绑定)随之传递。另一种读法(它只是 `input[type=number]` 上下箭头的步进, +从不表达「其他值非法」)经核查不成立:落地时 `step` 在本仓库与 `objectui` 中**没有 +任何消费者**——没有渲染器读它。按那种读法,这个键就是在为一个并不存在的渲染器表达 +「呈现」,那正是 ADR-0049 的洞,而不是 UI affordance。 + +**修法与 #5932 同形,是同一族的第五个成员:** + +- `step` 挂进 `DeclaredBounds` 与 `firstRangeViolation`,因此它按构造同时到达两扇门 + ——写入路径(`validatePatch`)与 env 路径(`effectiveEnvOverride` 这**一个**判定点) + ——不可能成为「只在一侧执行」的下一个键。 +- 越界发码表里现有的 `invalid_value`(ADR-0114:「rejected for a reason no other + member names」)。码表里没有任何成员命名「网格」,而码表是刻意封闭的; + `rest-server.ts` 早已把 Zod 的 `not_multiple_of` 映射到同一个成员,即同一条件从另一 + 个方向到达时的同一裁决。⛔ `packages/spec` 未改动。 +- 沿用 #5131 / #5932 的 **TOUCH 闸门**:只校验本次 patch 触及的键。网格在产品生命 + 周期里会被**放粗**(0.05 的滑杆改声明成 0.1),持有旧值的工作区必须仍能编辑无关设置。 + +**锚点(anchor)约定:** 值须落在 `min + k * step` 上;未声明 `min` 时锚点取 `0`。 +这是 HTML step-base 约定,也是声明读起来的唯一自洽含义 —— `min: 1, step: 2` 指的是 +**奇数**,而不是偶数;一律锚 0 会把这个 specifier 整个反转。`constraint` 同时带 +`step` 与(声明了的话)`min`,客户端据此自行重建网格。 + +**容差规则:** 网格判定为 `|value - nearest| <= max(|value|, |anchor|, |step|) * 1e-9`, +其中 `nearest = anchor + round((value - anchor) / step) * step`。精确取模是错的 —— +二进制浮点下 `0.7 / 0.1` 是 `6.999999999999999`、`1.2 / 0.1` 是 `11.999999999999998`, +而这两个都是控制台滑杆自己会发出的值。容差取**相对**而非绝对:绝对量随操作数变化, +`1e-9` 在 `step: 1e-6` 上会宽到三分之一步长,在 `max: 1048576` 上又比一个 ULP 还紧。 +`1e-9` 落在两类误差之间:double 的相对精度约 `2.2e-16`,几步算术累积约 `1e-15`,比这 +个界低六个数量级;而真正的越格差一小截步长(`0.15` 在 `0.1` 网格上差 `0.05`,相对 +`3e-1`),比它高八个数量级。比较在**值域**而非倍数域进行,以免容差的含义随网格粗细改变。 +剩余存疑的方向也是刻意的:本闸门是对「昨天什么都收」的收紧,所以在算术确实分辨不出时 +(量级大到网格比 double 自身间距还细)判**收**。 + +**非正的 `step` 声明不构成网格。** `step: 0`(`anchor + k * 0` 是一个点)、负值、 +非有限值一律**不记录网格**,与「option-bearing specifier 没有 options 表」同一处置: +无可执行者,行为不变,永不拒写。这与 #5204 的注册期姿态一致 —— 注册**报告**、从不 +拒绝 —— 而这里没有可报告的:声明了不可能网格的 manifest 既不拒写也不误配部署,它只是 +没有约束住,和其余没声明 `step` 的 specifier 处境完全相同。 + +**已知后果,裁决时已接受:** 全仓库唯一的 `step` 声明是 `ai.manifest.ts` 的 +`temperature`(`min: 0, max: 2, step: 0.1`)。执行之后 `0.15` 被拒。这是该声明按其 +字面绑定,而不是本闸门的缺陷;这份声明本身是否该改(若 `0.15` 应当合法,则该 manifest +应声明更细的 `step` 或不声明),属于 manifest 属主的问题。 diff --git a/packages/services/service-settings/src/envelope.conformance.test.ts b/packages/services/service-settings/src/envelope.conformance.test.ts index 4c65409f47..3c0a1a3602 100644 --- a/packages/services/service-settings/src/envelope.conformance.test.ts +++ b/packages/services/service-settings/src/envelope.conformance.test.ts @@ -457,6 +457,40 @@ describe('settings envelope (#4224) — SETTINGS_VALIDATION speaks the field-lev expect(field.constraint).toEqual({ allowed: 'smtp, log' }); }); + it('an off-grid number reaches the client as a parseable invalid_value (#6199)', async () => { + // The grid breach's whole ADR-0112 envelope, driven through the real route: + // the HTTP status AND the code, which is what makes this a refusal test + // rather than a "something threw" test. The service-level suite pins the + // `FieldError`; only here is the 400 observable, because + // `SettingsValidationError` carries no status of its own — the route maps + // it, and that mapping is the thing a client actually keys on. + const { http, service } = mount(); + service.registerManifest({ + namespace: 'stepped', + label: 'Stepped', + writePermission: 'setup.write', + readPermission: 'setup.access', + specifiers: [ + { key: 'temperature', type: 'slider', label: 'Temperature', min: 0, max: 2, step: 0.1 }, + ], + } as any); + const { status, body } = await drive(http, 'PUT /api/settings/:namespace', { + params: { namespace: 'stepped' }, + body: { temperature: 0.15 }, + }); + expect(status).toBe(400); + expect(body.error.code).toBe('SETTINGS_VALIDATION'); + + const [field] = body.error.details.fields; + expect(FieldErrorSchema.safeParse(field).success).toBe(true); + // `invalid_value` is the catalog's slot for "rejected for a reason no other + // member names" — no `FieldErrorCode` member names a grid, and the catalog + // is closed on purpose (ADR-0114), so a service does not get to invent one. + expect(field.code).toBe('invalid_value'); + // The spacing and its anchor both travel, so a client can rebuild the grid. + expect(field.constraint).toEqual({ step: 0.1, min: 0 }); + }); + it('the pre-#4224 map is gone from both of its old spellings', async () => { const http = lockedPattern(); const { body } = await drive(http, 'PUT /api/settings/:namespace', { diff --git a/packages/services/service-settings/src/settings-service.test.ts b/packages/services/service-settings/src/settings-service.test.ts index 9cc3824012..e2019c7f3e 100644 --- a/packages/services/service-settings/src/settings-service.test.ts +++ b/packages/services/service-settings/src/settings-service.test.ts @@ -1371,6 +1371,370 @@ describe('SettingsService — env overrides are checked against declared windows }); }); +/** + * #6199 — `step`, the fifth and last of `SpecifierSchema`'s value constraints, + * is enforced on the same two paths as the other four. + * + * The reading that settles it is the schema's own. `step` is declared under the + * SAME "numeric bounds and step" doc comment as `min`/`max`, so it is authored + * as a BOUND, and #5932's ruling — a declared bound binds — transfers with it. + * The competing reading, that `step` is only + * an `input[type=number]` arrow increment and never says other values are + * illegal, was checked and does not survive contact: `step` had ZERO read + * points at the time this landed — nothing in `packages/services/ + * service-settings`, nothing anywhere else in this repo, and nothing in + * `objectui` — so under that reading the key would be enforcing presentation + * for a renderer that does not exist. A declaration with no consumer at all is + * the ADR-0049 hole, not a UI affordance. + * + * The consequence is real and was accepted at ruling time: `ai.temperature` + * declares `min: 0, max: 2, step: 0.1`, and `0.15` — a perfectly sensible + * temperature for the model behind it — is now refused. That is the manifest's + * declaration binding as written; whether it SHOULD declare a 0.1 grid is the + * manifest owner's question, not this gate's. + */ +describe('SettingsService — the declared step grid is enforced at save time (#6199)', () => { + /** + * Three shapes the grid arrives in: with a window (the `ai.temperature` + * shape), with no window at all, and anchored somewhere other than zero. + */ + const gridManifest = { + namespace: 'grid', + version: 1, + label: 'Grid', + scope: 'tenant', + readPermission: 'setup.access', + writePermission: 'setup.access', + specifiers: [ + { type: 'slider', key: 'temperature', label: 'Temperature', required: false, + default: 0.7, min: 0, max: 2, step: 0.1 }, + { type: 'number', key: 'bare', label: 'Bare', required: false, step: 5 }, + { type: 'number', key: 'odd', label: 'Odd', required: false, min: 1, max: 9, step: 2 }, + ], + } as any; + + const gridService = () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(gridManifest); + return svc; + }; + + it('refuses a value that misses the grid, naming the step', async () => { + const svc = gridService(); + await expect(svc.setMany('grid', { temperature: 0.15 })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { + field: 'temperature', + // No `FieldErrorCode` member names a grid breach, so it takes + // `invalid_value` — the catalog's declared slot for "rejected for a + // reason no other member names" (ADR-0114). The same verdict + // `rest-server.ts` already reaches for Zod's `not_multiple_of`, which + // is this exact condition arriving from the other direction. + code: 'invalid_value', + label: 'Temperature', + // The spacing AND its anchor: a client cannot reconstruct the grid + // from the step alone, and this specifier's base is its `min`. + constraint: { step: 0.1, min: 0 }, + value: 0.15, + }, + ], + }); + // Atomic: the rejected batch persisted nothing. + expect((await svc.get('grid', 'temperature')).source).toBe('default'); + }); + + it('accepts the decimal multiples binary floating point cannot represent exactly', async () => { + // THE reason an exact modulo is the wrong implementation. Not every decimal + // multiple misses — `2 / 0.1` and `0.2 / 0.1` happen to land exactly — which + // is precisely what makes the trap dangerous: it fires on SOME values of a + // grid and not others, so a naive check looks correct until an author picks + // the wrong temperature. The two pinned below are values the console's own + // slider emits, and `n % step === 0` refuses both. + expect(0.7 / 0.1).not.toBe(7); // 6.999999999999999 + expect(1.2 / 0.1).not.toBe(12); // 11.999999999999998 + // The tolerance is relative (1e-9 of the magnitudes involved) — six orders + // of magnitude above the ~1e-15 a few double operations accumulate, and + // eight below the 3e-1 relative miss of a genuinely off-grid `0.15`. + const svc = gridService(); + for (const v of [0, 0.1, 0.2, 0.3, 0.1 + 0.2, 0.7, 0.9, 1.1, 1.2, 1.9, 2]) { + await expect( + svc.setMany('grid', { temperature: v }), + `${v} sits on the 0.1 grid and must be accepted`, + ).resolves.toBeDefined(); + } + }); + + it('refuses the off-grid neighbours of those same values', async () => { + // The other half of the tolerance pin: it must still be a check. Each of + // these is a half-step away from a legal value, not a rounding artefact. + const svc = gridService(); + for (const v of [0.05, 0.15, 0.25, 0.75, 1.99]) { + await expect( + svc.setMany('grid', { temperature: v }), + `${v} misses the 0.1 grid and must be refused`, + ).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [{ field: 'temperature', code: 'invalid_value' }], + }); + } + }); + + it('anchors the grid at the declared min, not at zero', async () => { + // The HTML step-base convention, and the only one that makes the + // declaration mean what it reads as: `min: 1, step: 2` names the ODD + // numbers. Anchoring at 0 regardless would invert this specifier entirely + // — it would accept exactly the values the author excluded. + const svc = gridService(); + for (const v of [1, 3, 5, 9]) { + await expect(svc.setMany('grid', { odd: v }), `${v} is on the min-anchored grid`) + .resolves.toBeDefined(); + } + for (const v of [2, 4, 8]) { + await expect(svc.setMany('grid', { odd: v }), `${v} is off the min-anchored grid`) + .rejects.toMatchObject({ + fields: [{ field: 'odd', code: 'invalid_value', constraint: { step: 2, min: 1 } }], + }); + } + }); + + it('falls back to a zero anchor when the specifier declares no min', async () => { + const svc = gridService(); + await expect(svc.setMany('grid', { bare: 0 })).resolves.toBeDefined(); + await expect(svc.setMany('grid', { bare: 15 })).resolves.toBeDefined(); + await expect(svc.setMany('grid', { bare: -10 })).resolves.toBeDefined(); + await expect(svc.setMany('grid', { bare: 12 })).rejects.toMatchObject({ + // No `min` was declared, so none travels in the constraint either — the + // client is told the spacing and nothing invented. + fields: [{ field: 'bare', code: 'invalid_value', constraint: { step: 5 } }], + }); + }); + + it('reports the WINDOW before the grid when a value breaches both', async () => { + // `validatePatch` emits at most one `FieldError` per key, so the ordering + // decides what the author is told. The window is the coarser, more + // actionable fact: `temperature: 40` is twenty times the declared maximum, + // and answering "it misses the 0.1 grid" — true, and useless — would bury + // that. + const svc = gridService(); + await expect(svc.setMany('grid', { temperature: 40.05 })).rejects.toMatchObject({ + fields: [{ field: 'temperature', code: 'max_value', constraint: { min: 0, max: 2 } }], + }); + await expect(svc.setMany('grid', { temperature: -0.05 })).rejects.toMatchObject({ + fields: [{ field: 'temperature', code: 'min_value' }], + }); + }); + + it('checks the grid only when the patch TOUCHES the key', async () => { + // The #5131/#5932 gate, inherited for the same reason and one more: a grid + // gets COARSENED over a product's life (a 0.05 slider re-declared at 0.1), + // and a workspace holding a value that was legal when it was written must + // still be able to edit its unrelated settings. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + ...gridManifest, + specifiers: gridManifest.specifiers.map((s: any) => + s.key === 'temperature' ? { ...s, step: 0.05 } : s, + ), + } as any); + await svc.setMany('grid', { temperature: 0.15 }); + svc.registerManifest(gridManifest); // the coarsened grid + + // The stale value is still there … + expect((await svc.get('grid', 'temperature')).value).toBe(0.15); + // … and a patch that never mentions it is not rejected on its account. + await expect(svc.setMany('grid', { bare: 10 })).resolves.toBeDefined(); + expect((await svc.get('grid', 'bare')).value).toBe(10); + // Only re-writing the key itself is refused. + await expect(svc.setMany('grid', { temperature: 0.15 })).rejects.toMatchObject({ + fields: [{ field: 'temperature', code: 'invalid_value' }], + }); + // And a reset still clears it — an all-null patch is never blocked. + await expect(svc.resetNamespace('grid')).resolves.toBeGreaterThan(0); + expect((await svc.get('grid', 'temperature')).value).toBe(0.7); // back to the default + }); + + it('compares a number that arrived as a string, so a form post is not enforced-as-transport', async () => { + const svc = gridService(); + await expect(svc.setMany('grid', { temperature: '0.2' })).resolves.toBeDefined(); + await expect(svc.setMany('grid', { temperature: '0.15' })).rejects.toMatchObject({ + fields: [{ field: 'temperature', code: 'invalid_value' }], + }); + }); + + it('leaves a value it cannot compare alone — that is a different constraint', async () => { + // Same posture the window check takes: the value's SHAPE is `invalid_type`'s + // business. `Number(true)` is 1 and `Number([])` is 0, so coercing here + // would invent a grid verdict about a shape this check never judges. + const svc = gridService(); + await expect(svc.setMany('grid', { temperature: true })).resolves.toBeDefined(); + await expect(svc.setMany('grid', { temperature: 'warm' })).resolves.toBeDefined(); + await expect(svc.setMany('grid', { temperature: [] })).resolves.toBeDefined(); + await expect(svc.setMany('grid', { temperature: { v: 0.15 } })).resolves.toBeDefined(); + // Empty is `required`'s business, not the grid's. + await expect(svc.setMany('grid', { temperature: null })).resolves.toBeDefined(); + }); + + it('treats a step that is not a positive spacing as no grid at all', async () => { + // `anchor + k * 0` is a single point, and a negative spacing names the same + // grid as its absolute value while reading as an author error. Neither is a + // grid, so neither records one — the same disposition an option-bearing + // specifier with no table gets. Registration REPORTS and never refuses + // (#5204); an impossible grid has nothing to report, because it rejects no + // write and misconfigures no deployment. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'nogrid', version: 1, label: 'No grid', scope: 'tenant', + readPermission: 'setup.access', writePermission: 'setup.access', + specifiers: [ + { type: 'number', key: 'zero', label: 'Zero', step: 0 }, + { type: 'number', key: 'negative', label: 'Negative', step: -0.1 }, + { type: 'number', key: 'nan', label: 'NaN', step: Number.NaN }, + // A bad step must not swallow the window declared beside it. + { type: 'number', key: 'windowed', label: 'Windowed', min: 0, max: 10, step: 0 }, + ], + } as any); + for (const key of ['zero', 'negative', 'nan']) { + await expect(svc.setMany('nogrid', { [key]: 3.14159 }), `${key} declares no grid`) + .resolves.toBeDefined(); + } + await expect(svc.setMany('nogrid', { windowed: 3.14159 })).resolves.toBeDefined(); + await expect(svc.setMany('nogrid', { windowed: 99 })).rejects.toMatchObject({ + fields: [{ field: 'windowed', code: 'max_value' }], + }); + }); + + it('never echoes the off-grid value for an encrypted specifier', async () => { + // Same rule as `invalid_option` and the window codes, same reason: a grid is + // not a secret, but `encrypted` is authorable on any specifier and this + // message travels back through the API and into logs. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'vaultgrid', version: 1, label: 'Vault grid', scope: 'tenant', + readPermission: 'setup.access', writePermission: 'setup.access', + specifiers: [ + { type: 'number', key: 'shard', label: 'Shard', encrypted: true, min: 0, step: 100 }, + ], + } as any); + const err = await svc.setMany('vaultgrid', { shard: 12345 }).catch((e) => e); + expect(err.code).toBe('SETTINGS_VALIDATION'); + expect(err.fields[0]).toMatchObject({ field: 'shard', code: 'invalid_value' }); + expect(err.fields[0].value).toBeUndefined(); + expect(err.message).not.toContain('12345'); + // The grid still travels, so the caller learns what to do. + expect(err.fields[0].constraint).toMatchObject({ step: 100, min: 0 }); + }); + + it('binds the real ai manifest — the consequence accepted at ruling time', async () => { + // The repo's ONLY `step` declaration: `ai.temperature`, `min: 0, max: 2, + // step: 0.1`. Under enforcement `0.15` is refused, and that is the + // declaration binding as written rather than a defect of this gate. + // `temperature` is `visible: "${data.provider !== 'memory'}"` and the + // default provider is `memory`, so the patch carries a real provider (and + // its required key) — otherwise the TOUCH/visible contract skips the + // specifier entirely, which would make this test green for the wrong reason. + const svc = new SettingsService({ env: {} }); + svc.registerManifest(aiSettingsManifest); + + await expect( + svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 0.15 }), + ).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { field: 'temperature', code: 'invalid_value', constraint: { step: 0.1, min: 0 }, value: 0.15 }, + ], + }); + // Nothing was stored — the whole batch is atomic. + expect((await svc.get('ai', 'provider')).value).toBe('memory'); + // …and the on-grid values the slider actually emits still go through. + await expect( + svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 0.7 }), + ).resolves.toBeDefined(); + expect((await svc.get('ai', 'temperature')).value).toBe(0.7); + }); +}); + +/** + * #6199, env half — the grid is judged at the ONE decision point + * (`effectiveEnvOverride`), for the reason #5204 is on file: the same + * comparison in two places is how the env half came to disagree with the save + * half in the first place. `step` rides `DeclaredBounds` and + * `firstRangeViolation`, so it reaches both doors by construction. + */ +describe('SettingsService — env overrides are checked against the declared step grid (#6199)', () => { + const spyLogger = () => { + const errors: string[] = []; + return { errors, logger: { error: (m: string) => void errors.push(m) } }; + }; + + it('ignores an off-grid OS_AI_TEMPERATURE and resolves the manifest default instead', async () => { + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AI_TEMPERATURE: '0.15' }, logger }); + svc.registerManifest(aiSettingsManifest); + + const r = await svc.get('ai', 'temperature'); + expect(r.value).toBe(0.7); // the manifest default, not 0.15 + expect(r.source).toBe('default'); + // Not in force, so it pins nothing either — read and write agree. + expect(r.locked).toBe(false); + expect(r.cascadeChain?.some((e) => e.scope === 'env')).toBe(false); + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('OS_AI_TEMPERATURE'); + // The grid breach gets its OWN sentence, not the window template: the value + // sits squarely inside `min 0, max 2`, so "is outside the declared step" + // would be a false description of what happened. + expect(errors[0]).toContain('does not sit on the declared step grid'); + expect(errors[0]).toContain('step 0.1'); + expect(errors[0]).toContain('IGNORED'); + expect(errors[0]).toContain('does NOT take effect'); + }); + + it('an on-grid override still wins at the top of the cascade and locks the key', async () => { + // The regression pin for the untouched path — the check must not turn into + // "env never applies to a stepped key". `1.2` is another float trap: + // `1.2 / 0.1` is `11.999999999999998`. + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AI_TEMPERATURE: '1.2' }, logger }); + svc.registerManifest(aiSettingsManifest); + + const r = await svc.get('ai', 'temperature'); + expect(r.value).toBe(1.2); + expect(r.source).toBe('env'); + expect(r.locked).toBe(true); + expect(errors).toHaveLength(0); + }); + + it('reports the misconfiguration at registration, and says it ONCE', async () => { + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AI_TEMPERATURE: '0.15' }, logger }); + expect(errors).toHaveLength(0); + svc.registerManifest(aiSettingsManifest); + expect(errors).toHaveLength(1); + for (let i = 0; i < 5; i++) await svc.get('ai', 'temperature'); + await svc.getNamespace('ai'); + expect(errors).toHaveLength(1); + }); + + it('a REJECTED override pins nothing — the key stays editable', async () => { + // The `locked` coherence rule #5204 established, extended to the grid for + // free BECAUSE both are judged at the one point: a key configurable by + // nothing (env ignored, UI refused) would be a lockout only an env edit + // could clear. + const { logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AI_TEMPERATURE: '0.15' }, logger }); + svc.registerManifest(aiSettingsManifest); + + expect((await svc.get('ai', 'temperature')).locked).toBe(false); + await expect( + svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 0.9 }), + ).resolves.toBeDefined(); + const after = await svc.get('ai', 'temperature'); + expect(after.value).toBe(0.9); + expect(after.source).toBe('global'); // the ai manifest is `scope: 'global'` + }); +}); + describe('SettingsService — user-scoped values', () => { it('isolates writes by ctx.userId', async () => { const svc = new SettingsService({ env: {} }); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index 88ffc1e146..7a92cec274 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -112,29 +112,103 @@ function firstRejectedOption(allowed: string[], value: unknown): { value: unknow } /** - * The value bounds a specifier declares — `min`/`max` (numeric window) and - * `minLength`/`maxLength` (string length window), as `SpecifierSchema` spells - * them (#5932). + * The value bounds a specifier declares — `min`/`max` (numeric window), + * `step` (numeric grid) and `minLength`/`maxLength` (string length window), as + * `SpecifierSchema` spells them (#5932, #6199). */ interface DeclaredBounds { min?: number; max?: number; + /** + * The grid spacing, from the `step` declared under `SpecifierSchema`'s + * "numeric bounds and step" comment (#6199). Only ever set to a FINITE + * POSITIVE number — see {@link declaredBounds} for why a `0`, a negative or a + * non-finite `step` declares no grid at all rather than an impossible one. + */ + step?: number; minLength?: number; maxLength?: number; } /** A breached bound, in the form both call sites need to report it. */ interface RangeViolation { - /** `FieldErrorCode`, ADR-0114 — the member that mirrors the breached property. */ - code: 'min_value' | 'max_value' | 'min_length' | 'max_length'; - /** `range` (numeric window) or `length` (character window) — picks the prose. */ - kind: 'range' | 'length'; + /** + * `FieldErrorCode`, ADR-0114 — the member that mirrors the breached property. + * + * The window codes are the property's own name (`min` → `min_value`). A grid + * breach has no such member, so it takes `invalid_value`, the catalog's + * declared slot for "rejected for a reason no other member names" — the same + * verdict `rest-server.ts` already reaches for Zod's `not_multiple_of`, which + * is this exact condition arriving from the other direction. Inventing a + * `not_multiple_of` member would be a `packages/spec` change, and the catalog + * is closed on purpose. + */ + code: 'min_value' | 'max_value' | 'min_length' | 'max_length' | 'invalid_value'; + /** + * `range` (numeric window), `step` (numeric grid) or `length` (character + * window) — picks the prose at both call sites. + */ + kind: 'range' | 'step' | 'length'; /** The declared window in machine form, for `FieldError.constraint`. */ constraint: Record; /** The declared window in prose (`min 6, max 64`), for the env log line. */ declared: string; } +/** + * Relative slack allowed when deciding whether a value sits ON the declared + * grid, as a fraction of the magnitudes involved (#6199). + * + * A grid check is `value === anchor + k * step` for some integer `k`, and in + * binary floating point that equation is almost never exactly true for the + * decimal grids people actually declare. `ai.temperature` declares `step: 0.1`; + * `0.7 / 0.1` is `6.999999999999999`, not `7`, so an exact modulo would reject + * a value the manifest's own default neighbourhood is made of. The tolerance is + * RELATIVE rather than absolute because the absolute error scales with the + * operands: at `step: 1e-6` an absolute `1e-9` would be a third of a step wide, + * and at `max: 1048576` (`ai.max_tokens`) it would be tighter than one ULP. + * + * `1e-9` sits deliberately between the two errors it must separate. A double + * carries ~2.2e-16 of relative precision, so a handful of arithmetic steps + * accumulate ~1e-15 at worst — six orders of magnitude of headroom below this + * bound. A genuine off-grid value is off by a fraction of a step: `0.15` on a + * `0.1` grid misses by `0.05`, which is `3e-1` relative — eight orders of + * magnitude above it. Nothing real lands in the gap. + * + * The direction of the remaining doubt is deliberate too. This gate is a + * TIGHTENING of a path that accepted everything yesterday, so where the + * arithmetic genuinely cannot tell (a value so large the grid is finer than the + * double's own spacing there), it accepts. Rejecting a legitimate write is the + * expensive mistake; letting one absurd-magnitude value through is not. + */ +const STEP_GRID_TOLERANCE = 1e-9; + +/** + * True when `value` sits on the grid `anchor + k * step` for some integer `k`, + * within {@link STEP_GRID_TOLERANCE} (#6199). + * + * The ANCHOR is the declared `min` when there is one, else `0` — the HTML + * step-base convention, and the one the vocabulary itself points at: `step` + * lives under `SpecifierSchema`'s `min`/`max` comment, and a `slider` declaring + * `min: 1, step: 2` means the odd numbers, not the even ones. Nothing in this + * repo declares a different base; the only other `multipleOf`-shaped rule + * anywhere (Zod's, mapped in `rest-server.ts`) is anchored at 0, which is the + * same convention with no `min` declared. + * + * The comparison happens in the VALUE domain, not the multiplier domain: + * `Math.abs(k - Math.round(k))` would measure the error as a fraction of a + * step, so its meaning would change with the grid's fineness. Measuring + * `value` against the nearest grid point keeps the tolerance a property of the + * numbers, which is what the floating-point error is a property of. + */ +function isOnStepGrid(value: number, anchor: number, step: number): boolean { + const k = Math.round((value - anchor) / step); + if (!Number.isFinite(k)) return true; // cannot judge — accept, per the posture above + const nearest = anchor + k * step; + const scale = Math.max(Math.abs(value), Math.abs(anchor), Math.abs(step)); + return Math.abs(value - nearest) <= scale * STEP_GRID_TOLERANCE; +} + /** * The bounds this specifier declares, or `null` when it declares none. * @@ -144,8 +218,8 @@ interface RangeViolation { * superRefine *ties* an option table to exactly `select`/`radio`/`multiselect` * (it rejects those three without one), so "types that must declare a table" * and "types whose value is checked against it" are the same set and no third - * list can drift. It ties `min`/`max`/`minLength`/`maxLength` to nothing — - * doc comments say `number`/`slider` and `text`/`textarea`, but the schema + * list can drift. It ties `min`/`max`/`step`/`minLength`/`maxLength` to nothing + * — doc comments say `number`/`slider` and `text`/`textarea`, but the schema * accepts them on any specifier and only checks their ordering. Inventing a * type list here would therefore BE the third list, and it would recreate this * issue one level down: a `min` authored on a type not on my list would parse, @@ -153,9 +227,23 @@ interface RangeViolation { * declared. The value's SHAPE decides applicability instead (see * {@link firstRangeViolation}), which is exactly how the `pattern` branch in * `validatePatch` has always worked. + * + * `step` (#6199) is admitted on a STRICTER test than the window bounds: finite + * AND positive. A window bound of `0` or `-3` is a perfectly meaningful window; + * a `step` of `0` or `-0.1` is not a grid at all — `anchor + k * 0` is a single + * point and a negative spacing names the same grid as its absolute value while + * reading as an author error. Such a declaration therefore records NO grid, + * which is the same disposition this function already gives a non-finite bound + * and the same one `registerManifest` gives an option-bearing specifier with no + * table: nothing to enforce, unchanged behaviour, never a refused write. It is + * also the posture #5204 settled for the declaration audit one level up — + * registration REPORTS, it never refuses — and there is nothing to report here, + * because a manifest that declares an impossible grid rejects no writes and + * misconfigures no deployment; it merely fails to constrain, which is exactly + * where every other specifier without a `step` already sits. */ function declaredBounds(spec: { - min?: unknown; max?: unknown; minLength?: unknown; maxLength?: unknown; + min?: unknown; max?: unknown; step?: unknown; minLength?: unknown; maxLength?: unknown; }): DeclaredBounds | null { const out: DeclaredBounds = {}; let any = false; @@ -166,6 +254,10 @@ function declaredBounds(spec: { any = true; } } + if (typeof spec.step === 'number' && Number.isFinite(spec.step) && spec.step > 0) { + out.step = spec.step; + any = true; + } return any ? out : null; } @@ -204,14 +296,21 @@ function numericValue(value: unknown): number | null { * open a second implementation of the same comparison. * * A value that is not comparable against a declared window is left alone (a - * non-numeric value under `min`/`max`, a non-string under + * non-numeric value under `min`/`max`/`step`, a non-string under * `minLength`/`maxLength`). Check ordering mirrors `record-validator.ts`'s * equivalent branches so the two report the same bound for the same value; a * value can breach only one side of a well-ordered window anyway, and * `SpecifierSchema` rejects `min > max` at parse time. + * + * The grid (`step`, #6199) is judged AFTER the window it lives inside, so a + * value that is both out of range and off grid is reported as out of range. + * That ordering is not cosmetic: the window is the coarser, more actionable + * fact, and `validatePatch` emits at most one `FieldError` per key — telling an + * author that `temperature: 40` misses the 0.1 grid, while true, buries that it + * is twenty times the declared maximum. */ function firstRangeViolation(bounds: DeclaredBounds, value: unknown): RangeViolation | null { - const { min, max, minLength, maxLength } = bounds; + const { min, max, step, minLength, maxLength } = bounds; if (typeof min === 'number' || typeof max === 'number') { const n = numericValue(value); @@ -232,6 +331,23 @@ function firstRangeViolation(bounds: DeclaredBounds, value: unknown): RangeViola } } + if (typeof step === 'number') { + const n = numericValue(value); + // The anchor is the declared `min` when there is one, else 0 — see + // {@link isOnStepGrid}. It travels in `constraint` alongside `step` because + // a client cannot reconstruct the grid from the spacing alone, and the + // specifier it came from may declare no `min` at all. + const anchor = typeof min === 'number' ? min : 0; + if (n !== null && !isOnStepGrid(n, anchor, step)) { + return { + code: 'invalid_value', + kind: 'step', + constraint: { step, ...(typeof min === 'number' ? { min } : {}) }, + declared: anchor === 0 ? `step ${step}` : `step ${step} from ${anchor}`, + }; + } + } + if (typeof minLength === 'number' || typeof maxLength === 'number') { if (typeof value === 'string') { const actual = value.length; @@ -280,7 +396,8 @@ interface RegisteredManifest { optionTables: Map; /** * Declared value bounds for every specifier that declares at least one of - * `min` / `max` / `minLength` / `maxLength`, keyed by specifier key (#5932). + * `min` / `max` / `step` / `minLength` / `maxLength`, keyed by specifier key + * (#5932, #6199). * * Precomputed for the same reason and read the same way as `optionTables`: * `get()` is the hottest path, and an ABSENT key means "this specifier @@ -449,7 +566,8 @@ export class SettingsService { if (typeof spec.default !== 'undefined') defaults.set(spec.key, spec.default); // Declared bounds are recorded wherever they are declared — see // `declaredBounds` for why this is keyed on the declaration and the - // option table is keyed on the type (#5932). + // option table is keyed on the type (#5932), and why a `step` that is not + // a finite positive spacing records no grid at all (#6199). const declared = declaredBounds(spec); if (declared) bounds.set(spec.key, declared); if (OPTION_BEARING_TYPES.has(spec.type)) { @@ -534,7 +652,10 @@ export class SettingsService { * env half came to disagree with the save half in the first place. Both * families are judged at this one point, by the same helpers the save path * calls, and both produce the same verdict — an override that is not in force - * contributes no value and pins nothing. + * contributes no value and pins nothing. #6199 folded `step` into that same + * family rather than opening a third branch: it rides `DeclaredBounds` and + * `firstRangeViolation`, so it arrives on both paths at once by construction + * and cannot be the next constraint that is enforced on one door only. */ private effectiveEnvOverride( reg: RegisteredManifest, @@ -566,11 +687,22 @@ export class SettingsService { if (bounds) { const breach = firstRangeViolation(bounds, value); if (breach) { - this.reportRejectedEnvOverride(reg, namespace, key, envName, value, { - what: `is outside the declared ${breach.kind} for`, - detail: `Allowed ${breach.kind}: ${breach.declared}.`, - fix: `a value within the allowed ${breach.kind}`, - }); + // A grid breach gets its own three fragments rather than being forced + // through the window template (#6199): "is outside the declared step" + // and "a value within the allowed step" are both false descriptions of + // what happened — the value can sit squarely inside every declared + // bound and still miss the grid, which is the whole point of the check. + this.reportRejectedEnvOverride(reg, namespace, key, envName, value, breach.kind === 'step' + ? { + what: 'does not sit on the declared step grid for', + detail: `Allowed values: ${breach.declared}.`, + fix: 'a value on the declared step grid', + } + : { + what: `is outside the declared ${breach.kind} for`, + detail: `Allowed ${breach.kind}: ${breach.declared}.`, + fix: `a value within the allowed ${breach.kind}`, + }); return null; } } @@ -1085,6 +1217,9 @@ export class SettingsService { * - `min` / `max` / `minLength` / `maxLength` + non-empty value outside the * declared window → rejected (`min_value` / `max_value` / `min_length` / * `max_length`, #5932). + * - `step` + non-empty numeric value that misses the declared grid + * (`min + k * step`, or `k * step` where no `min` is declared) → rejected + * (`invalid_value`, #6199). * - All-null patches (namespace reset) and unparseable visibility * expressions skip validation rather than block the write. * @@ -1245,6 +1380,17 @@ export class SettingsService { // (and negatives): the value reaches better-auth's password policy and // is honoured there, so the declaration was the only thing claiming a // floor existed and nothing was holding it. + // + // `step` (#6199) is the fifth and last of the value constraints + // `SpecifierSchema` declares, and it joins the family here rather than + // getting a branch of its own. The reading that settles it is the + // schema's own: `step` sits under the SAME "numeric bounds and step" + // comment as `min`/`max`, so it is authored as a bound and a declared + // bound binds. Read the other way — a pure `input[type=number]` arrow + // increment — it would have to have a UI consumer to be doing anything, + // and it has none: `step` had zero read points anywhere in this repo or + // in `objectui` when this branch was written, which is a declaration + // enforcing nothing rather than a declaration enforcing presentation. if (!empty) { const bounds = declaredBounds(spec); const breach = bounds ? firstRangeViolation(bounds, value) : null; @@ -1259,7 +1405,9 @@ export class SettingsService { code: breach.code, message: breach.kind === 'length' ? `${label} must be within the declared length (${breach.declared}).${got}` - : `${label} must be within the declared range (${breach.declared}).${got}`, + : breach.kind === 'step' + ? `${label} must line up with the declared step (${breach.declared}).${got}` + : `${label} must be within the declared range (${breach.declared}).${got}`, label, // The declared window as discrete values, so a client composes its // own sentence instead of parsing ours (`FieldError.constraint`,