fix: Reverse engineer items from #34

plugins/reverse/app/projectSection.ts

@@ -165,7 +165,7 @@ export function validateReversePart(raw: unknown): ReversePart {
   const r = object(raw, 'reverse')
   fields(r, 'nextFeatureId', 'number', 'reverse')
   if (r.version !== undefined && (typeof r.version !== 'number' || !Number.isFinite(r.version))) invalid('reverse.version')
-  object(r.nextOfKind, 'reverse.nextOfKind')
+  for (const n of Object.values(object(r.nextOfKind, 'reverse.nextOfKind'))) if (!Number.isSafeInteger(n)) invalid('reverse.nextOfKind')
   entries(r.features, 'reverse.features', validateFeatureJson)
   if (r.rolledTo !== undefined && (!Number.isSafeInteger(r.rolledTo) || (r.rolledTo as number) < 0 || (r.rolledTo as number) > (r.features as unknown[]).length)) invalid('reverse.rolledTo')
   if (r.parameters !== undefined) {
@@ -209,6 +209,12 @@ function importedKeys(features: readonly Feature[]): string[] {
   return [...new Set(features.flatMap((f) => (f.kind === 'import' ? [f.file.key] : [])))]
 }
 
+/** The STEP member's bytes as last encoded, by their text. The snapshot that
+ *  tells a saved project from a changed one keys members by the identity of
+ *  their bytes, so the same texts must come back the same bytes, or the
+ *  project never reads as saved and every checkpoint writes them again. */
+let stepBytes: { json: string; bytes: Uint8Array } | null = null
+
 /** The workspace as the file is to hold it, or undefined with no feature —
  *  with `ctx`, the STEP files its Imports name put into the archive
  *  beside it. */
@@ -217,9 +223,13 @@ export function collectReversePart(ctx?: Pick<CollectContext, 'addMember'>): Rev
   const all = [...re.features, ...re.rolledBack]
   if (all.length === 0) return undefined
   const keys = importedKeys(all)
-  const texts = Object.fromEntries(keys.flatMap((k) => (re.stepFiles[k] !== undefined ? [[k, re.stepFiles[k]]] : [])))
+  const texts = Object.fromEntries(keys.flatMap((k) => (Object.hasOwn(re.stepFiles, k) ? [[k, re.stepFiles[k]]] : [])))
   const steps = ctx && Object.keys(texts).length > 0
-  if (steps) ctx.addMember(STEP_MEMBER, strToU8(JSON.stringify(texts)))
+  if (steps) {
+    const json = JSON.stringify(texts)
+    if (stepBytes?.json !== json) stepBytes = { json, bytes: strToU8(json) }
+    ctx.addMember(STEP_MEMBER, stepBytes.bytes)
+  }
   // A playback moves the bar for the camera: the file has it where it stood.
   const rolledTo = re.playback?.origin ?? re.features.length
   // The lasting set: bodies a failed build or a rollback lacks now too.
@@ -316,6 +326,14 @@ export function migrateFeatures(features: readonly Feature[], version: number):
   return out
 }
 
+/** The next id to give, from the one a file holds: past every id already
+ *  taken whatever the file says, so a hand-edited or hostile counter cannot
+ *  hand out an id twice and make one removal take two features. */
+function nextIdPast(given: number | undefined, taken: readonly number[]): number {
+  const past = taken.reduce((m, id) => (Number.isFinite(id) ? Math.max(m, Math.floor(id) + 1) : m), 1)
+  return given !== undefined && Number.isSafeInteger(given) && given > past ? given : past
+}
+
 /** The features as the store holds them, split at the rollback bar. */
 function reversePartState(p: ReversePart | undefined, ctx?: RestoreContext) {
   if (!p) return null
@@ -326,10 +344,10 @@ function reversePartState(p: ReversePart | undefined, ctx?: RestoreContext) {
   return {
     features: all.slice(0, at),
     rolledBack: all.slice(at),
-    nextFeatureId: p.nextFeatureId,
+    nextFeatureId: nextIdPast(p.nextFeatureId, all.map((f) => f.id)),
     nextOfKind: p.nextOfKind,
     parameters,
-    nextParameterId: p.nextParameterId ?? parameters.reduce((m, q) => Math.max(m, q.id + 1), 1),
+    nextParameterId: nextIdPast(p.nextParameterId, parameters.map((q) => q.id)),
     parameterTable: evaluateParameters(parameters),
     bodyStyles: Object.fromEntries(Object.entries(p.bodyStyles ?? {}).map(([id, st]) => [Number(id), st])) as BodyStyles,
     hiddenBodies: p.hiddenBodies ?? [],

plugins/reverse/app/reSession.ts

@@ -15,6 +15,7 @@ import { fmtLen, fmtVol, type DisplayUnits } from '../../../src/core/units'
 import type { Vec3 } from '../../../src/core/types'
 import type { RefObject } from 'react'
 import { exportStem } from '../../../src/app/exports'
+import { onScan } from '../../../src/app/scanEvents'
 import { commandHost } from '../../../src/commands/host'
 import { useStore } from '../../../src/state/store'
 import type { SceneManager } from '../../../src/viewer/SceneManager'
@@ -36,6 +37,16 @@ import { kernelUnavailableReason } from '../core/wasmSupport'
 import { bodyColor, featuresToBuild, useReverse } from '../state/reStore'
 import { regionOf } from './reSurfaceRegion'
 
+/** How many times a scan has been loaded or swapped for another version of
+ *  itself. A smoothed or offset copy keeps the file's name and its triangle
+ *  count, so the kernel's copy of the scan is told stale by this, not by
+ *  what the store says of the scan. */
+let scanChanges = 0
+onScan({
+  loaded: () => void scanChanges++,
+  swapped: () => void scanChanges++,
+})
+
 const message = (e: unknown) => (e instanceof Error ? e.message : String(e))
 
 /** How long builds stay held back (session.hold) after the last change
@@ -83,7 +94,9 @@ export function meshDiagonal(mesh: KernelMesh): number {
 export function stepFilesFor(steps: readonly { kind: string; file?: unknown }[]): Map<string, string> {
   const stored = useReverse.getState().stepFiles
   const out = new Map<string, string>()
-  for (const s of steps) if (s.kind === 'import' && typeof s.file === 'string' && stored[s.file] !== undefined) out.set(s.file, stored[s.file])
+  // Own keys only: a key a project file gives may be `constructor` or the
+  // like, which a plain object answers from its prototype.
+  for (const s of steps) if (s.kind === 'import' && typeof s.file === 'string' && Object.hasOwn(stored, s.file)) out.set(s.file, stored[s.file])
   return out
 }
 
@@ -269,13 +282,22 @@ export function createReverseSession({
     // A build of a kernel a stop let go answers nothing: the stop took it
     // off the count.
     const c = client
+    // Taken off the count once only: a throw while the answer is written
+    // into the store lands in the catch below, which must not count it off
+    // a second time.
+    let counted = true
+    const uncount = () => {
+      if (!counted) return
+      counted = false
+      running--
+      kernelBuilds--
+    }
     c.build(steps, sketches, stepFilesFor(steps))
       .then((answer) => {
         if (c !== client) return
         // Not running any more before the store says what landed, so that
         // what waits on the store finds the session idle when it looks.
-        running--
-        kernelBuilds--
+        uncount()
         if (!answer) return
         // A body the build left as it was comes back the very object it
         // was, so what the viewport made of its mesh stays.
@@ -304,8 +326,7 @@ export function createReverseSession({
       })
       .catch((e) => {
         if (c !== client) return
-        running--
-        kernelBuilds--
+        uncount()
         landed = version
         useReverse.getState().failBuild(message(e))
       })
@@ -339,8 +360,11 @@ export function createReverseSession({
         }
         return { features: s.features.map(patch), rolledBack: s.rolledBack.map(patch), buildVersion: s.buildVersion + 1 }
       })
-      running--
     })
+      // Off the count whatever happened, or a wait for the build would wait
+      // out its whole limit for a read that is over.
+      .catch((e) => useReverse.getState().failBuild(message(e)))
+      .finally(() => running--)
   }
 
   const schedule = () => {
@@ -589,7 +613,7 @@ export function createReverseSession({
   // The viewport's vertices, every datum alignment baked in — the frame the
   // bodies are built in — handed over once per file and once more per
   // alignment.
-  let scanSent: { fileName: string | null; triangles: number; alignment: unknown } | null = null
+  let scanSent: { fileName: string | null; triangles: number; alignment: unknown; changes: number } | null = null
   let scanSending: Promise<void> | null = null
 
   /** The scan's vertices without a viewport to read them off — every one
@@ -618,8 +642,8 @@ export function createReverseSession({
     if (!client || useReverse.getState().kernelStatus !== 'ready') return 'none'
     const s = useStore.getState()
     if (!scene && !s.fileName) return 'none'
-    const now = { fileName: s.fileName, triangles: s.triangleCount, alignment: s.appliedAlignment }
-    if (scanSent && scanSent.fileName === now.fileName && scanSent.triangles === now.triangles && scanSent.alignment === now.alignment) {
+    const now = { fileName: s.fileName, triangles: s.triangleCount, alignment: s.appliedAlignment, changes: scanChanges }
+    if (scanSent && scanSent.fileName === now.fileName && scanSent.triangles === now.triangles && scanSent.alignment === now.alignment && scanSent.changes === now.changes) {
       if (scanSending) await scanSending
       return 'same'
     }

plugins/reverse/core/expression.ts

@@ -153,8 +153,18 @@ const BINARY_POWER: Record<'+' | '-' | '*' | '/' | '^', number> = { '+': 10, '-'
 /** Unary minus binds under a power, so −2² is −4, and over a product. */
 const NEGATE_POWER = 25
 
+/** How deep an expression may nest, in parentheses, functions and signs:
+ *  far past anything typed, and short of the stack the parser and the
+ *  evaluation recurse on, which an expression from a file could exhaust. */
+const MAX_DEPTH = 200
+
+/** The longest expression read, in characters: a chain of operators is
+ *  evaluated as deep as it is long. */
+const MAX_LENGTH = 4000
+
 class Parser {
   private i = 0
+  private depth = 0
   constructor(private tokens: Token[]) {}
 
   parse(): Node {
@@ -178,6 +188,15 @@ class Parser {
   }
 
   private expression(minPower: number): Node {
+    if (++this.depth > MAX_DEPTH) throw new ExpressionError('nests too deeply', this.peek().at)
+    try {
+      return this.operators(minPower)
+    } finally {
+      this.depth--
+    }
+  }
+
+  private operators(minPower: number): Node {
     let left = this.prefix()
     for (;;) {
       const t = this.peek()
@@ -426,13 +445,17 @@ function call(n: Extract<Node, { kind: 'call' }>, scope: Scope): Value {
 /** Evaluate an expression against a scope, to whatever kind it comes out
  *  as. */
 export function evaluate(src: string, scope: Scope): Evaluated {
+  if (src.length > MAX_LENGTH) return { ok: false, error: `is longer than ${MAX_LENGTH} characters`, at: [0, src.length] }
   try {
     const node = new Parser(tokenize(src)).parse()
     const v = evaluateNode(node, scope)
     if (!Number.isFinite(v.value)) return { ok: false, error: 'does not come out as a number', at: node.at }
     return { ok: true, value: v.value, dim: v.dim }
   } catch (e) {
     if (e instanceof ExpressionError) return { ok: false, error: e.message, at: e.at }
+    // Parameters reading each other in a chain long enough to run out of
+    // stack: an error of the expression, not of the page.
+    if (e instanceof RangeError) return { ok: false, error: 'reads parameters too deeply to evaluate', at: [0, src.length] }
     throw e
   }
 }

plugins/reverse/core/parameters.ts

@@ -147,6 +147,26 @@ export function evaluateParameters(parameters: readonly Parameter[]): ParameterT
     readings.set(p.id, reading)
     return reading
   }
+  // Read first those whose parameters are read already, the ones they read
+  // found without recursion: a long chain of parameters each reading the
+  // next is then read one link at a time, not as deep as it is long. One
+  // in a loop is left for the reading below, which says where it goes round.
+  const ordered = new Set<Parameter>()
+  for (const root of parameters) {
+    const stack: [Parameter, boolean][] = [[root, false]]
+    while (stack.length > 0) {
+      const [p, expanded] = stack.pop()!
+      const reads = references(p.expression).flatMap((name) => byName.get(name) ?? [])
+      if (expanded) {
+        if (reads.every((q) => readings.has(q.id))) read(p)
+        continue
+      }
+      if (ordered.has(p)) continue
+      ordered.add(p)
+      stack.push([p, true])
+      for (const q of reads) if (!ordered.has(q)) stack.push([q, false])
+    }
+  }
   const list = parameters.map(read)
   const byReadName = new Map<string, ParameterReading>()
   for (const r of list) if (!byReadName.has(r.name)) byReadName.set(r.name, r)

plugins/reverse/core/reHelper.ts

@@ -49,7 +49,12 @@ export function createHelperService(loadKernel: () => Promise<ReKernel>, tell?:
   return {
     async handle(msg: HelperRequest): Promise<{ response: HelperResponse; transfer: Transferable[] }> {
       try {
-        const k = await (kernel ??= loadKernel())
+        // A load that failed is not kept: the next request tries again, or
+        // this helper's share of every build would be lost until a reload.
+        const k = await (kernel ??= loadKernel().catch((e) => {
+          kernel = null
+          throw e
+        }))
         if (msg.type === 'init') return { response: { type: 'init-ok', requestId: msg.requestId }, transfer: [] }
         for (const [group, b] of builders) {
           if (msg.keep.includes(group)) continue

plugins/reverse/state/reStore.ts

@@ -567,7 +567,7 @@ export const useReverse = create<ReState>()((set, get) => ({
   stepFiles: {},
   addStepFile: (name, text) => {
     const file: StepFileRef = { key: stepFileKey(text), name, size: text.length }
-    if (get().stepFiles[file.key] === undefined) set((s) => ({ stepFiles: { ...s.stepFiles, [file.key]: text } }))
+    if (!Object.hasOwn(get().stepFiles, file.key)) set((s) => ({ stepFiles: { ...s.stepFiles, [file.key]: text } }))
     return file
   },
   setBodyStyle: (id, patch) =>

plugins/reverse/tests/reExpressionDepth.test.ts

@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+// Expressions and parameter tables deep enough to run out of stack, as a
+// project file can hold them: an error of the expression, never a throw.
+import { describe, expect, it } from 'vitest'
+import { evaluate } from '../core/expression'
+import { evaluateParameters, type Parameter } from '../core/parameters'
+
+describe('a deep expression', () => {
+  it('is refused, not thrown', () => {
+    const deep = '('.repeat(200_000) + '1' + ')'.repeat(200_000)
+    const r = evaluate(deep, () => undefined)
+    expect(r.ok).toBe(false)
+    const nested = '('.repeat(1000) + '1' + ')'.repeat(1000)
+    expect(evaluate(nested, () => undefined)).toMatchObject({ ok: false, error: 'nests too deeply' })
+    expect(evaluate('-'.repeat(1000) + '1', () => undefined)).toMatchObject({ ok: false, error: 'nests too deeply' })
+  })
+})
+
+describe('a long chain of parameters', () => {
+  it('reads to the end without running out of stack', () => {
+    const n = 30_000
+    const chain: Parameter[] = Array.from({ length: n }, (_, i) => ({ id: i + 1, name: `p${i}`, kind: 'number', expression: i === 0 ? '1' : `p${i - 1} + 1` }))
+    const table = evaluateParameters(chain.reverse())
+    expect(table.readings[0]).toMatchObject({ name: `p${n - 1}`, value: n, error: null })
+  })
+
+  it('still says where a loop goes round', () => {
+    const table = evaluateParameters([
+      { id: 1, name: 'a', kind: 'number', expression: 'b + 1' },
+      { id: 2, name: 'b', kind: 'number', expression: 'a + 1' },
+      { id: 3, name: 'c', kind: 'number', expression: '2' },
+    ])
+    expect(table.readings.map((r) => r.error)).toEqual(['goes round in a loop: a → b → a', 'goes round in a loop: a → b → a', null])
+  })
+})

plugins/reverse/tests/reHelperLoad.test.ts

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+// A helper kernel whose load fails once: the next request loads it again.
+import { describe, expect, it } from 'vitest'
+import { createHelperService } from '../core/reHelper'
+import type { ReKernel } from '../core/kernel'
+
+describe('a helper kernel', () => {
+  it('tries the load again after one that failed', async () => {
+    let loads = 0
+    const service = createHelperService(async () => {
+      if (++loads === 1) throw new Error('the network dropped')
+      return {} as ReKernel
+    })
+    const first = await service.handle({ type: 'init', requestId: 1 })
+    expect(first.response).toMatchObject({ type: 'error', message: 'the network dropped' })
+    const second = await service.handle({ type: 'init', requestId: 2 })
+    expect(second.response).toEqual({ type: 'init-ok', requestId: 2 })
+    expect(loads).toBe(2)
+  })
+})

plugins/reverse/tests/reProjectHostile.test.ts

@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+// The Reverse engineer part of a project as a file shapes it: what a saved
+// project reads back as, and what a hostile file cannot make of the store.
+import { beforeEach, describe, expect, it } from 'vitest'
+import { snapshotKey } from '../../../src/app/projectSnapshot'
+import { collectReversePart, reverseSection } from '../app/projectSection'
+import { stepFilesFor } from '../app/reSession'
+import { useReverse } from '../state/reStore'
+
+beforeEach(() => useReverse.getState().reset())
+
+describe('a Reverse project file', () => {
+  it('reads as saved when captured twice with an Import unchanged', () => {
+    const feature = { id: 1, kind: 'import', name: 'Import 1', file: { key: 'k1', name: 'a.step', size: 13 } }
+    useReverse.setState({ features: [feature as never], rolledBack: [], stepFiles: { k1: 'ISO-10303-21;' } })
+    const capture = () => {
+      const members: { name: string; bytes: Uint8Array }[] = []
+      const part = collectReversePart({ addMember: (name, bytes) => members.push({ name, bytes }) })
+      return { manifest: { reverse: part } as never, members }
+    }
+    const key = snapshotKey()
+    expect(key(capture())).toBe(key(capture()))
+  })
+})
+
+describe('a Reverse project file with an Import keyed like an object property', () => {
+  it('finds no STEP file for the key and saves none', () => {
+    const feature = { id: 1, kind: 'import', name: 'Import 1', file: { key: 'constructor', name: 'a.step', size: 1 } }
+    useReverse.setState({ features: [feature as never], rolledBack: [], stepFiles: {} })
+    expect(stepFilesFor([{ kind: 'import', file: 'constructor' }]).size).toBe(0)
+    const members: string[] = []
+    const part = collectReversePart({ addMember: (name) => members.push(name) })
+    expect(members).toEqual([])
+    expect(part?.stepFiles).toBeUndefined()
+  })
+})
+
+describe('a Reverse project file with hostile counters', () => {
+  const part = (nextFeatureId: number, extra: object = {}) => ({
+    version: 3,
+    features: [{ id: 7, kind: 'import', name: 'Import 1', file: { key: 'k', name: 'a.step', size: 1 }, visible: true }],
+    nextFeatureId,
+    nextOfKind: {},
+    elementsShown: true,
+    ...extra,
+  })
+  const load = (value: unknown) => reverseSection.prepare(value as never, { manifest: {} as never, members: new Map() })()
+
+  it('never hands out an id a feature or a parameter already has', () => {
+    for (const bad of [7, 3, -3, 0.5, 0]) {
+      load(part(bad, { parameters: [{ id: 4, name: 'a', kind: 'number', expression: '1' }], nextParameterId: 2 }))
+      expect(useReverse.getState().nextFeatureId).toBe(8)
+      expect(useReverse.getState().nextParameterId).toBe(5)
+    }
+    load(part(20))
+    expect(useReverse.getState().nextFeatureId).toBe(20)
+  })
+
+  it('refuses a name counter that is not a whole number', () => {
+    expect(() => load(part(8, { nextOfKind: { extrude: 0.5 } }))).toThrow()
+  })
+})