From f31428402ea9d5d26b2124ef04d19b00cb49ec29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Czachdive?= Date: Tue, 9 Jun 2026 15:21:14 -0700 Subject: [PATCH 1/4] feat(shared): typed parameter schema in build_parametric_model contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool input gains an optional `parameters` array — name, label, type, min/max/step, unit, description, group, enum options — so the model authors the parameter panel's interface as structured output instead of encoding it in Customizer comments the client regex-parses. applyParameterSpecs overlays the schema onto parseParameters output: the code stays ground truth for which variables exist and their values, the parse acts as validator (specs for nonexistent variables are dropped, bounds expand to contain the current value, degenerate ranges and extreme magnitudes are rejected, all metadata is type-checked and length-capped against unvalidated persisted parts), and artifacts without the field render exactly as before. The zod schema is deliberately lenient on the way back in: quoted numbers coerce, malformed fields drop to undefined via .catch — a bad spec must never fail tool-input validation, which would turn the whole build call into a dynamic-tool part and silently kill the turn. Co-Authored-By: Claude Fable 5 --- shared/applyParameterSpecs.test.ts | 200 +++++++++++++++++++++++++++++ shared/applyParameterSpecs.ts | 160 +++++++++++++++++++++++ shared/chatAi.ts | 91 ++++++++++++- shared/parametricParts.ts | 7 +- shared/types.ts | 37 ++++++ 5 files changed, 491 insertions(+), 4 deletions(-) create mode 100644 shared/applyParameterSpecs.test.ts create mode 100644 shared/applyParameterSpecs.ts diff --git a/shared/applyParameterSpecs.test.ts b/shared/applyParameterSpecs.test.ts new file mode 100644 index 00000000..31f1eb7d --- /dev/null +++ b/shared/applyParameterSpecs.test.ts @@ -0,0 +1,200 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import applyParameterSpecs from './applyParameterSpecs.ts'; +import parseParameters from './parseParameters.ts'; +import type { ParameterSpec } from './types.ts'; + +const CODE = ` +cup_height = 100; // [50:5:200] +style = "round"; +size = [30, 20, 10]; + +module body() {} +`; + +describe('applyParameterSpecs', () => { + it('returns parsed parameters untouched when no specs are given', () => { + const parsed = parseParameters(CODE); + assert.deepEqual(applyParameterSpecs(parsed, undefined), parsed); + assert.deepEqual(applyParameterSpecs(parsed, null), parsed); + assert.deepEqual(applyParameterSpecs(parsed, []), parsed); + }); + + it('overlays spec metadata over comment-derived metadata', () => { + const specs: ParameterSpec[] = [ + { + name: 'cup_height', + label: 'Cup Height', + description: 'Outer height of the mug body', + group: 'Body', + min: 60, + max: 180, + step: 10, + unit: 'mm', + }, + ]; + const [cupHeight] = applyParameterSpecs(parseParameters(CODE), specs); + assert.equal(cupHeight.name, 'cup_height'); + assert.equal(cupHeight.displayName, 'Cup Height'); + assert.equal(cupHeight.description, 'Outer height of the mug body'); + assert.equal(cupHeight.group, 'Body'); + assert.equal(cupHeight.unit, 'mm'); + // Spec range wins over the `// [50:5:200]` Customizer comment. + assert.deepEqual(cupHeight.range, { min: 60, max: 180, step: 10 }); + // Value and default still come from the code. + assert.equal(cupHeight.value, 100); + assert.equal(cupHeight.defaultValue, 100); + }); + + it('drops spec entries that reference variables missing from the code', () => { + const parsed = parseParameters(CODE); + const result = applyParameterSpecs(parsed, [ + { name: 'phantom_variable', min: 0, max: 1 }, + ]); + assert.deepEqual(result, parsed); + }); + + it('keeps comment-derived metadata for parameters without a spec', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'style', options: [{ value: 'round' }, { value: 'hex' }] }, + ]); + const cupHeight = result.find((param) => param.name === 'cup_height'); + assert.deepEqual(cupHeight?.range, { min: 50, step: 5, max: 200 }); + }); + + it('applies enum options, coercing values for number parameters', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { + name: 'style', + options: [{ value: 'round', label: 'Round' }, { value: 'hex' }], + }, + { + name: 'cup_height', + options: [{ value: '80' }, { value: '120' }, { value: 'tall' }], + }, + ]); + const style = result.find((param) => param.name === 'style'); + assert.deepEqual(style?.options, [ + { value: 'round', label: 'Round' }, + { value: 'hex', label: 'hex' }, + ]); + const cupHeight = result.find((param) => param.name === 'cup_height'); + // 'tall' is not a number — coerced out rather than corrupting the list. + assert.deepEqual(cupHeight?.options, [ + { value: 80, label: '80' }, + { value: 120, label: '120' }, + ]); + }); + + it('spreads a base-name spec across flattened vector components', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'size', min: 5, max: 50, unit: 'mm', label: 'Size' }, + ]); + const components = result.filter((param) => param.name.startsWith('size[')); + assert.equal(components.length, 3); + for (const component of components) { + assert.deepEqual(component.range, { min: 5, max: 50 }); + assert.equal(component.unit, 'mm'); + } + // Axis labels stay distinct — the shared label must not collapse them. + assert.notEqual(components[0].displayName, components[1].displayName); + }); + + it('expands a bound that excludes the current code value', () => { + // cup_height is 100 in the code — a min-only spec above it would pin + // the slider outside its own value and rewrite geometry on first drag. + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'cup_height', min: 150 }, + ]); + const cupHeight = result.find((param) => param.name === 'cup_height'); + assert.equal(cupHeight?.range?.min, 100); + + const shrunk = applyParameterSpecs(parseParameters(CODE), [ + { name: 'cup_height', min: 20, max: 60 }, + ]); + assert.deepEqual( + shrunk.find((param) => param.name === 'cup_height')?.range, + { min: 20, max: 100, step: 5 }, + ); + }); + + it('rejects a zero-span range (min === max)', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'cup_height', min: 100, max: 100 }, + ]); + const cupHeight = result.find((param) => param.name === 'cup_height'); + assert.deepEqual(cupHeight?.range, { min: 50, step: 5, max: 200 }); + }); + + it('ignores numeric range specs on string parameters', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'style', min: 1, max: 10 }, + ]); + const style = result.find((param) => param.name === 'style'); + // range.max means maxLength for strings — a numeric range spec on a + // string variable must not leak into it. + assert.deepEqual(style?.range, {}); + }); + + it('ignores non-string presentation fields from unvalidated persisted parts', () => { + const parsed = parseParameters(CODE); + const result = applyParameterSpecs(parsed, [ + { + name: 'cup_height', + label: { nested: true }, + description: 42, + group: [], + unit: { mm: true }, + }, + ] as unknown as ParameterSpec[]); + const cupHeight = result.find((param) => param.name === 'cup_height'); + const original = parsed.find((param) => param.name === 'cup_height'); + assert.equal(cupHeight?.displayName, original?.displayName); + assert.equal(cupHeight?.description, original?.description); + assert.equal(cupHeight?.unit, undefined); + }); + + it('caps unit length so it cannot blow up the layout', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'cup_height', unit: ' millimeters ' }, + ]); + assert.equal( + result.find((param) => param.name === 'cup_height')?.unit, + 'mill', + ); + }); + + it('rejects option values with unit suffixes for number parameters', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'cup_height', options: [{ value: '80mm' }, { value: '120' }] }, + ]); + // parseFloat would read "80mm" as 80 — Number() must reject it. + assert.deepEqual( + result.find((param) => param.name === 'cup_height')?.options, + [{ value: 120, label: '120' }], + ); + }); + + it('rejects inverted ranges and non-positive steps from a bad spec', () => { + const result = applyParameterSpecs(parseParameters(CODE), [ + { name: 'cup_height', min: 200, max: 50, step: -5 }, + ]); + const cupHeight = result.find((param) => param.name === 'cup_height'); + // Falls back to the comment-derived range instead of breaking sliders. + assert.deepEqual(cupHeight?.range, { min: 50, step: 5, max: 200 }); + }); + + it('tolerates malformed spec entries from an untrusted tool input', () => { + const parsed = parseParameters(CODE); + const result = applyParameterSpecs(parsed, [ + null, + 42, + { noName: true }, + { name: 'style', options: [null, { value: 7 }] }, + ] as unknown as ParameterSpec[]); + assert.equal(result.length, parsed.length); + const style = result.find((param) => param.name === 'style'); + // No usable options survive validation — comment-derived (empty) kept. + assert.deepEqual(style?.options, []); + }); +}); diff --git a/shared/applyParameterSpecs.ts b/shared/applyParameterSpecs.ts new file mode 100644 index 00000000..7e5aa564 --- /dev/null +++ b/shared/applyParameterSpecs.ts @@ -0,0 +1,160 @@ +import type { Parameter, ParameterOption, ParameterSpec } from './types.ts'; + +/** + * Overlay the model-authored parameter schema (`artifact.parameters`) onto + * the parameters parsed from the OpenSCAD source. + * + * The code stays the ground truth for which parameters EXIST and what value + * they currently hold — `parseParameters` extracts those. The structured + * schema is the source of truth for presentation metadata: ranges, units, + * labels, descriptions, groups, and enum options. + * + * The parse doubles as the validator on the schema: + * - spec entries that don't match a variable in the code are ignored, + * - nonsensical ranges (min > max, step <= 0) fall back to comment-derived + * metadata instead of breaking the sliders, + * - parameters without a spec keep their comment-derived metadata, + * so artifacts persisted before the schema existed — and models that omit + * it — render exactly as before. + */ +// Caps on model- and DB-sourced metadata. Persisted parts re-enter without +// zod validation (see `asParametricParts`), and ShareView renders other +// people's artifacts — unbounded specs would let one malicious shared link +// burn every viewer's memory and render time. +const MAX_SPECS = 100; +const MAX_OPTIONS = 50; +const MAX_LABEL_LENGTH = 80; +const MAX_DESCRIPTION_LENGTH = 300; +const MAX_GROUP_LENGTH = 60; +const MAX_OPTION_LENGTH = 120; +const MAX_UNIT_LENGTH = 4; +// Bounds beyond any physical CAD dimension (1e9 mm = 1000 km) are noise, +// and extreme magnitudes (1e308, 5e-324) break the slider's percent and +// decimal math downstream. +const MAX_BOUND_MAGNITUDE = 1e9; +const MIN_STEP = 1e-9; + +export default function applyParameterSpecs( + parameters: Parameter[], + specs: ParameterSpec[] | null | undefined, +): Parameter[] { + if (!Array.isArray(specs) || specs.length === 0) return parameters; + + const byName = new Map(); + for (const spec of specs.slice(0, MAX_SPECS)) { + if ( + spec && + typeof spec === 'object' && + typeof spec.name === 'string' && + !byName.has(spec.name) + ) { + byName.set(spec.name, spec); + } + } + + return parameters.map((parameter) => { + // `size[2]`-style names come from flattened vector declarations — fall + // back to a spec on the base vector name so one `size` entry covers all + // of its components. + const baseName = parameter.name.replace(/\[\d+\]$/, ''); + const exact = byName.get(parameter.name); + const spec = exact ?? byName.get(baseName); + if (!spec) return parameter; + + const merged: Parameter = { ...parameter }; + + // Every field is re-checked with `typeof` even though the live tool + // path is zod-validated: persisted message parts re-enter through + // `asParametricParts`, which does NOT validate `parameters`, and a + // non-string label/description would crash React as a child node — + // taking down ShareView for every viewer of a shared link. + // + // Vector components matched via the base name keep their derived axis + // label ("Body Width") — sharing one label across every axis would make + // the sliders indistinguishable. + if (typeof exact?.label === 'string' && exact.label) { + merged.displayName = exact.label.slice(0, MAX_LABEL_LENGTH); + } + if (typeof spec.description === 'string' && spec.description) { + merged.description = spec.description.slice(0, MAX_DESCRIPTION_LENGTH); + } + if (typeof spec.group === 'string' && spec.group) { + merged.group = spec.group.slice(0, MAX_GROUP_LENGTH); + } + if (typeof spec.unit === 'string' && spec.unit.trim()) { + // The unit renders in a fixed ~4-character span next to the value — + // cap it so a runaway model string can't blow up the layout. + merged.unit = spec.unit.trim().slice(0, MAX_UNIT_LENGTH); + } + + // Numeric bounds only apply to number parameters. For strings the + // parse-land `range.max` doubles as maxLength, so a numeric range spec + // on a string variable is a contradiction, not an instruction. + if (parameter.type === 'number' || parameter.type === undefined) { + const range = { ...parameter.range }; + if (typeof spec.min === 'number' && isSaneBound(spec.min)) { + range.min = spec.min; + } + if (typeof spec.max === 'number' && isSaneBound(spec.max)) { + range.max = spec.max; + } + if ( + typeof spec.step === 'number' && + isSaneBound(spec.step) && + spec.step >= MIN_STEP + ) { + range.step = spec.step; + } + // The code's current value is ground truth — a bound that excludes it + // would render a slider pinned outside its own value (and the next + // track interaction would silently rewrite the geometry), so expand + // the offending bound to contain it. + if (typeof parameter.value === 'number') { + if (range.min !== undefined && range.min > parameter.value) { + range.min = parameter.value; + } + if (range.max !== undefined && range.max < parameter.value) { + range.max = parameter.value; + } + } + // `>=`: a zero-span range NaNs the slider's percent math. + const degenerate = + range.min !== undefined && + range.max !== undefined && + range.min >= range.max; + merged.range = degenerate ? parameter.range : range; + } + + if (Array.isArray(spec.options) && spec.options.length > 0) { + const options: ParameterOption[] = []; + const seenValues = new Set(); + for (const option of spec.options.slice(0, MAX_OPTIONS)) { + if (!option || typeof option !== 'object') continue; + const raw = (option as { value?: unknown }).value; + if (typeof raw !== 'string' || raw.length === 0) continue; + if (raw.length > MAX_OPTION_LENGTH) continue; + // Duplicate values would render duplicate select entries (and + // duplicate React keys) — first occurrence wins. + if (seenValues.has(raw)) continue; + seenValues.add(raw); + // `Number`, not `parseFloat` — "80mm" must be rejected, not read + // as 80 and silently diverge from what the code would accept. + const value = parameter.type === 'number' ? Number(raw) : raw; + if (typeof value === 'number' && !Number.isFinite(value)) continue; + const label = + typeof option.label === 'string' && option.label + ? option.label.slice(0, MAX_OPTION_LENGTH) + : raw; + options.push({ value, label }); + } + if (options.length > 0) merged.options = options; + } + + return merged; + }); +} + +// Finite and within physical-CAD magnitude — see MAX_BOUND_MAGNITUDE. +function isSaneBound(value: number): boolean { + return Number.isFinite(value) && Math.abs(value) <= MAX_BOUND_MAGNITUDE; +} diff --git a/shared/chatAi.ts b/shared/chatAi.ts index 439a9cc3..84fb07e9 100644 --- a/shared/chatAi.ts +++ b/shared/chatAi.ts @@ -16,10 +16,99 @@ export const createMeshOutputSchema = z.object({ fileType: z.enum(['glb', 'stl', 'obj', 'fbx']), }); +/** + * A malformed `parameters` entry must never fail tool-input validation: + * an invalid input turns the whole build_parametric_model call into a + * `dynamic-tool` part the client never compiles, silently killing the + * build turn. So every field degrades instead of erroring: + * - numeric fields accept `"50"` (models routinely quote numbers) and + * drop to undefined on anything else (`null`, `""`, `1e309`, objects), + * - string fields drop to undefined on non-strings, + * - a spec entry with a broken `name` collapses to null, which the + * merge layer (`applyParameterSpecs`) skips. + * `.catch`/`.preprocess` don't change the JSON schema advertised to + * providers — they only relax what we accept back. + */ +const lenientNumber = z.preprocess( + (value) => + typeof value === 'string' && value.trim() !== '' ? Number(value) : value, + z.number().finite().optional(), +); + +export const parameterSpecSchema = z.object({ + name: z + .string() + .min(1) + .describe( + 'Exact name of the top-of-file OpenSCAD variable this entry describes.', + ), + label: z + .string() + .optional() + .catch(undefined) + .describe( + 'Display name shown in the parameter panel. Defaults to a title-cased variable name.', + ), + type: z.enum(['number', 'string', 'boolean']).optional().catch(undefined), + description: z + .string() + .optional() + .catch(undefined) + .describe('One short sentence on what the parameter controls.'), + group: z + .string() + .optional() + .catch(undefined) + .describe('Section heading grouping related parameters.'), + min: lenientNumber + .catch(undefined) + .describe('Slider minimum for number parameters.'), + max: lenientNumber + .catch(undefined) + .describe('Slider maximum for number parameters.'), + step: lenientNumber + .catch(undefined) + .describe('Slider step for number parameters.'), + unit: z + .string() + .optional() + .catch(undefined) + .describe('Physical unit such as "mm" or "deg". Omit for unitless values.'), + options: z + .array( + z.object({ + // Accept numbers too (models routinely emit `value: 80` for + // numeric enums) and normalize to string — the merge layer + // re-types against the parsed parameter. + value: z + .union([z.string(), z.number()]) + .transform((value) => String(value)) + .describe( + 'Option value exactly as it appears in the OpenSCAD source.', + ), + label: z.string().optional().catch(undefined), + }), + ) + .optional() + .catch(undefined) + .describe('Allowed values for enum-style parameters.'), +}); + export const parametricArtifactSchema = z.object({ title: z.string().min(1), version: z.string().default('v1'), code: z.string().min(20), + // Nullish (not just optional) so a model that emits an explicit `null` + // doesn't fail tool-input validation and stall the agent loop, and + // `.catch(null)` so a structurally broken array degrades to the + // comment-derived fallback instead of killing the build call. + parameters: z + .array(parameterSpecSchema) + .nullish() + .catch(null) + .describe( + 'Typed schema for every user-editable variable declared at the top of `code`. This is the source of truth for the parameter panel: ranges, units, enum options, grouping, descriptions. Every entry must reference a variable that exists in the code.', + ), }); export const parametricCompileOutputSchema = z.object({ @@ -42,7 +131,7 @@ export const answerUserSchema = z.object({ export const chatTools = { build_parametric_model: tool({ description: - 'Create or update the complete OpenSCAD CAD artifact. After the browser compiles it, inspect the returned multi-view preview sheet and call this tool again if the model needs another revision.', + 'Create or update the complete OpenSCAD CAD artifact, including the typed parameter schema that drives the parameter panel. After the browser compiles it, inspect the returned multi-view preview sheet and call this tool again if the model needs another revision.', inputSchema: parametricArtifactSchema, outputSchema: parametricCompileOutputSchema, }), diff --git a/shared/parametricParts.ts b/shared/parametricParts.ts index 388e1334..c1f26d1e 100644 --- a/shared/parametricParts.ts +++ b/shared/parametricParts.ts @@ -177,9 +177,10 @@ export function isParametricArtifact( return false; } const artifact = value as Partial; - // Title + code are the only load-bearing fields. `version` is metadata - // and `parts` is optional. Parameters are derived client-side from - // `code` via `parseParameters` so we don't check for them here either. + // Title + code are the only load-bearing fields. `version` is metadata. + // `parameters` is optional overlay metadata — parameter existence and + // values are still derived from `code` via `parseParameters`, with the + // schema applied on top by `applyParameterSpecs` — so we don't check it. return ( typeof artifact.title === 'string' && typeof artifact.code === 'string' ); diff --git a/shared/types.ts b/shared/types.ts index 039f895e..a32f8b84 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -39,6 +39,41 @@ export type ParametricArtifact = { title: string; version: string; code: string; + // Model-authored typed interface for the top-of-file variables in `code`. + // Optional (and nullish-tolerant) so artifacts persisted before the schema + // existed — and models that omit it — keep working via the Customizer + // comment fallback in `parseParameters`. See `applyParameterSpecs`. + parameters?: ParameterSpec[] | null; +}; + +/** + * One entry of the structured parameter schema the model emits alongside + * the OpenSCAD source in `build_parametric_model`. The code remains the + * ground truth for which variables exist and their current values; specs + * carry the presentation metadata (ranges, units, labels, groups, enum + * options) that previously had to be reverse-engineered from Customizer + * comments by regex. + */ +export type ParameterSpec = { + /** Exact name of the top-of-file OpenSCAD variable this spec describes. */ + name: string; + /** Display name for the parameter panel; defaults to a title-cased name. */ + label?: string; + type?: 'number' | 'string' | 'boolean'; + description?: string; + /** Section heading grouping related parameters. */ + group?: string; + min?: number; + max?: number; + step?: number; + /** Physical unit shown next to the value, e.g. "mm" or "deg". */ + unit?: string; + /** + * Allowed values for enum-style parameters. Values are strings exactly as + * they appear in the source (numbers as plain strings) — kept union-free + * so every provider's JSON-schema subset accepts the tool definition. + */ + options?: { value: string; label?: string }[]; }; export type ParameterOption = { value: string | number; label: string }; @@ -65,6 +100,8 @@ export type Parameter = { range?: ParameterRange; options?: ParameterOption[]; maxLength?: number; + /** Physical unit from the model-authored spec, e.g. "mm" or "deg". */ + unit?: string; }; export type Conversation = Omit< From 104b5fc6a6c5fd0f9655593da57896390b3410bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Czachdive?= Date: Tue, 9 Jun 2026 15:21:23 -0700 Subject: [PATCH 2/4] fix(params): rewrite vector components on slider edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseParameters flattens `size = [30, 20, 10];` into size[0..2] sliders, but updateParameter searched for a literal `size[1] = ...;` assignment that never exists — so vector sliders updated local state while the code and preview silently never changed. Rewrite the i-th element of the vector literal instead, preserving spacing and trailing comments. Also make the Parameter/ModelConfig imports type-only so node --test can run this file without resolving the @shared path alias. Co-Authored-By: Claude Fable 5 --- src/lib/updateParameter.test.ts | 62 +++++++++++++++++++++++++++++++++ src/lib/utils.ts | 46 ++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 src/lib/updateParameter.test.ts diff --git a/src/lib/updateParameter.test.ts b/src/lib/updateParameter.test.ts new file mode 100644 index 00000000..b12aa245 --- /dev/null +++ b/src/lib/updateParameter.test.ts @@ -0,0 +1,62 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { updateParameter } from './utils.ts'; +import type { Parameter } from '@shared/types'; + +function numberParam(name: string, value: number): Parameter { + return { + name, + displayName: name, + value, + defaultValue: value, + type: 'number', + }; +} + +describe('updateParameter', () => { + it('rewrites a scalar assignment in place', () => { + const code = 'cup_height = 100; // [50:5:200]\ncylinder(h=cup_height);'; + assert.equal( + updateParameter(code, numberParam('cup_height', 120)), + 'cup_height = 120; // [50:5:200]\ncylinder(h=cup_height);', + ); + }); + + it('rewrites one component of a vector literal', () => { + // `parseParameters` flattens `size = [30, 20, 10];` into size[0..2] + // sliders — editing size[1] must rewrite the middle element, not + // search for a nonexistent `size[1] = ...;` assignment. + const code = 'size = [30, 20, 10]; // body\ncube(size);'; + assert.equal( + updateParameter(code, numberParam('size[1]', 25)), + 'size = [30, 25, 10]; // body\ncube(size);', + ); + }); + + it('preserves spacing and trailing comments on vector rewrites', () => { + const code = 'size = [ 30 , 20 , 10 ]; // [10:50]'; + assert.equal( + updateParameter(code, numberParam('size[0]', 42)), + 'size = [ 42 , 20 , 10 ]; // [10:50]', + ); + }); + + it('leaves code untouched for an out-of-range vector index', () => { + const code = 'size = [30, 20, 10];'; + assert.equal(updateParameter(code, numberParam('size[5]', 99)), code); + }); + + it('still matches a literal name[i] scalar assignment for strings', () => { + // A string param whose *name* looks indexed must not go down the + // vector path — only number components come from flattening. + const code = 'label = "Cup"; // 24'; + const param: Parameter = { + name: 'label', + displayName: 'Label', + value: 'Mug', + defaultValue: 'Cup', + type: 'string', + }; + assert.equal(updateParameter(code, param), 'label = "Mug"; // 24'); + }); +}); diff --git a/src/lib/utils.ts b/src/lib/utils.ts index ef0dc82d..c9e4c6b8 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -1,7 +1,9 @@ import { clsx, type ClassValue } from 'clsx'; import { twMerge } from 'tailwind-merge'; -import { Parameter } from '@shared/types'; -import { ModelConfig } from '../types/misc.ts'; +// Type-only imports so `node --test` can run this file without resolving +// the `@shared` path alias (node strips type imports but can't map aliases). +import type { Parameter } from '@shared/types'; +import type { ModelConfig } from '../types/misc.ts'; export function cn(...inputs: ClassValue[]) { return twMerge(clsx(inputs)); @@ -105,6 +107,21 @@ export function validateRedirectUrlServer( } export function updateParameter(code: string, param: Parameter): string { + // `size[1]`-style names come from `parseParameters` flattening a vector + // declaration into per-component sliders. The source still reads + // `size = [30, 20, 10];`, so rewrite the i-th element of the vector + // literal — the scalar regex below would find no `size[1] = ...;` + // assignment and silently no-op the edit. + const vectorComponent = /^(.+)\[(\d+)\]$/.exec(param.name); + if (vectorComponent && (!param.type || param.type === 'number')) { + return updateVectorComponent( + code, + vectorComponent[1], + Number(vectorComponent[2]), + param.value, + ); + } + const escapedName = escapeRegExp(param.name); const regex = new RegExp( `^\\s*(${escapedName}\\s*=\\s*)[^;]+;([\\t\\f\\cK ]*\\/\\/[^\n]*)?`, @@ -147,6 +164,31 @@ export function updateParameter(code: string, param: Parameter): string { } } +function updateVectorComponent( + code: string, + name: string, + index: number, + value: Parameter['value'], +): string { + // Single-line vectors only — `parseParameters` skips multi-line values, + // so a flattened `name[i]` parameter always came from a one-line literal. + const regex = new RegExp( + `^(\\s*${escapeRegExp(name)}\\s*=\\s*\\[)([^\\]\\n]*)(\\][^\\n]*)$`, + 'm', + ); + return code.replace( + regex, + (match, head: string, body: string, tail: string) => { + const items = body.split(','); + if (index < 0 || index >= items.length) return match; + const leading = /^\s*/.exec(items[index])?.[0] ?? ''; + const trailing = /\s*$/.exec(items[index].trimStart())?.[0] ?? ''; + items[index] = `${leading}${value}${trailing}`; + return `${head}${items.join(',')}${tail}`; + }, + ); +} + export function getDiffString(param: Parameter) { let diffString: string = ''; let diffNumber: number = 0; From 75e34e5627eb0fbbb70266c571127914efe00103 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Czachdive?= Date: Tue, 9 Jun 2026 15:21:30 -0700 Subject: [PATCH 3/4] feat(ui): wire parameter schema into editor, share view, and inputs EditorView and ShareView overlay artifact.parameters onto the parsed parameters via applyParameterSpecs. ParameterInput renders enum-style parameters (model-authored options or Customizer comment enums) as a select instead of free text, and shows the spec-authored unit in place of the name-based mm heuristic when present. Co-Authored-By: Claude Fable 5 --- src/components/parameter/ParameterInput.tsx | 72 ++++++++++++++++++++- src/views/EditorView.tsx | 25 ++++--- src/views/ShareView.tsx | 15 ++++- 3 files changed, 100 insertions(+), 12 deletions(-) diff --git a/src/components/parameter/ParameterInput.tsx b/src/components/parameter/ParameterInput.tsx index ea8ebe17..6795bae0 100644 --- a/src/components/parameter/ParameterInput.tsx +++ b/src/components/parameter/ParameterInput.tsx @@ -10,6 +10,13 @@ import { import { ParameterSlider } from '@/components/parameter/ParameterSlider'; import { Label } from '@/components/ui/label'; import { ColorPicker } from '@/components/parameter/ColorPicker'; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from '@/components/ui/select'; export function ParameterInput({ param, @@ -39,6 +46,65 @@ export function ParameterInput({ handleCommit(paramState, value); }; + // Enum-style parameters (model-authored `options` in the parameter spec, + // or Customizer `// [a, b, c]` comments) render as a select — free-text + // input here lets the user type values the OpenSCAD code can't handle. + const scalarOptions = + (!paramState.type || + paramState.type === 'number' || + paramState.type === 'string') && + Array.isArray(paramState.options) && + paramState.options.length > 0 + ? paramState.options + : null; + if (scalarOptions) { + const currentValue = String(paramState.value); + const hasCurrent = scalarOptions.some( + (option) => String(option.value) === currentValue, + ); + return ( +
+ + +
+ ); + } + if (!paramState.type || paramState.type === 'number') { return (
@@ -71,7 +137,8 @@ export function ParameterInput({ }} /> - {isMeasurementParameter(paramState) ? 'mm' : ''} + {paramState.unit ?? + (isMeasurementParameter(paramState) ? 'mm' : '')}
@@ -265,7 +332,8 @@ export function ParameterInput({ }} /> - {isMeasurementParameter(paramState) ? 'mm' : ''} + {paramState.unit ?? + (isMeasurementParameter(paramState) ? 'mm' : '')} diff --git a/src/views/EditorView.tsx b/src/views/EditorView.tsx index bed7cd7d..d1f51767 100644 --- a/src/views/EditorView.tsx +++ b/src/views/EditorView.tsx @@ -22,6 +22,7 @@ import { messageRowToChatMessage, type ChatMessage, } from '@/lib/aiMessages'; +import applyParameterSpecs from '@shared/applyParameterSpecs'; import parseParameters from '@shared/parseParameters'; import { supabase } from '@/lib/supabase'; import { updateParameter } from '@/lib/utils'; @@ -431,17 +432,25 @@ function ConversationEditor() { const handleViewArtifact = useCallback( (artifact: ParametricArtifact, messageId: string) => { baseCodeRef.current = artifact.code; - // Parameters are derived from the OpenSCAD source — same code always - // yields the same ``, no matter which model wrote - // it. The current values come from the (possibly edited) artifact - // code; the `defaultValue` (Reset target / slider home / auto range) - // comes from `metadata.originalCode` — the model's first-authored - // source — so an in-place parameter edit doesn't redefine the default - // on the next reload. Read from the cache to keep this callback stable. + // Parameter existence and current values are derived from the OpenSCAD + // source — same code always yields the same ``, no + // matter which model wrote it. The model-authored `artifact.parameters` + // schema then overlays presentation metadata (ranges, units, labels, + // groups, enum options); entries that don't match a real variable are + // dropped, so the parse validates the schema. The `defaultValue` + // (Reset target / slider home / auto range) comes from + // `metadata.originalCode` — the model's first-authored source — so an + // in-place parameter edit doesn't redefine the default on the next + // reload. Read from the cache to keep this callback stable. const originalCode = queryClient .getQueryData(['messages', conversation.id]) ?.find((row) => row.id === messageId)?.metadata?.originalCode; - setParameters(mergeParameterDefaults(artifact.code, originalCode)); + setParameters( + applyParameterSpecs( + mergeParameterDefaults(artifact.code, originalCode), + artifact.parameters, + ), + ); setCurrentOutput(undefined); setDxfExporter(() => null); setActivePreview({ type: 'artifact', messageId, artifact }); diff --git a/src/views/ShareView.tsx b/src/views/ShareView.tsx index 08a314c7..910aaac9 100644 --- a/src/views/ShareView.tsx +++ b/src/views/ShareView.tsx @@ -9,6 +9,7 @@ import { ConversationContext } from '@/contexts/ConversationContext'; import { messageRowToChatMessage } from '@/lib/aiMessages'; import { supabase } from '@/lib/supabase'; import { updateParameter } from '@/lib/utils'; +import applyParameterSpecs from '@shared/applyParameterSpecs'; import parseParameters from '@shared/parseParameters'; import type { AppUIMessage } from '@shared/chatAi'; import { isParametricArtifact } from '@shared/parametricParts'; @@ -156,7 +157,12 @@ function ConversationShare({ conversation, messages }: ConversationShareProps) { lastAutoAppliedPreviewKeyRef.current = key; if (latest.type === 'artifact') { baseCodeRef.current = latest.artifact.code; - setParameters(parseParameters(latest.artifact.code)); + setParameters( + applyParameterSpecs( + parseParameters(latest.artifact.code), + latest.artifact.parameters, + ), + ); setCurrentOutput(undefined); setActivePreview({ type: 'artifact', @@ -178,7 +184,12 @@ function ConversationShare({ conversation, messages }: ConversationShareProps) { const handleViewArtifact = useCallback( (artifact: ParametricArtifact, messageId: string) => { baseCodeRef.current = artifact.code; - setParameters(parseParameters(artifact.code)); + setParameters( + applyParameterSpecs( + parseParameters(artifact.code), + artifact.parameters, + ), + ); setCurrentOutput(undefined); setActivePreview({ type: 'artifact', messageId, artifact }); setMobilePreviewVersion((version) => version + 1); From 90fde6fcbe8d73ed238b19d62b96682437c483c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Czachdive?= Date: Tue, 9 Jun 2026 15:21:39 -0700 Subject: [PATCH 4/4] feat(server): require the parameter schema in the parametric agent prompt The agent contract now lists `parameters` as part of the artifact, with field-by-field guidance (ranges must bracket the default, units for physical values, options for enums, vector entries cover all components) and a worked example mirroring the mug. Customizer comments stay for downloaded .scad compatibility, with the schema as the UI's source of truth. Co-Authored-By: Claude Fable 5 --- src/server/aiChat.ts | 46 ++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/src/server/aiChat.ts b/src/server/aiChat.ts index 4fc2e5d7..c934e9e4 100644 --- a/src/server/aiChat.ts +++ b/src/server/aiChat.ts @@ -99,6 +99,9 @@ The build_parametric_model tool input is the artifact shown to the user: - title: short object name - version: "v1" - code: complete raw OpenSCAD code, no markdown, no code fences +- parameters: the typed schema for every user-editable variable in the code — + this drives the parameter panel (sliders, ranges, units, groups) the user + sees next to the live preview After you call build_parametric_model, the browser compiles the OpenSCAD and returns a multi-view preview sheet covering isometric, front, back, left, @@ -168,18 +171,26 @@ BOSL2 library guidance: Parameters: - Declare every editable parameter as a top-of-file variable. - Use full descriptive snake_case names (e.g. \`wheel_radius\`, \`seat_offset\`) — - never abbreviate to single letters or short tokens (\`w_r\`, \`p_s\`). Names - render directly in the parameter panel, so they must read well to the user. -- Annotate each variable with a trailing OpenSCAD Customizer comment so the - UI can render the right widget: - width = 50; // [10:1:200] ← min:step:max for sliders - height = 25; // [5:50] ← min:max - style = "round"; // [round, square, hex] ← enum options - enabled = true; // ← booleans render as switches - label = "Cup"; // 24 ← maxLength for free-form strings -- Optionally put a "// Description of the parameter" comment on the line - ABOVE the variable so the UI can show a description. -- Group related parameters with /* [Group Name] */ section markers. + never abbreviate to single letters or short tokens (\`w_r\`, \`p_s\`). +- Describe every editable variable in the tool's \`parameters\` array. It is + the source of truth for the parameter panel — one entry per variable: + { "name": "cup_height", "label": "Cup Height", "type": "number", + "min": 50, "max": 200, "step": 5, "unit": "mm", + "group": "Body", "description": "Outer height of the mug" } +- For numbers, always provide min/max/step spanning a sensible design range + around the default, and a unit ("mm", "deg") when the value is physical. +- For enum-style strings, list options with values exactly as they appear in + the code: + "options": [{ "value": "round", "label": "Round" }, { "value": "hex" }] +- min/max must bracket the variable's default value in the code. +- Every entry's \`name\` must match a top-of-file variable verbatim — entries + that don't are ignored. One entry on a vector variable + (e.g. \`size = [30, 20, 10];\`) covers all of its components. +- Also annotate each variable with a trailing OpenSCAD Customizer comment + (\`width = 50; // [10:1:200]\`) and group markers (\`/* [Group Name] */\`) + so downloaded .scad files stay editable in the OpenSCAD desktop + Customizer. The \`parameters\` array is what the app's UI uses; keep the + two consistent. Color: - When the model has distinct parts, wrap each in a color() call with a @@ -238,6 +249,17 @@ module torus(r1, r2) { circle(r=r2); } +and its \`parameters\` should look like: + +[ + { "name": "cup_height", "type": "number", "min": 50, "max": 200, "step": 5, "unit": "mm", "group": "Body", "description": "Outer height of the mug" }, + { "name": "cup_radius", "type": "number", "min": 20, "max": 80, "step": 1, "unit": "mm", "group": "Body" }, + { "name": "handle_radius", "type": "number", "min": 15, "max": 60, "step": 1, "unit": "mm", "group": "Handle" }, + { "name": "handle_thickness", "type": "number", "min": 4, "max": 20, "step": 1, "unit": "mm", "group": "Handle" }, + { "name": "wall_thickness", "type": "number", "min": 2, "max": 6, "step": 0.5, "unit": "mm", "group": "Body", "description": "Must stay printable" }, + { "name": "mug_color", "type": "string", "label": "Mug Color", "group": "Appearance" } +] + # What never to say Do not mention tools, APIs, prompts, or implementation details to the user.