From 91f94600e7a10d5a6c018e6a420017a63d8ac025 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 07:38:14 +0000 Subject: [PATCH] =?UTF-8?q?fix(service-settings):=20remove=20step=200.1=20?= =?UTF-8?q?from=20ai.temperature=20=E2=80=94=20the=20window=20is=20the=20t?= =?UTF-8?q?rue=20domain=20(#6550)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #6199 a declared step binds as a value constraint on both doors, and temperature's true domain is continuous on [0, 2]: the 0.1 grid refused legal values (0.15 on PUT /api/settings/ai; OS_AI_TEMPERATURE=0.15 loudly ignored). min 0 / max 2 stay and keep binding. The #6199 grid machinery is untouched; its env-half tests move to a synthetic step-declaring fixture, and the real ai manifest's post-ruling behaviour is pinned in ai.manifest.test.ts. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01USNUyHEr7uaU6MoEWXitei --- .changeset/ai-temperature-step-removal.md | 5 + .../src/manifests/ai.manifest.test.ts | 103 ++++++++++++++++++ .../src/manifests/ai.manifest.ts | 6 +- .../src/settings-service.test.ts | 102 +++++++++-------- 4 files changed, 163 insertions(+), 53 deletions(-) create mode 100644 .changeset/ai-temperature-step-removal.md diff --git a/.changeset/ai-temperature-step-removal.md b/.changeset/ai-temperature-step-removal.md new file mode 100644 index 0000000000..5b0bc8c212 --- /dev/null +++ b/.changeset/ai-temperature-step-removal.md @@ -0,0 +1,5 @@ +--- +'@objectstack/service-settings': patch +--- + +Remove `step: 0.1` from the `ai.temperature` specifier (#6550). Since #6199 a declared `step` binds as a value constraint on both doors, and temperature's true domain is continuous on [0, 2]: the 0.1 grid refused legal values — `PUT /api/settings/ai` with `temperature: 0.15` was rejected and `OS_AI_TEMPERATURE=0.15` was loudly ignored. Both now work; `min: 0` / `max: 2` stay and keep binding (out-of-window values are still refused in the min/max vocabulary). #6199's grid machinery is untouched and still enforces any key that declares `step`. diff --git a/packages/services/service-settings/src/manifests/ai.manifest.test.ts b/packages/services/service-settings/src/manifests/ai.manifest.test.ts index 44f612e42b..1b619afbab 100644 --- a/packages/services/service-settings/src/manifests/ai.manifest.test.ts +++ b/packages/services/service-settings/src/manifests/ai.manifest.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from 'vitest'; import { SettingsManifestSchema } from '@objectstack/spec/system'; +import { SettingsService } from '../settings-service.js'; import { aiSettingsManifest, aiTestActionHandler, aiTestEmbedderActionHandler } from './ai.manifest.js'; describe('aiSettingsManifest', () => { @@ -91,6 +92,108 @@ describe('aiTestActionHandler', () => { }); }); +/** + * #6550 — `temperature` declares a window, not a grid. Since #6199 a declared + * `step` binds as a value constraint on both doors, and temperature's true + * domain is continuous on [0, 2]: the old `step: 0.1` refused legal values + * (`0.15`), the same declared-narrower-than-true-domain shape the #5712 ruling + * corrected for timezone/currency. A slider's step feel is a UI-layer display + * concern, not a contract refusal; `min`/`max` describe the real domain and + * stay binding. #6199's grid machinery is untouched — its enforcement and + * tests stand for any key that still declares `step`. + */ +describe('aiSettingsManifest — temperature declares a window, not a grid (#6550)', () => { + const byKey = (k: string) => (aiSettingsManifest.specifiers as any[]).find((s) => s.key === k); + + it('declares min 0 / max 2 and NO step, and that round-trips the spec parse', () => { + const t = byKey('temperature'); + expect(t.min).toBe(0); + expect(t.max).toBe(2); + expect(t.step).toBeUndefined(); + const parsed = SettingsManifestSchema.parse(aiSettingsManifest) as any; + const pt = parsed.specifiers.find((s: any) => s.key === 'temperature'); + expect(pt.min).toBe(0); + expect(pt.max).toBe(2); + expect(pt.step).toBeUndefined(); + }); + + it('write door: accepts 0.15 — the refusal that forced the ruling — and still 0.7', async () => { + // `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 and this test would be 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 }), + ).resolves.toBeDefined(); + expect((await svc.get('ai', 'temperature')).value).toBe(0.15); + // …and the values the slider emits keep working, of course. + 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); + }); + + it('write door: the window still binds — out-of-range values are refused in the min/max vocabulary', async () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(aiSettingsManifest); + await expect( + svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 2.5 }), + ).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { field: 'temperature', code: 'max_value', constraint: { min: 0, max: 2 }, value: 2.5 }, + ], + }); + await expect( + svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: -0.5 }), + ).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { field: 'temperature', code: 'min_value', constraint: { min: 0, max: 2 }, value: -0.5 }, + ], + }); + // Atomic: nothing landed. + expect((await svc.get('ai', 'temperature')).source).toBe('default'); + }); + + it('env door: OS_AI_TEMPERATURE=0.15 wins the cascade and locks the key', async () => { + // The mirror of the write door — #6199 judges both doors at the ONE + // decision point, so removing the grid frees this door too. + const errors: string[] = []; + const svc = new SettingsService({ + env: { OS_AI_TEMPERATURE: '0.15' }, + logger: { error: (m: string) => void errors.push(m) }, + }); + svc.registerManifest(aiSettingsManifest); + const r = await svc.get('ai', 'temperature'); + expect(r.value).toBe(0.15); + expect(r.source).toBe('env'); + expect(r.locked).toBe(true); + expect(r.cascadeChain?.some((e) => e.scope === 'env')).toBe(true); + expect(errors).toHaveLength(0); + }); + + it('env door: an out-of-window OS_AI_TEMPERATURE is still loudly ignored', async () => { + // The window survives the grid's removal: `2.5` sits outside `max: 2`. + const errors: string[] = []; + const svc = new SettingsService({ + env: { OS_AI_TEMPERATURE: '2.5' }, + logger: { error: (m: string) => void errors.push(m) }, + }); + svc.registerManifest(aiSettingsManifest); + const r = await svc.get('ai', 'temperature'); + expect(r.value).toBe(0.7); // the manifest default, not 2.5 + expect(r.source).toBe('default'); + expect(r.locked).toBe(false); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('OS_AI_TEMPERATURE'); + expect(errors[0]).toContain('IGNORED'); + expect(errors[0]).toContain('does NOT take effect'); + }); +}); + describe('aiSettingsManifest — embedder section', () => { it('exposes embedder_provider select with 10 options incl. none + 5 Chinese providers', () => { const f = (aiSettingsManifest.specifiers as any[]).find( diff --git a/packages/services/service-settings/src/manifests/ai.manifest.ts b/packages/services/service-settings/src/manifests/ai.manifest.ts index ff9a1386f8..e194a28b42 100644 --- a/packages/services/service-settings/src/manifests/ai.manifest.ts +++ b/packages/services/service-settings/src/manifests/ai.manifest.ts @@ -185,8 +185,12 @@ const manifest = { { type: 'group', id: 'defaults', label: 'Generation defaults', required: false, description: 'Applied when an agent or chat request does not specify its own value.', visible: "${data.provider !== 'memory'}" }, + // No `step` on purpose (#6550 ruling): since #6199 a declared `step` binds + // as a value constraint on both doors, and temperature's true domain is + // continuous on [0, 2] — a 0.1 grid refused legal values like `0.15`. + // `min`/`max` stay: the window IS the true domain, so it binds honestly. { type: 'slider', key: 'temperature', label: 'Temperature', - required: false, default: 0.7, min: 0, max: 2, step: 0.1, + required: false, default: 0.7, min: 0, max: 2, description: '0 = deterministic, 2 = highly creative.', visible: "${data.provider !== 'memory'}" }, { type: 'number', key: 'max_tokens', label: 'Max output tokens', diff --git a/packages/services/service-settings/src/settings-service.test.ts b/packages/services/service-settings/src/settings-service.test.ts index 90d2afc9d0..fd921a184f 100644 --- a/packages/services/service-settings/src/settings-service.test.ts +++ b/packages/services/service-settings/src/settings-service.test.ts @@ -1388,11 +1388,14 @@ describe('SettingsService — env overrides are checked against declared windows * 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. + * The consequence was real and was accepted at ruling time: `ai.temperature` + * then declared `min: 0, max: 2, step: 0.1`, so `0.15` — a perfectly sensible + * temperature for the model behind it — was refused. The manifest owner's + * question that ruling left open was answered by #6550: the grid came OFF the + * ai manifest (temperature's true domain is continuous on [0, 2]), so the + * fixtures below are synthetic step-declaring specifiers. The machinery they + * pin is unchanged and still binds any key that declares `step`; the real ai + * manifest's post-#6550 behaviour is pinned in `manifests/ai.manifest.test.ts`. */ describe('SettingsService — the declared step grid is enforced at save time (#6199)', () => { /** @@ -1626,33 +1629,12 @@ describe('SettingsService — the declared step grid is enforced at save time (# 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); - }); + // Until #6550 this block also pinned the one real declaration the gate + // bound: `ai.temperature`'s `step: 0.1` refusing `0.15`. That ruling took + // the grid off the ai manifest (temperature's true domain is continuous), + // so the real-manifest pin MOVED with the declaration — both doors' + // post-#6550 behaviour is pinned in `manifests/ai.manifest.test.ts`, and + // the grid machinery keeps binding through the synthetic fixtures above. }); /** @@ -1661,19 +1643,37 @@ describe('SettingsService — the declared step grid is enforced at save time (# * 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. + * + * The fixture is synthetic since #6550 took the grid off `ai.temperature` + * (this machinery needs a step-declaring key to bind, and the repo no longer + * ships one); it keeps the exact shape `ai.temperature` had at ruling time. + * The real ai manifest's env door is pinned in `manifests/ai.manifest.test.ts`. */ describe('SettingsService — env overrides are checked against the declared step grid (#6199)', () => { + const gridEnvManifest = { + namespace: 'gridenv', + version: 1, + label: 'Grid env', + scope: 'global', + 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 }, + ], + } as any; + 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 () => { + it('ignores an off-grid OS_GRIDENV_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 svc = new SettingsService({ env: { OS_GRIDENV_TEMPERATURE: '0.15' }, logger }); + svc.registerManifest(gridEnvManifest); - const r = await svc.get('ai', 'temperature'); + const r = await svc.get('gridenv', '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. @@ -1681,7 +1681,7 @@ describe('SettingsService — env overrides are checked against the declared ste expect(r.cascadeChain?.some((e) => e.scope === 'env')).toBe(false); expect(errors).toHaveLength(1); - expect(errors[0]).toContain('OS_AI_TEMPERATURE'); + expect(errors[0]).toContain('OS_GRIDENV_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. @@ -1696,10 +1696,10 @@ describe('SettingsService — env overrides are checked against the declared ste // "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 svc = new SettingsService({ env: { OS_GRIDENV_TEMPERATURE: '1.2' }, logger }); + svc.registerManifest(gridEnvManifest); - const r = await svc.get('ai', 'temperature'); + const r = await svc.get('gridenv', 'temperature'); expect(r.value).toBe(1.2); expect(r.source).toBe('env'); expect(r.locked).toBe(true); @@ -1708,12 +1708,12 @@ describe('SettingsService — env overrides are checked against the declared ste 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 }); + const svc = new SettingsService({ env: { OS_GRIDENV_TEMPERATURE: '0.15' }, logger }); expect(errors).toHaveLength(0); - svc.registerManifest(aiSettingsManifest); + svc.registerManifest(gridEnvManifest); expect(errors).toHaveLength(1); - for (let i = 0; i < 5; i++) await svc.get('ai', 'temperature'); - await svc.getNamespace('ai'); + for (let i = 0; i < 5; i++) await svc.get('gridenv', 'temperature'); + await svc.getNamespace('gridenv'); expect(errors).toHaveLength(1); }); @@ -1723,16 +1723,14 @@ describe('SettingsService — env overrides are checked against the declared ste // 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); + const svc = new SettingsService({ env: { OS_GRIDENV_TEMPERATURE: '0.15' }, logger }); + svc.registerManifest(gridEnvManifest); - 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((await svc.get('gridenv', 'temperature')).locked).toBe(false); + await expect(svc.setMany('gridenv', { temperature: 0.9 })).resolves.toBeDefined(); + const after = await svc.get('gridenv', 'temperature'); expect(after.value).toBe(0.9); - expect(after.source).toBe('global'); // the ai manifest is `scope: 'global'` + expect(after.source).toBe('global'); // the fixture is `scope: 'global'` }); });