From 9df295b9151d485d5f1ec04cddea8c78a84d43d0 Mon Sep 17 00:00:00 2001 From: John Dvorak Date: Fri, 22 May 2026 13:15:35 -0700 Subject: [PATCH] refactor: eliminate remaining hardcoded predicate-name dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extraction.ts (3 fixes): - Replace 2 'inStackingContext' string checks with isVariableArityPredicate() - Replace 7-name diagnostic formatting if/else with spec-driven getPredicateSpec() checks (isDirectional → gap message, isSize → threshold hint, else generic) Grammar.ts: Replace 8 hardcoded parser routing checks (atLeast/atMost/aspectRatio/between/clippedBy/attachedToScrollContainer/ escapeClippingChainOf/inStackingContext) with SIZE_PREDICATE_NAMES and TOPOLOGY_PREDICATE_NAMES Sets derived from spec table. Pipeline.ts: Replace 15-entry CODE_TO_CLAUSE_KIND map with runtime generation from PREDICATE_SPECS. Prefix derived from spec.isSize ('size.*') / validOptions.includes('axis') ('alignment.*') / else ('relation.*'). Manual override for aspectRatio code 15. Proofs.ts: Replace 11-case switch(kind) with 5 spec-driven if/else branches categorized by validOptions presence (hasGap→directional, hasAxis→alignment) + 2 specific name checks (inside overflow, aspectRatio ratio). 11 predicate names → 0 hardcoded. Lexer.ts: Export KEYWORDS map for conformance testing. Conformance tests: - Solver: every BUILTIN_PREDICATES entry matches its PREDICATE_SPECS counterpart; every spec name (incl. aliases) has a registered evaluator with matching descriptor (2 tests) - DSL: every predicate name from collectAllPredicateNames() appears in the lexer KEYWORDS table (1 test) 598 SDK + 3 conformance + 57 E2E = 658 tests pass. --- packages/imhotep-core/src/pipeline.ts | 35 ++- packages/imhotep-dsl/src/grammar.test.ts | 19 ++ packages/imhotep-dsl/src/grammar.ts | 15 +- packages/imhotep-dsl/src/lexer.ts | 2 +- packages/imhotep-playwright/src/extraction.ts | 23 +- .../imhotep-solver/src/predicates.test.ts | 53 ++++ packages/imhotep-solver/src/proofs.ts | 256 +++++++++--------- 7 files changed, 238 insertions(+), 165 deletions(-) diff --git a/packages/imhotep-core/src/pipeline.ts b/packages/imhotep-core/src/pipeline.ts index 52ce22c..166c2f6 100644 --- a/packages/imhotep-core/src/pipeline.ts +++ b/packages/imhotep-core/src/pipeline.ts @@ -106,6 +106,7 @@ import type { } from './diagnostics.js' import { createDiagnostic } from './diagnostics.js' +import { PREDICATE_SPECS } from './predicate-specs.js' // Adapter interfaces - implementations injected at runtime interface ExtractorRequest { @@ -433,25 +434,23 @@ export interface PipelineResult { /** * Reverse mapping from execution IR clause type codes to solver clause kinds. - * Mirrors the relationCodes table in imhotep-dsl/compiler.ts. + * Built from the predicate spec table — any predicate with a relationCode + * automatically gets a clause kind entry. */ -const CODE_TO_CLAUSE_KIND: Record = { - 1: 'relation.leftOf', - 2: 'relation.rightOf', - 3: 'relation.above', - 4: 'relation.below', - 5: 'alignment.alignedWith', - 6: 'alignment.leftAlignedWith', - 7: 'alignment.rightAlignedWith', - 8: 'alignment.topAlignedWith', - 9: 'alignment.bottomAlignedWith', - 10: 'alignment.centeredWithin', - 11: 'relation.inside', - 12: 'relation.contains', - 13: 'relation.overlaps', - 14: 'relation.separatedFrom', - 15: 'size.aspectRatio', -} +const CODE_TO_CLAUSE_KIND: Record = (() => { + const map: Record = {} + for (const spec of PREDICATE_SPECS) { + if (!spec.relationCode) continue + let prefix = 'relation' + if (spec.isSize) prefix = 'size' + else if (spec.validOptions.includes('axis')) prefix = 'alignment' + map[spec.relationCode] = `${prefix}.${spec.name}` + } + return map +})() +// DSL keyword 'aspectRatioBetween' uses code 15; maps to the canonical +// 'aspectRatio' predicate which has no separate relationCode. +CODE_TO_CLAUSE_KIND[15] = 'size.aspectRatio' // --------------------------------------------------------------------------- // Main Pipeline Orchestrator diff --git a/packages/imhotep-dsl/src/grammar.test.ts b/packages/imhotep-dsl/src/grammar.test.ts index c8536cb..a60367e 100644 --- a/packages/imhotep-dsl/src/grammar.test.ts +++ b/packages/imhotep-dsl/src/grammar.test.ts @@ -6,6 +6,8 @@ import assert from 'node:assert' import { parseSpec } from './parser.js' import { lowerToCanonical } from './lower-to-canonical.js' +import { KEYWORDS } from './lexer.js' +import { collectAllPredicateNames } from 'imhotep-core' function parse(source: string) { return parseSpec(source) @@ -449,3 +451,20 @@ describe('inline state attachments', () => { assert.strictEqual(top.state.kind, 'expanded') }) }) + +// --------------------------------------------------------------------------- +// Conformance: PREDICATE_SPECS ↔ Lexer KEYWORDS sync +// --------------------------------------------------------------------------- + +describe('PredicateSpec ↔ Lexer keyword conformance', () => { + it('every predicate name from PREDICATE_SPECS has a lexer keyword entry', () => { + const allNames = collectAllPredicateNames() + const missing: string[] = [] + for (const name of allNames) { + if (!(name in KEYWORDS)) { + missing.push(name) + } + } + assert.deepStrictEqual(missing, [], `Predicate names missing from lexer KEYWORDS: ${missing.join(', ')}`) + }) +}) diff --git a/packages/imhotep-dsl/src/grammar.ts b/packages/imhotep-dsl/src/grammar.ts index b8c2fdc..ae6d076 100644 --- a/packages/imhotep-dsl/src/grammar.ts +++ b/packages/imhotep-dsl/src/grammar.ts @@ -27,10 +27,14 @@ import type { ToleranceLiteralNode, } from 'imhotep-core' -import { isPredicateName, collectSpatialPredicateNames } from 'imhotep-core' +import { isPredicateName, collectSpatialPredicateNames, collectSizePredicateNames, collectTopologyPredicateNames } from 'imhotep-core' import type { Token } from './lexer.js' +// Cached predicate name sets derived from the spec table. +const SIZE_PREDICATE_NAMES = new Set(collectSizePredicateNames()) +const TOPOLOGY_PREDICATE_NAMES = new Set(collectTopologyPredicateNames()) + // --------------------------------------------------------------------------- // Extended TokenKind for grammar checks // --------------------------------------------------------------------------- @@ -1002,14 +1006,13 @@ export class GrammarParser { private parseClause(subject: SelectorRef, start: Point, negated: boolean): any | null { let assertion: any - // Size assertion - if (this.check('atLeast') || this.check('atMost') || this.check('aspectRatio') || this.check('between')) { + // Dispatch assertion type based on predicate category from spec table. + const kind = this.currentToken().kind as string + if (SIZE_PREDICATE_NAMES.has(kind)) { assertion = this.parseSizeAssertion(subject, start) - } else if (this.check('clippedBy') || this.check('attachedToScrollContainer') || this.check('escapeClippingChainOf') || this.check('inStackingContext')) { - // Topology assertion + } else if (TOPOLOGY_PREDICATE_NAMES.has(kind)) { assertion = this.parseTopologyAssertion(subject, start) } else { - // Relation assertion assertion = this.parseRelationAssertion(subject, start) } diff --git a/packages/imhotep-dsl/src/lexer.ts b/packages/imhotep-dsl/src/lexer.ts index 2f21897..4d88a28 100644 --- a/packages/imhotep-dsl/src/lexer.ts +++ b/packages/imhotep-dsl/src/lexer.ts @@ -143,7 +143,7 @@ export interface Token { // Keywords Map // --------------------------------------------------------------------------- -const KEYWORDS: Record = { +export const KEYWORDS: Record = { in: 'in', across: 'across', always: 'always', diff --git a/packages/imhotep-playwright/src/extraction.ts b/packages/imhotep-playwright/src/extraction.ts index 8bbc34b..c82adbb 100644 --- a/packages/imhotep-playwright/src/extraction.ts +++ b/packages/imhotep-playwright/src/extraction.ts @@ -56,6 +56,8 @@ import { getPredicateDiagnosticCode, getPredicateDecomposition, isUnaryPredicate, + isVariableArityPredicate, + getPredicateSpec, } from 'imhotep-core' import { buildGeometryWorld } from './world-builder.js' import { @@ -1206,7 +1208,7 @@ export function compileCanonicalClauseToFormula(clause: CanonicalClauseDescripto } } else { const unaryPredicate = isUnaryPredicate(clause.relation) - && !(clause.relation === 'inStackingContext' && clause.reference) + && !(isVariableArityPredicate(clause.relation) && clause.reference) body = { type: 'FormulaNode', @@ -1251,7 +1253,7 @@ export function compileCanonicalClauseToFormula(clause: CanonicalClauseDescripto } const isUnary = isUnaryPredicate(clause.relation) - && !(clause.relation === 'inStackingContext' && clause.reference) + && !(isVariableArityPredicate(clause.relation) && clause.reference) if (isUnary) { return { @@ -1645,19 +1647,12 @@ export function mapFolDiagnostic( : 'min' const expectedGap = gapKind === 'min' ? (minGap ?? 0) : (maxGap ?? 0) const boundDescription = gapKind === 'min' ? 'minimum required gap' : 'maximum required gap' - if (predicateName === 'leftOf' && observedGap !== undefined) { - message = `leftOf assertion failed: measured gap is ${observedGap.toFixed(0)}px, but ${boundDescription} is ${expectedGap}px.` + const spec = predicateName ? getPredicateSpec(predicateName) : undefined + const isDirectional = spec?.validOptions.includes('minGap') ?? false + if (isDirectional && observedGap !== undefined) { + message = `${predicateName} assertion failed: measured gap is ${observedGap.toFixed(0)}px, but ${boundDescription} is ${expectedGap}px.` fixHints.push(`The measured gap is ${observedGap.toFixed(0)}px. Consider ${gapKind === 'min' ? 'lowering minGap' : 'increasing maxGap'} or checking element positions.`) - } else if (predicateName === 'above' && observedGap !== undefined) { - message = `above assertion failed: measured gap is ${observedGap.toFixed(0)}px, but ${boundDescription} is ${expectedGap}px.` - fixHints.push(`The measured gap is ${observedGap.toFixed(0)}px. Consider ${gapKind === 'min' ? 'lowering minGap' : 'increasing maxGap'} or checking element positions.`) - } else if (predicateName === 'below' && observedGap !== undefined) { - message = `below assertion failed: measured gap is ${observedGap.toFixed(0)}px, but ${boundDescription} is ${expectedGap}px.` - fixHints.push(`The measured gap is ${observedGap.toFixed(0)}px. Consider ${gapKind === 'min' ? 'lowering minGap' : 'increasing maxGap'} or checking element positions.`) - } else if (predicateName === 'rightOf' && observedGap !== undefined) { - message = `rightOf assertion failed: measured gap is ${observedGap.toFixed(0)}px, but ${boundDescription} is ${expectedGap}px.` - fixHints.push(`The measured gap is ${observedGap.toFixed(0)}px. Consider ${gapKind === 'min' ? 'lowering minGap' : 'increasing maxGap'} or checking element positions.`) - } else if (predicateName === 'atLeast' || predicateName === 'atMost' || predicateName === 'between') { + } else if (spec?.isSize) { fixHints.push(`Check the expected size threshold and the actual element dimensions using ui.extract(selector).`) } else { fixHints.push(`Verify the expected layout and consider adjusting thresholds.`) diff --git a/packages/imhotep-solver/src/predicates.test.ts b/packages/imhotep-solver/src/predicates.test.ts index fc035a6..0c8c739 100644 --- a/packages/imhotep-solver/src/predicates.test.ts +++ b/packages/imhotep-solver/src/predicates.test.ts @@ -17,8 +17,10 @@ import { clearPredicateRegistry, registerDefaultPredicates, getPredicateEvaluator, + BUILTIN_PREDICATES, type GeometryWorld, } from './index.js'; +import { PREDICATE_SPECS, getAllPredicateSpecs, getPredicateSpec } from 'imhotep-core'; // --- Test helpers ------------------------------------------------------------ @@ -683,4 +685,55 @@ describe('spatial alias predicates', () => { assert.ok(Math.abs((result.metrics?.gap ?? 0) - Math.sqrt(200)) < 0.0001); }); }); + + // --------------------------------------------------------------------------- + // Conformance: PREDICATE_SPECS ↔ BUILTIN_PREDICATES sync + // --------------------------------------------------------------------------- + + describe('PredicateSpec ↔ BUILTIN_PREDICATES conformance', () => { + it('every BUILTIN_PREDICATES entry has a matching PREDICATE_SPECS entry', () => { + for (const bp of BUILTIN_PREDICATES) { + const spec = getPredicateSpec(bp.name) + assert.ok(spec, `BUILTIN_PREDICATES entry "${bp.name}" not found in PREDICATE_SPECS`) + assert.strictEqual( + spec.arity === 'variable' ? 2 : spec.arity, + bp.arity, + `Arity mismatch for "${bp.name}": PREDICATE_SPECS has ${spec.arity}, BUILTIN_PREDICATES has ${bp.arity}`, + ) + for (const fact of bp.requiredFacts) { + assert.ok( + spec.requiredFacts.includes(fact), + `Required fact "${fact}" for "${bp.name}" is in BUILTIN_PREDICATES but not in PREDICATE_SPECS`, + ) + } + } + }) + + it('every registered predicate has an evaluator with matching descriptor', () => { + registerDefaultPredicates() + // Collect all predicate names from spec table (including aliases as separate evaluator names). + const specNames = new Set() + for (const spec of PREDICATE_SPECS) { + specNames.add(spec.name) + for (const alias of spec.aliases) specNames.add(alias) + } + for (const name of specNames) { + const evaluator = getPredicateEvaluator(name) + assert.ok(evaluator, `No evaluator registered for predicate "${name}" from PREDICATE_SPECS`) + const spec = getPredicateSpec(name) + if (spec) { + const desc = evaluator.descriptor + assert.strictEqual(desc.name, name, `Evaluator name mismatch for "${name}"`) + const expectedArity = spec.arity === 'variable' ? 2 : spec.arity + assert.strictEqual(desc.arity, expectedArity, `Evaluator arity mismatch for "${name}"`) + for (const fact of spec.requiredFacts) { + assert.ok( + desc.requiredFacts.includes(fact), + `Required fact "${fact}" for "${name}" is in PREDICATE_SPECS but missing from evaluator descriptor`, + ) + } + } + } + }) + }) }); diff --git a/packages/imhotep-solver/src/proofs.ts b/packages/imhotep-solver/src/proofs.ts index af81457..aac024b 100644 --- a/packages/imhotep-solver/src/proofs.ts +++ b/packages/imhotep-solver/src/proofs.ts @@ -16,6 +16,8 @@ import { type GeometryWorld, } from './registry.js'; +import { getPredicateSpec } from 'imhotep-core' + let proofCounter = 0; function nextProofId(): string { @@ -49,134 +51,136 @@ function buildFailedPredicate( relationKind: kind, }; - switch (kind) { - case 'leftOf': - case 'rightOf': - case 'above': - case 'below': { - const gap = metrics.observedGap ?? metrics.gap ?? 0; - const min = metrics.minGap ?? 0; - const max = metrics.maxGap ?? Infinity; - return { - ...base, - op: gap < min ? '<' : '>', - left: gap, - right: gap < min ? min : max, - measuredGap: gap, - expectedMinGap: Number.isFinite(min) ? min : undefined, - expectedMaxGap: Number.isFinite(max) ? max : undefined, - subjectRect: - metrics.subjectLeft !== undefined - ? { - left: metrics.subjectLeft, - top: metrics.subjectTop ?? 0, - right: metrics.subjectRight ?? 0, - bottom: metrics.subjectBottom ?? 0, - } - : undefined, - referenceRect: - metrics.refLeft !== undefined - ? { - left: metrics.refLeft, - top: metrics.refTop ?? 0, - right: metrics.refRight ?? 0, - bottom: metrics.refBottom ?? 0, - } - : undefined, - }; - } + const spec = getPredicateSpec(kind) + // Determine diagnostic shape family from spec metadata. + const hasGap = spec?.validOptions.includes('minGap') && spec?.validOptions.includes('maxGap') + const hasAxis = spec?.validOptions.includes('axis') + const isSize = spec?.isSize - case 'inside': { - const overflowLeft = metrics.overflowLeft ?? 0; - const overflowTop = metrics.overflowTop ?? 0; - const overflowRight = metrics.overflowRight ?? 0; - const overflowBottom = metrics.overflowBottom ?? 0; - return { - ...base, - op: 'not-contained', - left: 0, - right: 0, - overflowEdges: { - left: overflowLeft, - top: overflowTop, - right: overflowRight, - bottom: overflowBottom, - }, - subjectRect: - metrics.subjectLeft !== undefined - ? { - left: metrics.subjectLeft, - top: metrics.subjectTop ?? 0, - right: metrics.subjectRight ?? 0, - bottom: metrics.subjectBottom ?? 0, - } - : undefined, - referenceRect: - metrics.refLeft !== undefined - ? { - left: metrics.refLeft, - top: metrics.refTop ?? 0, - right: metrics.refRight ?? 0, - bottom: metrics.refBottom ?? 0, - } - : undefined, - }; - } - - case 'atLeast': - case 'atMost': - case 'between': { - const observed = metrics.observed ?? metrics.value ?? 0; - const min = metrics.min ?? -Infinity; - const max = metrics.max ?? Infinity; - return { - ...base, - op: kind === 'atMost' ? '>' : '<', - left: observed, - right: kind === 'atMost' ? max : min, - measuredValue: observed, - expectedMin: Number.isFinite(min) ? min : undefined, - expectedMax: Number.isFinite(max) ? max : undefined, - }; - } - - case 'aspectRatio': { - const observed = metrics.observed ?? 0; - const minRatio = metrics.minRatio ?? -Infinity; - const maxRatio = metrics.maxRatio ?? Infinity; - return { - ...base, - op: observed < minRatio ? '<' : '>', - left: observed, - right: observed < minRatio ? minRatio : maxRatio, - measuredValue: observed, - expectedMin: Number.isFinite(minRatio) ? minRatio : undefined, - expectedMax: Number.isFinite(maxRatio) ? maxRatio : undefined, - }; - } - - case 'alignedWith': - case 'centeredWithin': { - const delta = metrics.delta ?? metrics.deltaX ?? metrics.deltaY ?? 0; - const tolerance = metrics.tolerance ?? 0; - return { - ...base, - op: '>', - left: delta, - right: tolerance, - measuredValue: delta, - expectedMax: tolerance, - }; - } - - default: - // Fallback to generic synthesis from the first two numeric metrics. - const fallback = synthesizeGenericFailedPredicate(metrics); - if (fallback) { - return { ...fallback, relationKind: kind }; - } - return fallback; + // --- Directional gap (leftOf / rightOf / above / below) --- + if (hasGap) { + const gap = metrics.observedGap ?? metrics.gap ?? 0; + const min = metrics.minGap ?? 0; + const max = metrics.maxGap ?? Infinity; + return { + ...base, + op: gap < min ? '<' : '>', + left: gap, + right: gap < min ? min : max, + measuredGap: gap, + expectedMinGap: Number.isFinite(min) ? min : undefined, + expectedMaxGap: Number.isFinite(max) ? max : undefined, + subjectRect: + metrics.subjectLeft !== undefined + ? { + left: metrics.subjectLeft, + top: metrics.subjectTop ?? 0, + right: metrics.subjectRight ?? 0, + bottom: metrics.subjectBottom ?? 0, + } + : undefined, + referenceRect: + metrics.refLeft !== undefined + ? { + left: metrics.refLeft, + top: metrics.refTop ?? 0, + right: metrics.refRight ?? 0, + bottom: metrics.refBottom ?? 0, + } + : undefined, + }; } + + // --- Containment / inside (unique overflow metrics) --- + if (kind === 'inside') { + const overflowLeft = metrics.overflowLeft ?? 0; + const overflowTop = metrics.overflowTop ?? 0; + const overflowRight = metrics.overflowRight ?? 0; + const overflowBottom = metrics.overflowBottom ?? 0; + return { + ...base, + op: 'not-contained', + left: 0, + right: 0, + overflowEdges: { + left: overflowLeft, + top: overflowTop, + right: overflowRight, + bottom: overflowBottom, + }, + subjectRect: + metrics.subjectLeft !== undefined + ? { + left: metrics.subjectLeft, + top: metrics.subjectTop ?? 0, + right: metrics.subjectRight ?? 0, + bottom: metrics.subjectBottom ?? 0, + } + : undefined, + referenceRect: + metrics.refLeft !== undefined + ? { + left: metrics.refLeft, + top: metrics.refTop ?? 0, + right: metrics.refRight ?? 0, + bottom: metrics.refBottom ?? 0, + } + : undefined, + }; + } + + // --- Size threshold (atLeast / atMost / between) --- + if (isSize && kind !== 'aspectRatio') { + const observed = metrics.observed ?? metrics.value ?? 0; + const min = metrics.min ?? -Infinity; + const max = metrics.max ?? Infinity; + return { + ...base, + op: kind === 'atMost' ? '>' : '<', + left: observed, + right: kind === 'atMost' ? max : min, + measuredValue: observed, + expectedMin: Number.isFinite(min) ? min : undefined, + expectedMax: Number.isFinite(max) ? max : undefined, + }; + } + + // --- Aspect ratio --- + if (kind === 'aspectRatio') { + const observed = metrics.observed ?? 0; + const minRatio = metrics.minRatio ?? -Infinity; + const maxRatio = metrics.maxRatio ?? Infinity; + return { + ...base, + op: observed < minRatio ? '<' : '>', + left: observed, + right: observed < minRatio ? minRatio : maxRatio, + measuredValue: observed, + expectedMin: Number.isFinite(minRatio) ? minRatio : undefined, + expectedMax: Number.isFinite(maxRatio) ? maxRatio : undefined, + }; + } + + // --- Alignment (alignedWith / centeredWithin) --- + if (hasAxis || kind === 'centeredWithin') { + const delta = metrics.delta ?? metrics.deltaX ?? metrics.deltaY ?? 0; + const tolerance = metrics.tolerance ?? 0; + return { + ...base, + op: '>', + left: delta, + right: tolerance, + measuredValue: delta, + expectedMax: tolerance, + }; + } + + // Fallback to generic synthesis from the first two numeric metrics. + const fallback = synthesizeGenericFailedPredicate(metrics); + if (fallback) { + return { ...fallback, relationKind: kind }; + } + return fallback; } function synthesizeGenericFailedPredicate(