From 14dbdb57ff25047359db8e4d309a7e7c2efd1cd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Thu, 6 Aug 2026 10:56:25 +0200 Subject: [PATCH 1/9] refactor(semver): one parser for every version comparison (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The codebase answered "is X at least Y" two different ways, and they disagreed on exactly the inputs that show up in the field: input | semver.ts | runtime-version-gate.ts ------------|----------------|------------------------ "v4" | 4.0.0 | rejected "4.1" | 4.1.0 | rejected "garbage" | 0.0.0 (lowest) | rejected Neither was wrong for its own caller: a corrupt package manifest should not break a card in the catalog UI, and an unidentifiable runtime must not receive an upload. What was wrong is that the DIFFERENCE lived in two separate parsers, where nothing named it and nothing tested it side by side — and DOPE-448 adds three more comparisons on top. Now one parse, one ordering, and the lenient-vs-strict choice made by name at the call site: parseVersionStrict fails closed, parseVersionLenient degrades to 0.0.0. parseRuntimeVersion and compareSemver become thin wrappers, so no call site changes and their tests pass untouched. It lives in frontend/utils/ rather than backend/shared/ because the layer rules let backend-shared import utils and not the reverse, and both the VPP surface and the runtime gates need it. No layer exception. Also adds the two message builders the gates will use, and renames MIN_STRUCPP_RUNTIME_VERSION to MIN_RUNTIME_VERSION (old name kept as an alias) so the constant says what it is rather than why it appeared. Co-Authored-By: Claude Opus 5 --- .../shared/firmware/runtime-version-gate.ts | 100 ++++++++--- src/frontend/utils/__tests__/semver.test.ts | 158 ++++++++++++++++- src/frontend/utils/semver.ts | 159 +++++++++++++++--- 3 files changed, 367 insertions(+), 50 deletions(-) diff --git a/src/backend/shared/firmware/runtime-version-gate.ts b/src/backend/shared/firmware/runtime-version-gate.ts index 179010a1a..5efedb694 100644 --- a/src/backend/shared/firmware/runtime-version-gate.ts +++ b/src/backend/shared/firmware/runtime-version-gate.ts @@ -22,37 +22,41 @@ * orchestrator-agent the same way). */ -/** Minimum runtime version that speaks the STruC++ wire format. */ -export const MIN_STRUCPP_RUNTIME_VERSION = '4.1.0' +// Relative import on purpose: `npm run validate:arch` only inspects relative +// specifiers, so this path is actually checked against the layer rules — +// `backend-shared -> utils` is allowed, and using `@root/` here would have +// skipped the check rather than passed it. +import type { ParsedVersion } from '../../../frontend/utils/semver' +import { parseVersionStrict } from '../../../frontend/utils/semver' -export interface ParsedRuntimeVersion { - major: number - minor: number - patch: number - /** Pre-release identifier (e.g. `rc.3`) if present, otherwise undefined. */ - prerelease?: string -} +/** + * Oldest runtime this editor will upload to — the editor's own + * `minRuntimeVersion` declaration (DOPE-448). 4.1.0 is the floor + * because that is where the STruC++ pipeline landed. + * + * `MIN_STRUCPP_RUNTIME_VERSION` is kept as an alias so existing call + * sites and their tests keep working; new code should use the plain + * name, which says what the constant is rather than why it was + * introduced. + */ +export const MIN_RUNTIME_VERSION = '4.1.0' + +/** @deprecated Use `MIN_RUNTIME_VERSION`. */ +export const MIN_STRUCPP_RUNTIME_VERSION = MIN_RUNTIME_VERSION + +/** @deprecated Use `ParsedVersion` from `shared/utils/version-compare`. */ +export type ParsedRuntimeVersion = ParsedVersion /** * Parses a runtime version string. Returns null when the string - * doesn't carry enough information to compare against - * `MIN_STRUCPP_RUNTIME_VERSION` — e.g. the legacy `"v4"` or `"dev"` - * builds. Callers treat null as "incompatible". + * doesn't carry enough information to compare — e.g. the legacy `"v4"` + * or `"dev"` builds. Callers treat null as "incompatible". + * + * Delegates to the shared strict parser so the VPP surface and the + * runtime gates can never drift apart on what `"v4"` or `"4.1"` means. */ export function parseRuntimeVersion(raw: string | null | undefined): ParsedRuntimeVersion | null { - if (!raw) return null - const trimmed = raw.trim() - if (trimmed.length === 0) return null - // Require all three numeric components — `v4` alone is the legacy - // hardcoded header and must be rejected. - const match = trimmed.match(/^v?(\d+)\.(\d+)\.(\d+)(?:-(.+))?$/) - if (!match) return null - return { - major: parseInt(match[1], 10), - minor: parseInt(match[2], 10), - patch: parseInt(match[3], 10), - prerelease: match[4], - } + return parseVersionStrict(raw) } /** @@ -104,7 +108,51 @@ export function describeIncompatibleRuntime(raw: string | null | undefined): str const reported = raw && raw.trim().length > 0 ? raw.trim() : 'unknown' return ( `Runtime version ${reported} is not compatible with this editor. ` + - `Upload requires OpenPLC Runtime v${MIN_STRUCPP_RUNTIME_VERSION} or newer (STruC++ pipeline). ` + + `Upload requires OpenPLC Runtime v${MIN_RUNTIME_VERSION} or newer (STruC++ pipeline). ` + `Please upgrade the runtime on the target device before pushing this build.` ) } + +/** + * The other direction: this runtime declared a `minEditorVersion` at + * `GET /api/capabilities` and this editor is below it. + * + * Names both versions and the single action that fixes it — a bare + * "incompatible versions" turns into a support ticket. + */ +export function describeEditorTooOldForRuntime(args: { + runtimeVersion: string | null | undefined + minEditorVersion: string + editorVersion: string + deviceLabel?: string +}): string { + const runtime = args.runtimeVersion?.trim() ?? 'unknown' + const where = args.deviceLabel ? ` on ${args.deviceLabel}` : '' + return ( + `Runtime ${runtime}${where} requires OpenPLC Editor ${args.minEditorVersion} or newer. ` + + `This editor is ${args.editorVersion}. ` + + `Update the editor, or connect to a runtime that accepts ${args.editorVersion}.` + ) +} + +/** + * The VPP providing the selected board declares a runtime floor the + * connected runtime does not meet. + * + * Names the package's board rather than the package id — the board is + * what the user picked and recognises. + */ +export function describeVppRuntimeMismatch(args: { + boardTarget: string + minRuntimeVersion: string + runtimeVersion: string | null | undefined + deviceLabel?: string +}): string { + const runtime = args.runtimeVersion?.trim() ?? 'unknown' + const where = args.deviceLabel ? `The runtime at ${args.deviceLabel} reports` : 'The connected runtime reports' + return ( + `Board "${args.boardTarget}" requires OpenPLC Runtime v${args.minRuntimeVersion} or newer. ` + + `${where} ${runtime}. ` + + `Upgrade the runtime on that device, or select a board supported by ${runtime}.` + ) +} diff --git a/src/frontend/utils/__tests__/semver.test.ts b/src/frontend/utils/__tests__/semver.test.ts index 97ec6d22f..61fd6c16e 100644 --- a/src/frontend/utils/__tests__/semver.test.ts +++ b/src/frontend/utils/__tests__/semver.test.ts @@ -1,4 +1,160 @@ -import { compareSemver, isCompatibleEditorVersion } from '../semver' +import { + compareParsedVersions, + compareSemver, + isCompatibleEditorVersion, + isVersionAtLeast, + parseVersionLenient, + parseVersionStrict, +} from '../semver' + +describe('parseVersionStrict', () => { + it('parses a plain three-part version', () => { + expect(parseVersionStrict('4.1.9')).toEqual({ major: 4, minor: 1, patch: 9, prerelease: undefined }) + }) + + it('accepts the tag-style v prefix the runtime reports', () => { + expect(parseVersionStrict('v4.2.0')).toEqual({ major: 4, minor: 2, patch: 0, prerelease: undefined }) + }) + + it('captures pre-release and build suffixes without failing', () => { + expect(parseVersionStrict('4.1.0-rc.3')?.prerelease).toBe('rc.3') + expect(parseVersionStrict('4.1.0+build.5')?.prerelease).toBe('build.5') + }) + + it('tolerates surrounding whitespace', () => { + expect(parseVersionStrict(' 4.1.9 ')).toEqual({ major: 4, minor: 1, patch: 9, prerelease: undefined }) + }) + + // The whole point of the strict parser: these are the values a runtime in + // the field actually reports when it cannot identify itself, and every one + // of them must stay unparseable so a gate fails closed instead of guessing. + it.each([ + ['v4', 'the legacy hardcoded header'], + ['4.1', 'a two-part version'], + ['dev', 'a source build with no CI tag'], + ['garbage', 'anything else'], + ['', 'an empty string'], + ])('returns null for %p (%s)', (input) => { + expect(parseVersionStrict(input)).toBeNull() + }) + + it('returns null for null and undefined', () => { + expect(parseVersionStrict(null)).toBeNull() + expect(parseVersionStrict(undefined)).toBeNull() + }) +}) + +describe('parseVersionLenient', () => { + it('parses a plain three-part version', () => { + expect(parseVersionLenient('4.1.9')).toEqual({ major: 4, minor: 1, patch: 9 }) + }) + + it('fills missing components with zero', () => { + expect(parseVersionLenient('4.1')).toEqual({ major: 4, minor: 1, patch: 0 }) + expect(parseVersionLenient('4')).toEqual({ major: 4, minor: 0, patch: 0 }) + }) + + it('strips the v prefix and any suffix before parsing', () => { + expect(parseVersionLenient('v4.1.9')).toEqual({ major: 4, minor: 1, patch: 9 }) + expect(parseVersionLenient('4.1.9-rc.1')).toEqual({ major: 4, minor: 1, patch: 9 }) + expect(parseVersionLenient('4.1.9+build.5')).toEqual({ major: 4, minor: 1, patch: 9 }) + }) + + // Degrading to the lowest possible version means a corrupt manifest field + // loses every comparison rather than winning one. + it.each([ + ['garbage', 'a non-numeric string'], + ['abc.def.ghi', 'non-numeric components'], + ['', 'an empty string'], + ])('degrades %p to 0.0.0 (%s)', (input) => { + expect(parseVersionLenient(input)).toEqual({ major: 0, minor: 0, patch: 0 }) + }) + + it('degrades null and undefined to 0.0.0', () => { + expect(parseVersionLenient(null)).toEqual({ major: 0, minor: 0, patch: 0 }) + expect(parseVersionLenient(undefined)).toEqual({ major: 0, minor: 0, patch: 0 }) + }) +}) + +describe('compareParsedVersions', () => { + const v = (major: number, minor: number, patch: number) => ({ major, minor, patch }) + + it('orders by major first', () => { + expect(compareParsedVersions(v(5, 0, 0), v(4, 9, 9))).toBe(1) + expect(compareParsedVersions(v(4, 9, 9), v(5, 0, 0))).toBe(-1) + }) + + it('orders by minor when majors match', () => { + expect(compareParsedVersions(v(4, 2, 0), v(4, 1, 99))).toBe(1) + expect(compareParsedVersions(v(4, 1, 99), v(4, 2, 0))).toBe(-1) + }) + + it('orders by patch when major and minor match', () => { + expect(compareParsedVersions(v(4, 1, 10), v(4, 1, 9))).toBe(1) + expect(compareParsedVersions(v(4, 1, 9), v(4, 1, 10))).toBe(-1) + }) + + it('returns 0 for equal triples', () => { + expect(compareParsedVersions(v(4, 1, 9), v(4, 1, 9))).toBe(0) + }) + + it('ignores pre-release when ordering', () => { + // Load-bearing deviation from strict semver: the rc builds on a version + // line ARE the builds shipping that line's features, so treating them as + // "less than" the release would reject runtimes that work. + const rc = { ...v(4, 1, 0), prerelease: 'rc.3' } + expect(compareParsedVersions(rc, v(4, 1, 0))).toBe(0) + expect(compareParsedVersions(v(4, 1, 0), rc)).toBe(0) + }) +}) + +describe('isVersionAtLeast', () => { + it('passes when the candidate is above the floor', () => { + expect(isVersionAtLeast('4.2.10', '4.2.1')).toBe(true) + }) + + it('passes when the candidate sits exactly on the floor', () => { + expect(isVersionAtLeast('4.2.1', '4.2.1')).toBe(true) + }) + + it('fails when the candidate is below the floor', () => { + expect(isVersionAtLeast('4.2.0', '4.2.1')).toBe(false) + }) + + it('passes a pre-release build of the required version', () => { + expect(isVersionAtLeast('v4.1.9-rc.1', '4.1.9')).toBe(true) + }) + + // A peer that asks for nothing gets nothing enforced — this is what keeps + // runtimes predating /api/capabilities working unchanged. + const NOTHING_DECLARED: Array<[string | null | undefined, string]> = [ + [undefined, 'undefined'], + [null, 'null'], + ['', 'an empty string'], + ] + + it.each(NOTHING_DECLARED)('passes when the floor is %p (%s)', (floor) => { + expect(isVersionAtLeast('4.2.0', floor)).toBe(true) + }) + + it('passes when the floor itself is unparseable, since it declares nothing', () => { + expect(isVersionAtLeast('4.2.0', 'garbage')).toBe(true) + expect(isVersionAtLeast('4.2.0', 'v4')).toBe(true) + }) + + // Fails closed: an unidentifiable peer never clears a real floor. + const UNIDENTIFIABLE: Array<[string | null | undefined, string]> = [ + ['v4', 'the legacy header'], + ['dev', 'a source build'], + ['garbage', 'a corrupt value'], + [null, 'an unreachable peer'], + [undefined, 'a missing value'], + ] + + it.each(UNIDENTIFIABLE)('fails when the candidate is %p (%s) and a real floor exists', (candidate) => { + expect(isVersionAtLeast(candidate, '4.1.0')).toBe(false) + }) +}) describe('compareSemver', () => { it('returns 0 when versions are identical', () => { diff --git a/src/frontend/utils/semver.ts b/src/frontend/utils/semver.ts index 673a026eb..39a14f2c6 100644 --- a/src/frontend/utils/semver.ts +++ b/src/frontend/utils/semver.ts @@ -1,37 +1,150 @@ /** - * Tiny semver helpers used by the VPP catalog browser to compare a package - * version's `minEditorVersion` against the running editor's `APP_VERSION`. + * Single source of truth for comparing OpenPLC version strings. * - * Intentionally local — adding the full `semver` npm dependency for two - * comparisons would inflate the renderer bundle for no real gain. Pre-release - * suffixes (`-rc.1`, `+build.5`) are stripped before parsing; this matches - * what arduino-cli does when matching boards.txt menu constraints, and we - * don't currently publish pre-release VPPs. + * Four independent compatibility questions are decided by comparing two + * version strings (DOPE-448): * - * Malformed strings degrade to `0.0.0` so a corrupt manifest in the wild - * doesn't crash the UI — it just compares as the lowest possible version. + * 1. is this runtime new enough for this editor? (`MIN_RUNTIME_VERSION`) + * 2. is this editor new enough for this runtime? (`minEditorVersion` from + * `GET /api/capabilities`) + * 3. is this editor new enough for this VPP? (`package.minEditorVersion`) + * 4. is this runtime new enough for this VPP? (`package.minRuntimeVersion`) + * + * This file used to answer only #3, with `firmware/runtime-version-gate.ts` + * carrying its own parser for the runtime side. The two disagreed on exactly + * the inputs that show up in the field: + * + * input | catalog parser | runtime parser + * -------------|------------------|---------------- + * "v4" | 4.0.0 | rejected + * "4.1" | 4.1.0 | rejected + * "garbage" | 0.0.0 (lowest) | rejected + * + * Neither behaviour was wrong for its own caller. A package manifest carrying + * a corrupt version should not crash the catalog UI, and an unidentifiable + * runtime must not receive an upload. What was wrong is that the DIFFERENCE + * lived in two separate parsers, where nothing named it and nothing tested it + * side by side. + * + * So: one parse, one comparison, and the lenient-vs-strict choice made + * explicitly by name at the call site. `parseVersionStrict` returns null for + * anything it cannot fully identify — callers that must fail closed use it. + * `parseVersionLenient` fills missing components with 0 and degrades garbage to + * 0.0.0 — callers rendering untrusted metadata use it. + * + * Pre-release and build suffixes (`-rc.1`, `+build.5`) are parsed but do NOT + * affect ordering: `4.1.0-rc.3` compares equal to `4.1.0`. This is deliberate + * and load-bearing for the runtime gate — the rc tags on a version line ARE + * the builds shipping that line's features, so treating them as "less than" + * the release (strict semver's rule) would reject runtimes that work. + * + * Lives in `frontend/utils/` rather than `backend/shared/` on purpose: the + * architecture rules let `backend-shared` import `utils` but not the reverse, + * and both the VPP surface and the runtime gate need this. No layer exception + * required. */ -type Triple = readonly [number, number, number] +export interface ParsedVersion { + major: number + minor: number + patch: number + /** Pre-release identifier (e.g. `rc.3`) when present. Never affects ordering. */ + prerelease?: string +} + +/** `v4.1.0-rc.3` / `4.1.0` — all three numeric components required. */ +const STRICT_RE = /^v?(\d+)\.(\d+)\.(\d+)(?:[-+](.+))?$/ -function parseSemver(input: string): Triple { - const stripped = input.split(/[-+]/)[0] - const parts = stripped.split('.') - const major = Number.parseInt(parts[0] ?? '', 10) - const minor = Number.parseInt(parts[1] ?? '', 10) - const patch = Number.parseInt(parts[2] ?? '', 10) - return [Number.isFinite(major) ? major : 0, Number.isFinite(minor) ? minor : 0, Number.isFinite(patch) ? patch : 0] +/** + * Parse a version string, requiring all three numeric components. + * + * Returns null for anything else — `"v4"`, `"4.1"`, `"dev"`, `""`, null. Use + * this when an unidentifiable version must block an action: the caller cannot + * accidentally treat "I don't know" as "old enough" or "new enough", because + * there is no number to compare. + */ +export function parseVersionStrict(raw: string | null | undefined): ParsedVersion | null { + if (!raw) return null + const match = raw.trim().match(STRICT_RE) + if (!match) return null + return { + major: Number.parseInt(match[1], 10), + minor: Number.parseInt(match[2], 10), + patch: Number.parseInt(match[3], 10), + prerelease: match[4], + } } -export function compareSemver(a: string, b: string): -1 | 0 | 1 { - const [aMajor, aMinor, aPatch] = parseSemver(a) - const [bMajor, bMinor, bPatch] = parseSemver(b) - if (aMajor !== bMajor) return aMajor > bMajor ? 1 : -1 - if (aMinor !== bMinor) return aMinor > bMinor ? 1 : -1 - if (aPatch !== bPatch) return aPatch > bPatch ? 1 : -1 +/** + * Parse a version string, filling in whatever is missing with zero. + * + * `"4.1"` becomes 4.1.0; `"garbage"` and `""` become 0.0.0 — the lowest + * possible version, so a corrupt value loses every comparison instead of + * winning one. Use this for untrusted metadata being rendered rather than + * enforced, where a malformed field should degrade the display and not throw. + */ +export function parseVersionLenient(raw: string | null | undefined): ParsedVersion { + // Deliberately unanchored at the end: it consumes as many leading numeric + // components as it finds and ignores whatever follows, so `4.1.9-rc.1` and + // `4.1` both parse without a separate suffix-stripping pass. + const match = (raw ?? '').trim().match(/^v?(\d+)(?:\.(\d+))?(?:\.(\d+))?/) + if (!match) return { major: 0, minor: 0, patch: 0 } + const toInt = (value: string | undefined): number => { + const parsed = Number.parseInt(value ?? '', 10) + return Number.isFinite(parsed) ? parsed : 0 + } + return { major: toInt(match[1]), minor: toInt(match[2]), patch: toInt(match[3]) } +} + +/** + * Order two parsed versions. Pre-release suffixes are ignored (see the module + * comment) — only the numeric triple decides. + */ +export function compareParsedVersions(a: ParsedVersion, b: ParsedVersion): -1 | 0 | 1 { + if (a.major !== b.major) return a.major > b.major ? 1 : -1 + if (a.minor !== b.minor) return a.minor > b.minor ? 1 : -1 + if (a.patch !== b.patch) return a.patch > b.patch ? 1 : -1 return 0 } +/** + * `candidate >= minimum`, where an unparseable `candidate` fails closed. + * + * This is the shape every DOPE-448 gate wants: "may I proceed?" answered + * `false` when the peer cannot be identified. An absent `minimum` means no + * constraint was declared, which is a pass — a peer that asks for nothing gets + * nothing enforced, which is what keeps runtimes predating + * `/api/capabilities` working unchanged. + */ +export function isVersionAtLeast(candidate: string | null | undefined, minimum: string | null | undefined): boolean { + if (!minimum) return true + const min = parseVersionStrict(minimum) + if (!min) return true // a floor we cannot read declares nothing + const version = parseVersionStrict(candidate) + if (!version) return false // an unidentifiable peer never clears a real floor + return compareParsedVersions(version, min) >= 0 +} + +// --------------------------------------------------------------------------- +// Lenient VPP-surface helpers +// --------------------------------------------------------------------------- + +/** + * Lenient comparison, used by the VPP catalog and the package install gate. + * + * Lenient is right *here* specifically: a package manifest is untrusted + * third-party metadata, and a corrupt `version` string should sort as the + * lowest possible version rather than break a card in the catalog UI. Gates + * deciding whether to talk to a runtime use `isVersionAtLeast` instead. + */ +export function compareSemver(a: string, b: string): -1 | 0 | 1 { + return compareParsedVersions(parseVersionLenient(a), parseVersionLenient(b)) +} + +/** + * True when `current` satisfies `minRequired`. An absent or empty minimum + * means the package declared no floor, which is a pass. + */ export function isCompatibleEditorVersion(minRequired: string | undefined, current: string): boolean { if (!minRequired) return true return compareSemver(current, minRequired) >= 0 From 7ad73f67bf5f823425a87282009caa82f326ae24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Thu, 6 Aug 2026 10:56:44 +0200 Subject: [PATCH 2/9] feat(vpp): enforce minEditorVersion when installing a package (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit manifest.package.minEditorVersion has been required by the schema all along, and openplc-packages/docs/package-format.md:69 promised the editor refuses to install a package requiring a newer editor. It did not. The only consumer was the catalog UI (catalog-browser.tsx), which renders an "Editor outdated" button state. package-manager-module.ts::install — the trust boundary that both the remote install and the local "Add from file…" flow converge on, which already validates the schema, verifies the signature and hardens the path — never looked at the field. So a .vpp dragged in from disk ignored it completely, and a package installed before an editor downgrade kept loading. The check now sits next to the signature verification, so one gate covers both entry paths. This is also the mechanism that answers "what about a VPP needing a UI engine the editor may not have": the engine shipped in some release, the package declares that release as its floor, an older editor cannot install it. No capability enumeration needed. minEditorVersion and minRuntimeVersion are typed in the zod schema rather than left to .passthrough() — a field a gate reads should not reach it as unknown. Both stay optional so packages built before they existed keep installing. Co-Authored-By: Claude Opus 5 --- .../package-manager/package-manager-module.ts | 52 +++++++++++++++++++ .../shared/ports/package-manifest-schema.ts | 11 ++++ src/middleware/shared/ports/types.ts | 12 +++++ 3 files changed, 75 insertions(+) diff --git a/src/backend/editor/package-manager/package-manager-module.ts b/src/backend/editor/package-manager/package-manager-module.ts index 7319b5837..e588c826f 100644 --- a/src/backend/editor/package-manager/package-manager-module.ts +++ b/src/backend/editor/package-manager/package-manager-module.ts @@ -3,6 +3,8 @@ import extract from 'extract-zip' import { existsSync, mkdirSync, mkdtempSync, readFileSync, renameSync, rmSync, writeFileSync } from 'fs' import { join } from 'path' +import { APP_VERSION } from '../../../frontend/data/constants/app-version' +import { isCompatibleEditorVersion } from '../../../frontend/utils/semver' import { PackageManifestSchema } from '../../../middleware/shared/ports/package-manifest-schema' import { validatePathId } from '../../shared/utils/path-safety' import { TRUSTED_PACKAGE_KEYS } from '../../shared/utils/vpp/trusted-keys' @@ -79,6 +81,28 @@ class PackageManagerModule { } } + // Compatibility floor (DOPE-448). This is the ONLY place the editor + // enforces `minEditorVersion`, and it sits here because both entry paths + // — remote catalog install and the local "Add from file…" picker — + // converge on this method. The catalog UI's "Editor outdated" button + // state is a courtesy that stops the user earlier; it is not the gate, + // and before this check existed a `.vpp` dragged in from disk bypassed + // the constraint entirely. + // + // A package declares a floor when it needs an editor feature it cannot + // work without — a UI engine, a new screen widget, a layout the renderer + // learned in some release. Installing it on an older editor produces a + // board that renders wrong rather than an error, so refuse up front. + if (!isCompatibleEditorVersion(manifest.package.minEditorVersion, APP_VERSION)) { + return { + success: false, + error: + `Package "${manifest.package.name}" ${manifest.package.version} requires ` + + `OpenPLC Editor ${manifest.package.minEditorVersion} or newer. This editor is ${APP_VERSION}. ` + + `Update the editor, or install an older version of this package.`, + } + } + // Validate package.id BEFORE using it as a path component. Without // this, a malicious .vpp with `"id": "../../something"` would have // `targetDir` resolve outside packagesDir and the rmSync below @@ -278,6 +302,34 @@ class PackageManagerModule { return pkg?.path ?? null } + /** + * `package.minRuntimeVersion` of the installed package that provides + * `boardName`, or null when no installed package does, when the + * matching device is not a `runtime-v4` target, or when the package + * declares no floor (DOPE-448). + * + * Board lookup is by device *name* because that is the identifier the + * compile pipeline carries as `boardTarget` — the same match + * `handleVendorPluginPackaging` performs. + * + * Only runtime-v4 devices can carry a meaningful floor: their HAL is + * plugin code built against the runtime's API. An `arduino-cli` + * device never talks to the runtime, so a floor there would be a + * claim nothing can check — openplc-packages' `validate.ts` rejects + * it at authoring time, and this returns null if one slips through. + */ + getRuntimeFloorForBoard(boardName: string): string | null { + for (const pkg of this.listInstalled()) { + const manifest = this.getInstalledPackageManifest(pkg.packageId) + if (!manifest) continue + const device = manifest.devices.find((d) => d.name === boardName) + if (!device) continue + if (device.target.type !== 'runtime-v4') return null + return manifest.package.minRuntimeVersion ?? null + } + return null + } + private readRegistry(): PackageRegistry { if (!existsSync(this.registryPath)) { return { formatVersion: '1.0', packages: {} } diff --git a/src/middleware/shared/ports/package-manifest-schema.ts b/src/middleware/shared/ports/package-manifest-schema.ts index 815230e3e..ef23107b9 100644 --- a/src/middleware/shared/ports/package-manifest-schema.ts +++ b/src/middleware/shared/ports/package-manifest-schema.ts @@ -44,6 +44,17 @@ export const PackageManifestSchema = z id: z.string().min(1), name: z.string().min(1), version: z.string().min(1), + // Compatibility floors (DOPE-448). Optional on purpose: packages built + // before these fields existed must keep installing, and a package that + // declares no floor declares no constraint. They are typed here rather + // than left to `.passthrough()` because the install gate compares them + // — a field a gate reads should not reach it as `unknown`. + // + // Authoring-side rules (minRuntimeVersion required iff a device targets + // runtime-v4, rejected otherwise) live in openplc-packages' + // `scripts/validate.ts`, per this file's split of responsibilities. + minEditorVersion: z.string().min(1).optional(), + minRuntimeVersion: z.string().min(1).optional(), }) .passthrough(), devices: z.array(z.object({}).passthrough()).min(1), diff --git a/src/middleware/shared/ports/types.ts b/src/middleware/shared/ports/types.ts index f795de683..11234f1c6 100644 --- a/src/middleware/shared/ports/types.ts +++ b/src/middleware/shared/ports/types.ts @@ -718,7 +718,19 @@ export interface PackageManifest { } description: string license?: string + /** + * Oldest editor that may install this package. The install gate refuses a + * package whose floor is above `APP_VERSION` — this is how a package + * requires an editor feature it cannot work without (DOPE-448). + */ minEditorVersion?: string + /** + * Oldest runtime this package works with. Declared only by packages with a + * `runtime-v4` target, whose plugin code executes inside the runtime + * process. Checked at compile time, not install time: the target device is + * unknown until the user connects to one. + */ + minRuntimeVersion?: string } devices: Array<{ id: string From 6547b87c4bc386b1757a628d531a870e3e563902 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Thu, 6 Aug 2026 10:57:04 +0200 Subject: [PATCH 3/9] feat(compile): block upload when a declared version floor is not met (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the two directions that had no enforcement at all. Runtime -> editor: probe-runtime-version.ts now prefers GET /api/capabilities, which carries the runtime version and its minEditorVersion in one round-trip, and falls back to /api/version otherwise. A runtime predating the endpoint answers 401 from the / catch-all (not 404) — both land in the same fallback, silently, because that 401 is the normal answer from every device in the field and a warning there would nag on every upload. VPP -> runtime: packageVppPlugin surfaces the package's minRuntimeVersion, compared against the connected runtime before the upload. It cannot be checked at install time — the target device is unknown until the user connects — and it must be checked before sending, because the failure it prevents is vendor plugin code that loads on a live PLC and dies at scan time. Both gates are inert against everything currently deployed, in two independent ways: a runtime that declares no floor passes, and a caller that passes no editorVersion passes. editorVersion is injected through RunCompilePipelineArgs rather than imported: APP_VERSION lives in frontend/data/, which the layer rules keep out of backend/shared/ — correctly, since which build is running is a fact about the host app, not about the compile. Same reasoning for getVppRuntimeFloor on the adapter context, which additionally avoids pulling the Electron-dependent logger in at module load. Co-Authored-By: Claude Opus 5 --- .../editor-compiler-platform-port.test.ts | 71 +++++++- .../editor/compiler/compiler-module.ts | 12 ++ .../compiler/editor-compiler-platform-port.ts | 75 ++++++-- .../shared/compile/__tests__/pipeline.test.ts | 127 ++++++++++++++ src/backend/shared/compile/pipeline.ts | 67 ++++++- .../__tests__/probe-runtime-version.test.ts | 163 +++++++++++++++++- .../shared/library/probe-runtime-version.ts | 91 +++++++++- .../shared/ports/compiler-platform-port.ts | 28 +++ 8 files changed, 600 insertions(+), 34 deletions(-) diff --git a/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts b/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts index 1de587f46..66b2edcda 100644 --- a/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts +++ b/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts @@ -277,7 +277,29 @@ describe('createEditorCompilerPlatformPort', () => { const port = createEditorCompilerPlatformPort(makeHandlers({ handleVendorPluginPackaging }), makeContext()) const result = await port.packageVppPlugin({ boardTarget: 'SLM-RP4' }, () => undefined) expect(handleVendorPluginPackaging).toHaveBeenCalledTimes(1) - expect(result).toEqual({ files: {} }) + // No `getVppRuntimeFloor` in the default context, so no floor is known — + // which the pipeline reads as "no constraint". + expect(result).toEqual({ files: {}, minRuntimeVersion: null }) + }) + + it('packageVppPlugin surfaces the VPP runtime floor when the context can resolve one', async () => { + const getVppRuntimeFloor = jest.fn(() => '4.1.9') + const port = createEditorCompilerPlatformPort(makeHandlers(), makeContext({ getVppRuntimeFloor })) + const result = await port.packageVppPlugin({ boardTarget: 'SLM-RP4' }, () => undefined) + expect(getVppRuntimeFloor).toHaveBeenCalledWith('SLM-RP4') + expect(result.minRuntimeVersion).toBe('4.1.9') + }) + + it('packageVppPlugin reports no floor when the resolver throws', async () => { + // A gate that failed the build because it could not read its own metadata + // would be worse than the mismatch it exists to catch. + const getVppRuntimeFloor = jest.fn(() => { + throw new Error('registry unreadable') + }) + const port = createEditorCompilerPlatformPort(makeHandlers(), makeContext({ getVppRuntimeFloor })) + const result = await port.packageVppPlugin({ boardTarget: 'SLM-RP4' }, () => undefined) + expect(result.minRuntimeVersion).toBeNull() + expect(result.errors).toBeUndefined() }) it('packageVppPlugin returns an errors[] when the handler throws', async () => { @@ -327,7 +349,48 @@ describe('createEditorCompilerPlatformPort', () => { { context: { kind: 'editor-https', ip: '10.0.0.1', jwt: 'token' } }, () => undefined, ) - expect(result).toEqual({ ok: true, version: '4.1.2' }) + // This stub answers every endpoint with a `/api/version` body, so + // `/api/capabilities` yields no usable `runtimeVersion` and the probe + // falls back — the exact shape of a runtime predating the endpoint. + expect(result).toEqual({ ok: true, version: '4.1.2', minEditorVersion: null }) + }) + + it('checkRuntimeVersion reads the editor floor from /api/capabilities when the device serves it', async () => { + const makeRuntimeApiRequest = jest.fn(async (_ip: string, endpoint: string) => { + if (endpoint === '/api/capabilities') { + return { success: true as const, data: { runtimeVersion: 'v4.2.0', minEditorVersion: '4.2.1' } } + } + return { success: true as const, data: { version: 'SHOULD-NOT-BE-USED' } } + }) as unknown as EditorCompilerPlatformPortContext['mainProcessBridge']['makeRuntimeApiRequest'] + const port = createEditorCompilerPlatformPort( + makeHandlers(), + makeContext({ mainProcessBridge: { makeRuntimeApiRequest, makeRuntimeApiUpload: jest.fn() } }), + ) + const result = await port.checkRuntimeVersion( + { context: { kind: 'editor-https', ip: '10.0.0.1', jwt: 'token' } }, + () => undefined, + ) + expect(result).toEqual({ ok: true, version: 'v4.2.0', minEditorVersion: '4.2.1' }) + }) + + it('checkRuntimeVersion falls back to /api/version when capabilities 404s', async () => { + const makeRuntimeApiRequest = jest.fn(async (_ip: string, endpoint: string) => { + if (endpoint === '/api/capabilities') return { success: false as const, error: '404 Not Found' } + return { success: true as const, data: { version: 'v4.1.7' } } + }) as unknown as EditorCompilerPlatformPortContext['mainProcessBridge']['makeRuntimeApiRequest'] + const log = jest.fn() + const port = createEditorCompilerPlatformPort( + makeHandlers(), + makeContext({ mainProcessBridge: { makeRuntimeApiRequest, makeRuntimeApiUpload: jest.fn() } }), + ) + const result = await port.checkRuntimeVersion( + { context: { kind: 'editor-https', ip: '10.0.0.1', jwt: 'token' } }, + log, + ) + expect(result).toEqual({ ok: true, version: 'v4.1.7', minEditorVersion: null }) + // The 404 is the normal answer from every deployed runtime — it must not + // nag the user on every upload. + expect(log).not.toHaveBeenCalled() }) it('checkRuntimeVersion returns version=null and logs a warning on probe failure', async () => { @@ -344,7 +407,7 @@ describe('createEditorCompilerPlatformPort', () => { { context: { kind: 'editor-https', ip: '10.0.0.1', jwt: 'token' } }, log, ) - expect(result).toEqual({ ok: true, version: null }) + expect(result).toEqual({ ok: true, version: null, minEditorVersion: null }) expect(log).toHaveBeenCalledWith(expect.stringContaining('Could not reach runtime'), 'warning') }) @@ -361,7 +424,7 @@ describe('createEditorCompilerPlatformPort', () => { { context: { kind: 'editor-https', ip: '10.0.0.1', jwt: 'token' } }, log, ) - expect(result).toEqual({ ok: true, version: null }) + expect(result).toEqual({ ok: true, version: null, minEditorVersion: null }) expect(log).toHaveBeenCalledWith(expect.stringContaining('probe blew up'), 'warning') }) }) diff --git a/src/backend/editor/compiler/compiler-module.ts b/src/backend/editor/compiler/compiler-module.ts index 6a3ae3cdb..7a578f56e 100644 --- a/src/backend/editor/compiler/compiler-module.ts +++ b/src/backend/editor/compiler/compiler-module.ts @@ -112,6 +112,7 @@ import { buildModuleConfigEntries, generateVendorPluginConfig, } from '@root/backend/shared/utils/vpp/generate-vendor-plugin-config' +import { APP_VERSION } from '@root/frontend/data/constants/app-version' import { getErrorMessage } from '@root/frontend/utils/get-error-message' import { app as electronApp, dialog, MessageChannelMain } from 'electron' import type { MessagePortMain } from 'electron/main' @@ -2585,6 +2586,11 @@ class CompilerModule { cleanBuild: cleanBuild ?? false, mainProcessBridge, compressSourceFolder: (folderPath: string) => this.compressSourceFolder(folderPath), + // VPP runtime floor (DOPE-448). Constructed per call rather than + // held on the class because the registry is read off disk and may + // have changed since the last compile (a package installed or + // removed mid-session). + getVppRuntimeFloor: (board: string) => new PackageManagerModule().getRuntimeFloorForBoard(board), pollTimeoutMs: CompilerModule.COMPILATION_STATUS_TIMEOUT_MS, pollIntervalMs: CompilerModule.COMPILATION_STATUS_POLL_INTERVAL_MS, startTimeoutMs: POST_BUILD_START_TIMEOUT_MS, @@ -2665,6 +2671,12 @@ class CompilerModule { communicationPort: communicationPort ?? undefined, ...(vppModbusState ? { vppModbusState } : {}), vendorScreenData: effectiveVendorScreenData, + // Compared against the `minEditorVersion` a runtime publishes at + // `/api/capabilities` (DOPE-448). Injected because the pipeline + // lives in `backend/shared/`, which the layer rules keep out of + // `frontend/data/` — which build is running is a fact about the + // host app, not about the compile. + editorVersion: APP_VERSION, }, platformPort, (event) => { diff --git a/src/backend/editor/compiler/editor-compiler-platform-port.ts b/src/backend/editor/compiler/editor-compiler-platform-port.ts index 8b689d18d..6324d6ed9 100644 --- a/src/backend/editor/compiler/editor-compiler-platform-port.ts +++ b/src/backend/editor/compiler/editor-compiler-platform-port.ts @@ -117,6 +117,22 @@ export interface EditorCompilerPlatformPortContext { * in the `archiver`-dependent compressSourceFolder method (which * has its own private state on CompilerModule). */ compressSourceFolder: (folderPath: string) => Promise + /** + * `package.minRuntimeVersion` of the VPP providing a given board, or + * null when the board isn't from a VPP / declares no floor + * (DOPE-448). The pipeline compares it against the connected + * runtime after the version probe. + * + * Injected rather than resolved here for the same reason as + * `compressSourceFolder`: importing `PackageManagerModule` directly + * pulls in the Electron-dependent logger at module load, which + * breaks anything importing this adapter outside a real Electron + * process (its own unit test included). + * + * Optional so callers predating this stay valid; absent means "no + * floor known", which the pipeline treats as no constraint. + */ + getVppRuntimeFloor?: (boardTarget: string) => string | null /** Timeout for the post-upload compile-status poll. */ pollTimeoutMs: number /** Interval for the post-upload compile-status poll. */ @@ -495,8 +511,14 @@ export function createEditorCompilerPlatformPort( }, /** - * Probe the device's `/api/version` (unauthenticated) so the - * pipeline can short-circuit uploads to pre-4.1.0 runtimes. + * Probe the device (unauthenticated) so the pipeline can + * short-circuit uploads in both directions: to a runtime too old + * for this editor, and from an editor too old for this runtime. + * + * Tries `/api/capabilities` first — it carries the runtime version + * AND the runtime's `minEditorVersion` in one round-trip — and + * falls back to `/api/version` for runtimes that predate it + * (DOPE-448). * * Transport: Electron's HTTPS bridge → device IP. * Response parsing + null-fallback live in the shared @@ -505,19 +527,21 @@ export function createEditorCompilerPlatformPort( */ async checkRuntimeVersion(args: CheckRuntimeVersionArgs, log: PlatformLog): Promise { const deviceContext = assertEditorHttpsContext(args.context) - const { version } = await probeRuntimeVersion({ - fetchVersion: async () => { - const result = await context.mainProcessBridge.makeRuntimeApiRequest<{ version: string }>( - deviceContext.ip, - '/api/version', - (data: string) => JSON.parse(data) as { version: string }, - ) - if (!result.success) return { success: false, error: result.error } - return { success: true, body: result.data } - }, + const getJson = async (endpoint: string) => { + const result = await context.mainProcessBridge.makeRuntimeApiRequest( + deviceContext.ip, + endpoint, + (data: string) => JSON.parse(data) as unknown, + ) + if (!result.success) return { success: false as const, error: result.error } + return { success: true as const, body: result.data } + } + const { version, minEditorVersion } = await probeRuntimeVersion({ + fetchCapabilities: () => getJson('/api/capabilities'), + fetchVersion: () => getJson('/api/version'), log, }) - return { ok: true, version } + return { ok: true, version, minEditorVersion } }, /** @@ -560,7 +584,12 @@ export function createEditorCompilerPlatformPort( log(message, logLevel ?? 'info') }, ) - return { files: {} } + // Surface the package's runtime floor so the pipeline can compare + // it against the connected runtime after the version probe + // (DOPE-448). Read here rather than inside the handler because + // the handler returns void and writes straight to disk; the + // registry lookup is cheap next to the packaging work that ran. + return { files: {}, minRuntimeVersion: readVppRuntimeFloor(context, args.boardTarget) } } catch (error) { const message = error instanceof Error ? error.message : String(error) return { @@ -590,6 +619,24 @@ export function assertEditorHttpsContext( return context } +/** + * `package.minRuntimeVersion` of the VPP providing `boardTarget`, or + * null when no resolver was injected, the board is not from a VPP, is + * not a `runtime-v4` target, or the package declares no floor. + * + * Never throws: a missing or unreadable registry means "no declared + * floor", which the pipeline treats as no constraint. A version gate + * that failed the build because it could not read its own metadata + * would be worse than the mismatch it exists to catch. + */ +function readVppRuntimeFloor(context: EditorCompilerPlatformPortContext, boardTarget: string): string | null { + try { + return context.getVppRuntimeFloor?.(boardTarget) ?? null + } catch { + return null + } +} + /** * Find the arduino-cli-produced `Baremetal.ino.hex` under the build * directory. arduino-cli writes it to a board-FQBN-specific diff --git a/src/backend/shared/compile/__tests__/pipeline.test.ts b/src/backend/shared/compile/__tests__/pipeline.test.ts index 06f2f3353..169789e12 100644 --- a/src/backend/shared/compile/__tests__/pipeline.test.ts +++ b/src/backend/shared/compile/__tests__/pipeline.test.ts @@ -40,6 +40,17 @@ jest.mock('../../firmware/runtime-version-gate', () => ({ describeIncompatibleRuntime: jest.fn( (v: string | null) => `Runtime ${String(v)} is too old; please upgrade to 4.1.0+.`, ), + // The two DOPE-448 message builders. Only the message text is stubbed — + // the *decisions* are made by `isVersionAtLeast` from + // `frontend/utils/semver`, which is deliberately NOT mocked so these tests + // exercise the real comparison. + describeEditorTooOldForRuntime: jest.fn( + (a: { minEditorVersion: string }) => `This editor is older than the runtime requires (${a.minEditorVersion}).`, + ), + describeVppRuntimeMismatch: jest.fn( + (a: { boardTarget: string; minRuntimeVersion: string }) => + `Board "${a.boardTarget}" needs runtime ${a.minRuntimeVersion} or newer.`, + ), })) // Mock the conf-generator step so tests can deterministically force // the runtime-v4 confs branch to throw (covers the pipeline's outer @@ -379,6 +390,122 @@ describe('runCompilePipeline — runtime v4 path', () => { expect(events.some((e) => /too old|upgrade/i.test(e.message))).toBe(true) }) + // ------------------------------------------------------------------------- + // DOPE-448: the two floors the runtime and the VPP declare + // ------------------------------------------------------------------------- + + const v4Args = (overrides: Partial = {}) => + makeArgs({ + isSimulator: false, + isRuntimeV4: true, + boardRuntime: 'openplc-compiler', + deviceContext: deviceContextFixture, + // The host app injects its own version; `4.2.0` is low enough to be + // refused by the floors below and high enough to clear the passing ones. + editorVersion: '4.2.0', + ...overrides, + }) + + it('aborts when the runtime declares a minEditorVersion above this editor', async () => { + const port = makePort({ + checkRuntimeVersion: jest.fn().mockResolvedValue({ + ok: true, + version: 'v4.2.0', + minEditorVersion: '4.2.1', + }), + }) + const { events, emit } = captureEvents() + const result = await runCompilePipeline(v4Args(), port, emit) + + expect(result.success).toBe(false) + expect(port.uploadRuntimeV4).not.toHaveBeenCalled() + expect(events.some((e) => /older than the runtime requires \(4\.2\.1\)/.test(e.message))).toBe(true) + }) + + // The second, independent way the gate stays inert: a caller that never + // opts in. Web passes no `editorVersion` until its adapter wires one up, + // and must keep uploading. + it('uploads when the caller passes no editorVersion, even against a declared floor', async () => { + const port = makePort({ + checkRuntimeVersion: jest.fn().mockResolvedValue({ ok: true, version: 'v4.2.0', minEditorVersion: '99.0.0' }), + }) + const { emit } = captureEvents() + const result = await runCompilePipeline(v4Args({ editorVersion: undefined }), port, emit) + + expect(result.success).toBe(true) + expect(port.uploadRuntimeV4).toHaveBeenCalledTimes(1) + }) + + // This is every runtime currently in the field: it answers /api/version + // only, so no floor reaches the pipeline. The gate must be completely + // inert for them — that is what makes shipping this safe. + const NO_FLOOR_DECLARED: Array<[string | null | undefined, string]> = [ + [undefined, 'the field is absent (runtime predates /api/capabilities)'], + [null, 'the runtime declares no floor'], + ] + + it.each(NO_FLOOR_DECLARED)('uploads normally when minEditorVersion is %p — %s', async (minEditorVersion) => { + const port = makePort({ + checkRuntimeVersion: jest.fn().mockResolvedValue({ ok: true, version: 'v4.1.7', minEditorVersion }), + }) + const { emit } = captureEvents() + const result = await runCompilePipeline(v4Args(), port, emit) + + expect(result.success).toBe(true) + expect(result.uploaded).toBe(true) + expect(port.uploadRuntimeV4).toHaveBeenCalledTimes(1) + }) + + it('uploads when this editor satisfies the runtime-declared floor', async () => { + const port = makePort({ + checkRuntimeVersion: jest.fn().mockResolvedValue({ ok: true, version: 'v4.2.0', minEditorVersion: '1.0.0' }), + }) + const { emit } = captureEvents() + const result = await runCompilePipeline(v4Args(), port, emit) + + expect(result.success).toBe(true) + expect(port.uploadRuntimeV4).toHaveBeenCalledTimes(1) + }) + + it('aborts when the VPP requires a newer runtime than the device reports', async () => { + const port = makePort({ + checkRuntimeVersion: jest.fn().mockResolvedValue({ ok: true, version: 'v4.1.7' }), + packageVppPlugin: jest.fn().mockResolvedValue({ files: {}, minRuntimeVersion: '4.1.9' }), + }) + const { events, emit } = captureEvents() + const result = await runCompilePipeline(v4Args(), port, emit) + + expect(result.success).toBe(false) + // Blocked BEFORE the upload: the failure this prevents is a vendor plugin + // that loads on a live PLC and dies at scan time. + expect(port.uploadRuntimeV4).not.toHaveBeenCalled() + expect(events.some((e) => /needs runtime 4\.1\.9 or newer/.test(e.message))).toBe(true) + }) + + it('uploads when the runtime satisfies the VPP floor', async () => { + const port = makePort({ + checkRuntimeVersion: jest.fn().mockResolvedValue({ ok: true, version: 'v4.2.0' }), + packageVppPlugin: jest.fn().mockResolvedValue({ files: {}, minRuntimeVersion: '4.1.9' }), + }) + const { emit } = captureEvents() + const result = await runCompilePipeline(v4Args(), port, emit) + + expect(result.success).toBe(true) + expect(port.uploadRuntimeV4).toHaveBeenCalledTimes(1) + }) + + it('uploads when the board is not from a VPP (no floor declared)', async () => { + const port = makePort({ + checkRuntimeVersion: jest.fn().mockResolvedValue({ ok: true, version: 'v4.1.7' }), + packageVppPlugin: jest.fn().mockResolvedValue({ files: {}, minRuntimeVersion: null }), + }) + const { emit } = captureEvents() + const result = await runCompilePipeline(v4Args(), port, emit) + + expect(result.success).toBe(true) + expect(port.uploadRuntimeV4).toHaveBeenCalledTimes(1) + }) + it('compileOnly on v4 returns success without invoking checkRuntimeVersion or uploadRuntimeV4', async () => { const port = makePort() const { emit } = captureEvents() diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index c3922edf1..1944d15bb 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -20,6 +20,7 @@ * `emit` callback (progress events). No disk I/O, no globals. */ +import { isVersionAtLeast } from '../../../frontend/utils/semver' import type { CompilerPlatformPort, PlatformDeviceContext, @@ -30,7 +31,12 @@ import { composeRuntimeV4Bundle } from '../../../middleware/shared/utils/library import { resolveTargetCapabilities } from '../../../middleware/shared/utils/target-capabilities' import type { BoardHalsCompileEntry } from '../firmware/build-arduino-cli-args' import { buildArduinoCliCompileArgs } from '../firmware/build-arduino-cli-args' -import { describeIncompatibleRuntime, isStrucppCompatibleRuntime } from '../firmware/runtime-version-gate' +import { + describeEditorTooOldForRuntime, + describeIncompatibleRuntime, + describeVppRuntimeMismatch, + isStrucppCompatibleRuntime, +} from '../firmware/runtime-version-gate' import { buildKnownPous, emitCompileErrorEvents } from '../library/program-build-helpers' import { runProgramBuildPipeline } from '../library/program-build-pipeline' import type { DevicePin } from '../types/PLC/devices' @@ -211,6 +217,19 @@ export interface RunCompilePipelineArgs { * addresses without re-reading the file. Called once per * successful strucpp compile. */ cacheDebugData?: (md5: string, debugMapJson: string) => void + /** + * This editor's own version, compared against the `minEditorVersion` + * a runtime publishes at `GET /api/capabilities` (DOPE-448). + * + * Injected rather than imported: `APP_VERSION` lives in + * `frontend/data/`, and the layer rules forbid `backend/shared/` + * from reaching into `data` — correctly, since which build is + * running is a fact about the host app, not about the compile. + * + * Absent means the caller opts out of the check, so the gate is + * inert for callers written before it existed. + */ + editorVersion?: string /** Persisted VPP Modbus screen state for the target device, * sourced from `DeviceConfiguration.vendorScreenData` under * the `modbus_rtu` / `modbus_tcp` keys. Threaded straight @@ -353,6 +372,7 @@ async function runCompilePipelineInner( cacheDebugData, vppModbusState, vendorScreenData, + editorVersion, } = args // Resolve the board's effective capabilities from `boardEntry`. @@ -588,6 +608,51 @@ async function runCompilePipelineInner( return bailError(emit, 'runtime-version', describeIncompatibleRuntime(versionCheck.version)) } + // The other direction (DOPE-448): the runtime published a + // `minEditorVersion` at `/api/capabilities` and this editor is below + // it. Inert in two independent ways, both of which describe the + // world as it is today: `versionCheck.minEditorVersion` is null for + // every runtime predating that endpoint, and `editorVersion` is + // absent for any caller that hasn't opted in — `isVersionAtLeast` + // passes on an absent floor either way. + if (editorVersion && !isVersionAtLeast(editorVersion, versionCheck.minEditorVersion)) { + return bailError( + emit, + 'runtime-version', + describeEditorTooOldForRuntime({ + runtimeVersion: versionCheck.version, + // Narrowed by the guard above: `isVersionAtLeast` only returns + // false when it parsed a real floor out of this field. + minEditorVersion: versionCheck.minEditorVersion ?? '', + editorVersion, + // Only the editor's direct-HTTPS context knows an address the + // user would recognise; on web the device sits behind an + // orchestrator agent, so the message omits the label rather + // than printing an agent id nobody can act on. + deviceLabel: deviceContext.kind === 'editor-https' ? deviceContext.ip : undefined, + }), + ) + } + + // Arc 4 (DOPE-448): the VPP whose HAL is about to be built on this + // device declares a runtime floor, and this runtime is below it. + // Checked here rather than at install time because the target device + // is unknown until the user connects to one — and checked BEFORE the + // upload, because the failure mode it prevents is a plugin that + // loads on a live PLC and dies at scan time. + if (!isVersionAtLeast(versionCheck.version, vppResult.minRuntimeVersion)) { + return bailError( + emit, + 'runtime-version', + describeVppRuntimeMismatch({ + boardTarget, + minRuntimeVersion: vppResult.minRuntimeVersion ?? '', + runtimeVersion: versionCheck.version, + deviceLabel: deviceContext.kind === 'editor-https' ? deviceContext.ip : undefined, + }), + ) + } + emit({ stage: 'upload', message: 'Uploading Runtime v4 bundle...', level: 'info' }) const uploadResult = await port.uploadRuntimeV4({ bundle, context: deviceContext }, makePlatformLog(emit, 'upload')) if (!uploadResult.ok) { diff --git a/src/backend/shared/library/__tests__/probe-runtime-version.test.ts b/src/backend/shared/library/__tests__/probe-runtime-version.test.ts index e090b5c77..27030b35a 100644 --- a/src/backend/shared/library/__tests__/probe-runtime-version.test.ts +++ b/src/backend/shared/library/__tests__/probe-runtime-version.test.ts @@ -16,7 +16,7 @@ describe('probeRuntimeVersion', () => { fetchVersion: async () => ({ success: true, body: { version: '4.1.2' } }), log, }) - expect(result).toEqual({ version: '4.1.2' }) + expect(result).toEqual({ version: '4.1.2', minEditorVersion: null }) expect(log).not.toHaveBeenCalled() }) @@ -26,7 +26,7 @@ describe('probeRuntimeVersion', () => { fetchVersion: async () => ({ success: true, body: { version: '4.0.5' } }), log, }) - expect(result).toEqual({ version: '4.0.5' }) + expect(result).toEqual({ version: '4.0.5', minEditorVersion: null }) }) it('returns version=null and logs a warning when the transport fails', async () => { @@ -35,7 +35,7 @@ describe('probeRuntimeVersion', () => { fetchVersion: async () => ({ success: false, error: 'ECONNREFUSED' }), log, }) - expect(result).toEqual({ version: null }) + expect(result).toEqual({ version: null, minEditorVersion: null }) expect(log).toHaveBeenCalledWith(expect.stringContaining('Could not reach runtime: ECONNREFUSED'), 'warning') }) @@ -47,7 +47,7 @@ describe('probeRuntimeVersion', () => { }, log, }) - expect(result).toEqual({ version: null }) + expect(result).toEqual({ version: null, minEditorVersion: null }) expect(log).toHaveBeenCalledWith( expect.stringContaining('Runtime version probe failed: orchestrator HTTP down'), 'warning', @@ -63,7 +63,7 @@ describe('probeRuntimeVersion', () => { }, log, }) - expect(result).toEqual({ version: null }) + expect(result).toEqual({ version: null, minEditorVersion: null }) expect(log).toHaveBeenCalledWith(expect.stringContaining('plain string failure'), 'warning') }) @@ -73,7 +73,7 @@ describe('probeRuntimeVersion', () => { fetchVersion: async () => ({ success: true, body: { otherField: 'noise' } }), log, }) - expect(result).toEqual({ version: null }) + expect(result).toEqual({ version: null, minEditorVersion: null }) expect(log).not.toHaveBeenCalled() }) @@ -83,7 +83,7 @@ describe('probeRuntimeVersion', () => { fetchVersion: async () => ({ success: true, body: { version: 4 } }), log, }) - expect(result).toEqual({ version: null }) + expect(result).toEqual({ version: null, minEditorVersion: null }) }) it('returns version=null when the body is null', async () => { @@ -92,7 +92,7 @@ describe('probeRuntimeVersion', () => { fetchVersion: async () => ({ success: true, body: null }), log, }) - expect(result).toEqual({ version: null }) + expect(result).toEqual({ version: null, minEditorVersion: null }) }) it('returns version=null when the body is a primitive (not an object)', async () => { @@ -101,6 +101,151 @@ describe('probeRuntimeVersion', () => { fetchVersion: async () => ({ success: true, body: 'a string' }), log, }) - expect(result).toEqual({ version: null }) + expect(result).toEqual({ version: null, minEditorVersion: null }) + }) +}) + +// --------------------------------------------------------------------------- +// /api/capabilities (DOPE-448) +// --------------------------------------------------------------------------- + +describe('probeRuntimeVersion — capabilities endpoint', () => { + /** A `fetchVersion` that fails the test if the fallback is reached. */ + const versionMustNotBeCalled = () => { + const spy = jest.fn(async () => ({ success: true as const, body: { version: 'FALLBACK' } })) + return spy + } + + it('prefers the capabilities endpoint and reads both fields from it', async () => { + const log = jest.fn() + const fetchVersion = versionMustNotBeCalled() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ + success: true, + body: { runtimeVersion: 'v4.2.0', minEditorVersion: '4.2.1' }, + }), + fetchVersion, + log, + }) + expect(result).toEqual({ version: 'v4.2.0', minEditorVersion: '4.2.1' }) + // One round-trip, not two: the capabilities answer is complete. + expect(fetchVersion).not.toHaveBeenCalled() + expect(log).not.toHaveBeenCalled() + }) + + it('reports minEditorVersion=null when the endpoint answers without that field', async () => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ success: true, body: { runtimeVersion: 'v4.2.0' } }), + fetchVersion: versionMustNotBeCalled(), + log, + }) + expect(result).toEqual({ version: 'v4.2.0', minEditorVersion: null }) + }) + + it('ignores a non-string minEditorVersion rather than passing it to a comparison', async () => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ + success: true, + body: { runtimeVersion: 'v4.2.0', minEditorVersion: 421 }, + }), + fetchVersion: versionMustNotBeCalled(), + log, + }) + expect(result).toEqual({ version: 'v4.2.0', minEditorVersion: null }) + }) + + // The single most important case: this is every runtime currently + // deployed. The fallback must be silent and unremarkable — if it warned, + // every existing device would nag on every upload. + // + // Note the 401: a pre-DOPE-448 runtime does NOT answer 404 for an unknown + // path. Its `restapi.py` ends in a catch-all `/` route guarded by + // `@jwt_required()`, so `/api/capabilities` lands there and comes back as + // "Missing Authorization Header". Observed against a real container — the + // 404 row is kept because a future runtime could answer either way. + const LEGACY_RESPONSES: Array<[string, string]> = [ + ['401 Missing Authorization Header', 'the / catch-all swallows the unknown path'], + ['404 Not Found', 'a runtime that routes unknown paths properly'], + ] + + it.each(LEGACY_RESPONSES)('falls back to /api/version on %p (%s), without warning', async (error) => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ success: false, error }), + fetchVersion: async () => ({ success: true, body: { version: 'v4.1.7' } }), + log, + }) + expect(result).toEqual({ version: 'v4.1.7', minEditorVersion: null }) + expect(log).not.toHaveBeenCalled() + }) + + // Belt and braces: if a transport surfaces the 401 as a *successful* fetch + // carrying the error body (rather than as a failure), the probe must still + // fall back — there is no `runtimeVersion` to read out of it. + it('falls back when the 401 body arrives as a successful fetch', async () => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ success: true, body: { msg: 'Missing Authorization Header' } }), + fetchVersion: async () => ({ success: true, body: { version: 'v4.1.7' } }), + log, + }) + expect(result).toEqual({ version: 'v4.1.7', minEditorVersion: null }) + expect(log).not.toHaveBeenCalled() + }) + + it('falls back when the capabilities transport throws', async () => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => { + throw new Error('TLS handshake failed') + }, + fetchVersion: async () => ({ success: true, body: { version: 'v4.1.7' } }), + log, + }) + expect(result).toEqual({ version: 'v4.1.7', minEditorVersion: null }) + expect(log).not.toHaveBeenCalled() + }) + + // A partial answer is rejected wholesale: if we cannot read the version out + // of this response we do not trust the minEditorVersion beside it either. + const UNUSABLE_BODIES: Array<[unknown, string]> = [ + [{ minEditorVersion: '4.2.1' }, 'runtimeVersion is missing'], + [{ runtimeVersion: 42, minEditorVersion: '4.2.1' }, 'runtimeVersion is not a string'], + [null, 'the body is null'], + ['a string', 'the body is a primitive'], + ] + + it.each(UNUSABLE_BODIES)('falls back when %j (%s)', async (body) => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ success: true, body }), + fetchVersion: async () => ({ success: true, body: { version: 'v4.1.7' } }), + log, + }) + expect(result).toEqual({ version: 'v4.1.7', minEditorVersion: null }) + }) + + it('behaves exactly as before when no capabilities transport is wired', async () => { + // Platforms that have not adopted the endpoint yet (and every caller + // written before it existed) keep their previous behaviour. + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchVersion: async () => ({ success: true, body: { version: '4.1.2' } }), + log, + }) + expect(result).toEqual({ version: '4.1.2', minEditorVersion: null }) + }) + + it('still warns about an unreachable device when the fallback also fails', async () => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ success: false, error: '404 Not Found' }), + fetchVersion: async () => ({ success: false, error: 'ECONNREFUSED' }), + log, + }) + expect(result).toEqual({ version: null, minEditorVersion: null }) + expect(log).toHaveBeenCalledWith(expect.stringContaining('Could not reach runtime: ECONNREFUSED'), 'warning') }) }) diff --git a/src/backend/shared/library/probe-runtime-version.ts b/src/backend/shared/library/probe-runtime-version.ts index ecf1b3cfc..1190265c0 100644 --- a/src/backend/shared/library/probe-runtime-version.ts +++ b/src/backend/shared/library/probe-runtime-version.ts @@ -50,6 +50,25 @@ export interface ProbeRuntimeVersionOptions { * hit the device's `/api/version`; web POSTs to the * orchestrator's `run-command` with `api: 'api/version'`. */ fetchVersion(): Promise + /** + * Transport callback for `GET /api/capabilities` — the endpoint + * where a runtime declares what it requires of an editor + * (DOPE-448). Optional: a platform that hasn't wired it up yet + * behaves exactly as before. + * + * A runtime predating the endpoint does NOT answer 404. Its + * `restapi.py` ends in a catch-all `@restapi_bp.route("/")` + * guarded by `@jwt_required()`, so an unknown path under `/api/` + * falls into it and comes back as **401 Missing Authorization + * Header** — verified against a real pre-DOPE-448 container. Both + * outcomes land here the same way (a failed fetch, or a body with + * no `runtimeVersion`), which is why this probe keys off "can I + * read a version out of the answer" rather than off a status code. + * + * Either way `minEditorVersion` comes back `null`, meaning "this + * runtime declares no floor" — never "the editor is too old". + */ + fetchCapabilities?(): Promise /** Warning channel for diagnostics the user can see in the * compile console (e.g. "Could not reach runtime: ECONNREFUSED"). * Wired to the platform port's `log` callback by the caller so @@ -62,24 +81,74 @@ export interface ProbeRuntimeVersionResult { * when the probe couldn't extract one. The shared compile * pipeline feeds this verbatim to `isStrucppCompatibleRuntime`. */ version: string | null + /** + * The oldest editor this runtime accepts programs from, as + * declared at `GET /api/capabilities`, or `null` when the runtime + * declares nothing — it predates the endpoint, the platform has no + * transport for it, or the field was missing/malformed. + * + * `null` must be treated as "no constraint", not as a failure: it + * is the state of every runtime currently in the field, and the + * whole point of the runtime advertising rather than enforcing is + * that shipping this can't lock those out. + */ + minEditorVersion: string | null } /** * Run the probe. Always resolves — never throws — so the pipeline * gets a deterministic answer it can branch on. + * + * `/api/capabilities` is preferred where available because it carries + * both halves of the compatibility question in one round-trip, and it + * reports the version under `runtimeVersion` rather than `version`. + * When it is absent or unusable the probe falls back to + * `/api/version`, which every runtime has. */ export async function probeRuntimeVersion(opts: ProbeRuntimeVersionOptions): Promise { + const capabilities = await tryFetchCapabilities(opts) + if (capabilities) return capabilities + try { const result = await opts.fetchVersion() if (!result.success) { opts.log(`Could not reach runtime: ${result.error}`, 'warning') - return { version: null } + return { version: null, minEditorVersion: null } } - return { version: extractVersionFromBody(result.body) } + return { version: extractVersionFromBody(result.body), minEditorVersion: null } } catch (error) { const message = error instanceof Error ? error.message : String(error) opts.log(`Runtime version probe failed: ${message}`, 'warning') - return { version: null } + return { version: null, minEditorVersion: null } + } +} + +/** + * Attempt the capabilities endpoint. Returns null — meaning "fall + * back to /api/version" — for every unusable outcome: no transport + * wired, a 404 from a runtime that predates the endpoint, a thrown + * transport error, or a body with no usable `runtimeVersion`. + * + * A partial answer is deliberately not accepted. If we cannot read + * the version out of this response we do not trust the + * `minEditorVersion` beside it either, and `/api/version` is the + * authority on the version anyway. + */ +async function tryFetchCapabilities(opts: ProbeRuntimeVersionOptions): Promise { + if (!opts.fetchCapabilities) return null + try { + const result = await opts.fetchCapabilities() + if (!result.success) { + // Expected against every runtime older than the endpoint — in + // practice a 401 from the `/` catch-all, not a 404 — so + // this is an ordinary fact, not a problem worth a warning. + return null + } + const version = extractStringField(result.body, 'runtimeVersion') + if (version === null) return null + return { version, minEditorVersion: extractStringField(result.body, 'minEditorVersion') } + } catch { + return null } } @@ -91,8 +160,18 @@ export async function probeRuntimeVersion(opts: ProbeRuntimeVersionOptions): Pro * answer as incompatible. */ function extractVersionFromBody(body: unknown): string | null { + return extractStringField(body, 'version') +} + +/** + * Read a top-level string field out of a response body, collapsing + * every other shape (not an object, field absent, field not a string) + * to `null` so callers get one "unknown" value to branch on instead of + * having to distinguish the ways a body can disappoint them. + */ +function extractStringField(body: unknown, field: string): string | null { if (typeof body !== 'object' || body === null) return null - if (!('version' in body)) return null - const v = (body as { version: unknown }).version - return typeof v === 'string' ? v : null + if (!(field in body)) return null + const value = (body as Record)[field] + return typeof value === 'string' ? value : null } diff --git a/src/middleware/shared/ports/compiler-platform-port.ts b/src/middleware/shared/ports/compiler-platform-port.ts index c7698ac92..cc13ae2fa 100644 --- a/src/middleware/shared/ports/compiler-platform-port.ts +++ b/src/middleware/shared/ports/compiler-platform-port.ts @@ -236,6 +236,17 @@ export interface CheckRuntimeVersionResult { * when the runtime is unreachable or doesn't expose the * endpoint (very old v3 runtimes). */ version: string | null + /** + * Oldest editor this runtime accepts programs from, declared at + * `GET /api/capabilities` (DOPE-448). `null` means the runtime + * declares no floor — it predates the endpoint, or this platform + * has no transport for it. + * + * `null` is "no constraint", never "too old": every runtime + * currently deployed answers `null`, and the runtime only + * advertises this value — the editor is what compares and refuses. + */ + minEditorVersion?: string | null } /** VPP (Vendor Plugin Package) runtime-v4 packaging. Boards that @@ -263,6 +274,23 @@ export interface PackageVppPluginResult { * and return an empty record without errors. */ files: Record errors?: StructuredCompileError[] + /** + * `package.minRuntimeVersion` from the manifest of the VPP this + * board came from (DOPE-448) — the oldest runtime whose plugin API + * the package's HAL was built against. + * + * `null`/absent for non-VPP boards, for packages that declare no + * floor, and on platforms without VPP integration. The pipeline + * compares it against the connected runtime's reported version + * right after the version probe, which is the earliest point where + * both halves are known — a VPP plugin is built against a runtime + * API, so an older runtime loads it and fails at scan time, on a + * live PLC. + * + * This cannot be enforced at install time: the target device is + * unknown until the user connects to one. + */ + minRuntimeVersion?: string | null } // --------------------------------------------------------------------------- From 7e57734f4efee86922e115b648f83d5c2ccb13ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Thu, 6 Aug 2026 10:57:17 +0200 Subject: [PATCH 4/9] docs(compat): record the editor/runtime/VPP compatibility strategy (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three artefacts on independent release cadences, five arcs that can be mismatched in both directions, and four disconnected checks that were each invented separately. Documents the agreed design — each component declares a minimum, the editor is the only component that compares — plus what exists today with file:line references, the error-message rules, and the delivery phases. Section 9 records what was considered and dropped, so nobody re-derives it: monotonic integer contracts, a bundle-manifest.json inside the upload ZIP, an editor->runtime advertising handshake, and max* bounds (an upper bound naming releases that do not exist yet is unknowable). Section 7 records the accepted limitation in its own section: the runtime advertises rather than enforces, so a client that skips the check can still upload. Co-Authored-By: Claude Opus 5 --- docs/version-compatibility-strategy.md | 350 +++++++++++++++++++++++++ 1 file changed, 350 insertions(+) create mode 100644 docs/version-compatibility-strategy.md diff --git a/docs/version-compatibility-strategy.md b/docs/version-compatibility-strategy.md new file mode 100644 index 000000000..f5495657f --- /dev/null +++ b/docs/version-compatibility-strategy.md @@ -0,0 +1,350 @@ +# Editor + Runtime + VPP Version Compatibility — Strategy + +> Status: **agreed design** (Marcone + Thiago Alves, 2026-08-05) +> Jira: [DOPE-448](https://autonomylogic.atlassian.net/browse/DOPE-448) (epic DOPE-317 — VPP in the Editor) +> Scope: `openplc-editor` + `openplc-web` (shared surface — byte-identical), +> `openplc-runtime`, `openplc-packages` + +## 1. The decision + +Three declarations, one comparer. + +| Component | Declares | Where it lives | +| ----------- | ---------------------------------------- | ---------------------------------------------- | +| **VPP** | `minEditorVersion` + `minRuntimeVersion` | `manifest.json` → `package` (openplc-packages) | +| **Runtime** | `minEditorVersion` | new `GET /api/capabilities` (openplc-runtime) | +| **Editor** | `minRuntimeVersion` | global constant in the shared surface | + +**The editor is the only component that compares.** The runtime publishes what it +requires and never enforces it; there is no editor→runtime advertising handshake +and nothing new travels inside the upload bundle. + +Two consequences of that choice, both good: + +- **No release-ordering constraint between phases.** A runtime that starts + publishing `minEditorVersion` changes nothing for editors that don't read it + yet. Every phase below is independently shippable in any order, and none can + break a device already in the field. +- **One place to debug.** When an upload is refused, the decision was made in the + editor, with both version strings in hand. + +The accepted trade-off: the runtime _advertises_ rather than _enforces_, so a +client that skips the check can still upload. This protects the real case (our +editor against our runtime) and is not a security boundary — see §7. + +## 2. What is missing today, per declaration + +### 2.1 VPP — the field exists, nothing checks it + +`schema/manifest.schema.json` already makes `package.minEditorVersion` +**required**, and `openplc-packages/docs/package-format.md:69` already promises: + +> `minEditorVersion`: Editor will refuse to install packages requiring a newer version. + +**That promise is not implemented.** The only consumer is the remote catalog UI — +`catalog-browser.tsx:272` uses `isCompatibleEditorVersion(v.minEditorVersion, APP_VERSION)` +to render an "Editor outdated" button state. + +`package-manager-module.ts::install` — the single trust boundary that _both_ the +remote install and the local "Add from file…" flow pass through, which already +validates the manifest schema, verifies the package signature and hardens the +path — **never looks at `minEditorVersion`**. So today: + +- installing a `.vpp` from disk ignores it entirely; +- a package installed before an editor downgrade keeps loading; +- compiling against an incompatible package is never blocked. + +This matters more than a missing field would, because the agreed design _relies_ +on this mechanism: a VPP that needs a UI engine only present from editor 4.2.1 +onward is expected to be unable to install on 4.2.0. Making that true is the +first piece of work. + +`minRuntimeVersion` does not exist in the schema at all. It is needed because a +`runtime-v4-plugin` HAL is code that runs **inside the runtime process** — +`apply_vpp_plugin_conf()` installs its conf on every upload — so a package built +against a newer runtime plugin API currently installs cleanly and fails at load +or scan time. + +### 2.2 Runtime — publishes a version, requires nothing + +`webserver/version.py` resolves `RUNTIME_VERSION` (baked in at image build time +via `ARG RUNTIME_VERSION`), exposed at `GET /api/version` and on the +`X-OpenPLC-Runtime-Version` header (`webserver/restapi.py:46`). + +There is no endpoint where the runtime states what it needs from an editor. +`handle_upload_file` accepts any ZIP that passes `analyze_zip` (path traversal, +size, ZIP-bomb ratio, denylisted executable extensions). + +### 2.3 Editor — the constant exists under a narrower name + +`src/backend/shared/firmware/runtime-version-gate.ts` (shared surface) already +holds the editor's floor: + +```ts +export const MIN_STRUCPP_RUNTIME_VERSION = '4.1.0' +``` + +This _is_ the global `minRuntimeVersion` the design calls for — it blocks upload +to any runtime below it. It only needs a name that says so. + +**It must not absorb the per-feature gates.** The same file also holds: + +```ts +export const MIN_USER_MANAGEMENT_RUNTIME_VERSION = '4.1.9' +``` + +That one hides a UI screen; it does not block upload. Collapsing the two into one +number forces a bad choice: declare `4.1.0` and the User Management screen breaks +on a 4.1.7 runtime; declare `4.1.9` and upload is blocked on a 4.1.7 runtime that +handles upload perfectly. They stay separate. + +### 2.4 Two semver parsers with divergent semantics + +The whole design rests on comparing version strings, and the codebase currently +answers "is X at least Y" two different ways: + +| | `frontend/utils/semver.ts` | `firmware/runtime-version-gate.ts` | +| ----------------------- | -------------------------- | ---------------------------------- | +| Consumers | VPP catalog | runtime gates | +| `"v4"` | `4.0.0` | `null` (rejected) | +| `"4.1"` | `4.1.0` | `null` (rejected) | +| `"garbage"` | `0.0.0` (lowest) | `null` (rejected) | +| `4.1.0-rc.3` vs `4.1.0` | equal (suffix stripped) | equal, **deliberately** | +| Failure mode | degrade to lowest | fail closed | + +Both are individually well-reasoned. Together they mean the three comparisons in +§1 could disagree depending on which helper a call site happened to import — +exactly on the malformed and pre-release inputs that show up in the field. +Unifying them is cheap and is a precondition for everything else. + +## 3. How it works + +### 3.1 Runtime → Editor + +Scenario: editor **4.2.10**, Raspberry Pi at **192.168.1.50** running runtime **4.2.0**. + +``` +GET http://192.168.1.50/api/capabilities +``` + +```json +{ + "runtimeVersion": "v4.2.0", + "minEditorVersion": "4.2.1" +} +``` + +The editor compares, both directions, locally: + +``` +runtime 4.2.0 >= editor's MIN_RUNTIME_VERSION (4.1.0)? yes → ok +editor 4.2.10 >= runtime's minEditorVersion (4.2.1)? yes → ok +→ upload proceeds +``` + +Reverse case — editor **4.2.0** against the same runtime: + +``` +editor 4.2.0 >= runtime's minEditorVersion (4.2.1)? NO → blocked +``` + +Nothing is sent. This is the "and vice-versa" direction from the card, and it is +what does not exist today. + +**Legacy runtime** — `GET /api/capabilities` returns `404`: + +``` +GET /api/version → {"version": "v4.1.7"} + +runtime declares no floor → nothing to check in that direction +runtime 4.1.7 >= MIN_RUNTIME_VERSION (4.1.0)? yes → upload proceeds +user-management needs 4.1.9? no → screen hidden +``` + +Identical to today's behaviour, plus one console warning that the runtime does +not publish its requirements. + +### 3.2 VPP → Editor, at install time + +```json +{ + "package": { + "id": "com.automationdirect.p1am", + "version": "2.0.0", + "minEditorVersion": "4.2.1", + "minRuntimeVersion": "4.1.9" + } +} +``` + +`package-manager-module.ts::install`, right after the signature check: + +``` +manifest schema valid? ok +signature verified? ok +package.id safe as a path component? ok +APP_VERSION (4.2.0) >= minEditorVersion (4.2.1)? NO → install rejected +``` + +Covers both entry paths — remote catalog install and local "Add from file…" — +because both converge here. `catalog-browser.tsx` keeps its "Editor outdated" +state but derives it from the shared helper rather than owning the decision. + +This is the mechanism that answers "what about a VPP that needs a UI engine the +editor may not have": the engine landed in some editor release, the package +declares that release as its floor, and an older editor cannot install it. + +### 3.3 VPP → Runtime, at compile time + +`minRuntimeVersion` cannot be checked at install — the target runtime is unknown +until you connect to a device. So it is checked in the compile pipeline, when the +selected board comes from a VPP whose target type is `runtime-v4`: + +``` +VPP requires runtime >= 4.1.9 +connected runtime reports v4.1.7 +→ compile blocked +``` + +## 4. Where each check lives + +| Check | Gate | Failure surface | +| ---------------------------------- | ----------------------------------------------------------------- | ---------------------------------- | +| runtime new enough for this editor | `probe-runtime-version.ts` + `MIN_RUNTIME_VERSION` | upload blocked pre-compile | +| editor new enough for this runtime | `probe-runtime-version.ts` + `minEditorVersion` from the endpoint | upload blocked pre-compile | +| editor new enough for this VPP | `package-manager-module.ts::install` (+ on load) | install rejected; package unusable | +| runtime new enough for this VPP | compile pipeline, `runtime-v4` targets | compile blocked | +| per-feature runtime capability | existing predicates in `runtime-version-gate.ts` | UI surface hidden | + +Every row is decided in the editor. The runtime and the VPP only declare. + +## 5. Error messages + +A gate that fires is a support ticket unless the message is complete. Following +the existing `describeIncompatibleRuntime`, every rejection names **what** was +refused, **which two versions** disagree, **which side** is stale, and **the one +action** that fixes it. + +``` +Runtime v4.2.0 requires OpenPLC Editor 4.2.1 or newer. +This editor is 4.2.0. +Update the editor, or connect to a runtime that accepts 4.2.0. +``` + +``` +Package "AutomationDirect P1AM" 2.0.0 requires OpenPLC Editor 4.2.1 or newer. +This editor is 4.2.0. +Update the editor, or install package version 1.4.2. +``` + +``` +Package "AutomationDirect P1AM" 2.0.0 requires runtime v4.1.9 or newer. +The runtime at 192.168.1.50 reports v4.1.7. +Upgrade the runtime on that device. +``` + +Rules: always name the device when one is involved; always state the direction — +never "incompatible versions". + +## 6. Peers that declare nothing + +Everything already in the field predates this work, so absence must be a +supported state rather than an error. + +| Peer state | Behaviour | +| -------------------------------------------------------- | -------------------------------------------------------------------------------------------- | +| Runtime without `/api/capabilities` | no editor floor to check; existing `MIN_RUNTIME_VERSION` gate still applies; console warning | +| Runtime version unparseable (`"v4"`, `"dev"`) | fail closed — current behaviour, unchanged | +| VPP without `minRuntimeVersion` on a `runtime-v4` target | install allowed, runtime match unverifiable, console warning at install | + +Because the runtime never enforces, no closing window is needed on the upload +path: an old runtime simply contributes no constraint. The only future tightening +worth scheduling is making `minRuntimeVersion` mandatory for `runtime-v4` +packages, which is enforced at **package build time** (`scripts/validate.ts`) and +so never breaks an installed package. + +## 7. Accepted limitation + +The runtime advertises its requirement; it does not enforce it. A client that +does not implement the check — an older editor, or any other tool speaking the +upload API — can still push a program. + +This was decided deliberately: it keeps the upload path untouched, requires no +new metadata inside the bundle, and removes any possibility of a runtime release +locking out editors already installed. It is not a security control, and should +not be described as one. If enforcement is ever needed, the natural follow-up is +for the editor to send its version on upload and for the runtime to compare — +additive, and not required by anything in this plan. + +## 8. Delivery phases + +No ordering constraint between phases: the runtime never blocks, so a phase +shipping early cannot break a peer. Ordered by value delivered. + +**Phase 1 — one semver parser.** Unify `frontend/utils/semver.ts` and +`parseRuntimeVersion` into a single shared helper with one explicit policy for +malformed and pre-release inputs. Existing call sites keep their current +functions as thin wrappers so no behaviour changes and existing tests pass +untouched. Precondition for phases 2–4, which all compare version strings. +_Repos: editor + web (byte-identical) · ~half a day._ + +**Phase 2 — VPP install gate.** `package-manager-module.ts::install` checks +`minEditorVersion` against `APP_VERSION`, next to the signature verification; +same check when loading an already-installed package. `catalog-browser.tsx` +derives its state from the shared helper. Correct +`openplc-packages/docs/package-format.md:69` — it becomes true. +_Repos: editor + web · ~1 day._ + +**Phase 3 — `minRuntimeVersion` in the manifest.** Add the field to +`schema/manifest.schema.json`, conditionally required when any device targets +`runtime-v4`; enforce in `scripts/validate.ts` so a malformed package never +reaches a user; check it in the compile pipeline against the connected runtime. +_Repos: packages + editor + web · ~1 day._ + +**Phase 4 — `GET /api/capabilities`.** New unauthenticated endpoint returning +`runtimeVersion` + `minEditorVersion`, with the constant living beside +`RUNTIME_VERSION` in `webserver/version.py`. +_Repo: runtime · ~half a day._ + +**Phase 5 — editor consumes it.** `probe-runtime-version.ts` reads the endpoint, +treats `404` as "declares nothing", and blocks upload when +`APP_VERSION < minEditorVersion`. Rename `MIN_STRUCPP_RUNTIME_VERSION` to +`MIN_RUNTIME_VERSION` (keeping the old name as an alias) so the global reads as +what it is. +_Repos: editor + web · ~1 day._ + +**Phase 6 — tests.** Table of `(editorVersion, runtimeVersion, vppManifest) → +allow | block | warn` covering each of the four comparisons plus the +declares-nothing and unparseable rows. +_Repos: editor + web · ~half a day._ + +Total: **~4,5 days**. + +## 9. Explicitly out of scope + +Considered and dropped, so nobody re-derives them: + +- **Monotonic integer contracts** (`PROGRAM_CONTRACT_VERSION` and friends). + Would decouple the release trains and isolate a debug-protocol change from the + upload path, at the cost of a second versioning concept to maintain. Human + semver is the agreed key. +- **`bundle-manifest.json` inside the upload ZIP.** Not needed once the runtime + publishes its floor and the editor compares locally. +- **Editor→runtime advertising handshake** (`/api/adv` or similar). Decided + against: the editor validates. +- **`maxEditorVersion` / `maxRuntimeVersion`.** An upper bound pointing at + releases that do not exist yet is unknowable — any value either blocks + compatible future peers or does nothing. + +## 10. Open questions + +1. **What value does the runtime publish as `minEditorVersion` today?** Needs a + concrete audit of when the current bundle layout stabilised. Publishing a + floor that is too high locks out working editors; too low makes the field + decorative. Safest start is the oldest editor known to work with the strucpp + pipeline. +2. **Does an installed-but-incompatible VPP get hidden or shown-as-unusable?** + Hiding is cleaner; showing explains why a board disappeared after an editor + downgrade. +3. **Warning surface for peers that declare nothing** (§6) — console only, or a + one-time notice in the UI? From 97a12393924ab168f1e34a070b0a73aa1b65b52c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Thu, 6 Aug 2026 16:46:31 +0200 Subject: [PATCH 5/9] docs(compat): mark the strategy as shipped, not planned (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #993 caught that the doc reads as a roadmap for work this PR already contains: §2.2 stated there is no endpoint where the runtime declares what it needs, and §8 listed phases 1-6 with day estimates. A maintainer opening this in three months could reasonably redo or revert what already exists. - Header states "implemented", lists the four shipping PRs, and tells the reader §2 is a dated snapshot of the state BEFORE this work, kept because the design rationale only makes sense against what it was fixing. - §8 becomes a landed-where table plus per-phase notes, no estimates. Cross-checking every claim against the code turned up three the doc asserted and the code does not do — all now corrected to describe what actually ships: - §4 claimed the VPP editor-floor check also runs "on load". It does not; it runs at install only, so a package installed under a newer editor keeps loading after a downgrade. Recorded as a known gap instead of a feature. - §6 promised a console warning for a runtime with no /api/capabilities. The fallback is deliberately silent, and the section now says why: that is every deployed device, so warning there would fire on every upload. - §6 promised a console warning when a runtime-v4 VPP declares no minRuntimeVersion. There is none; that case is caught at package build time by validate.ts instead. Also corrects 404 → 401 for the legacy-runtime fallback (the / catch-all behind @jwt_required() swallows unknown paths), and turns §10 into decided-vs-open: the runtime's published floor is settled at 4.1.0, and the four findings from review are recorded with the note that three touch shared surface and need a mirrored commit in openplc-web. Docs only — no behaviour change. Co-Authored-By: Claude Opus 5 --- docs/version-compatibility-strategy.md | 227 ++++++++++++++++--------- 1 file changed, 149 insertions(+), 78 deletions(-) diff --git a/docs/version-compatibility-strategy.md b/docs/version-compatibility-strategy.md index f5495657f..29fb91ad1 100644 --- a/docs/version-compatibility-strategy.md +++ b/docs/version-compatibility-strategy.md @@ -1,9 +1,17 @@ # Editor + Runtime + VPP Version Compatibility — Strategy -> Status: **agreed design** (Marcone + Thiago Alves, 2026-08-05) +> Status: **agreed design, implemented** (Marcone + Thiago Alves, 2026-08-05) > Jira: [DOPE-448](https://autonomylogic.atlassian.net/browse/DOPE-448) (epic DOPE-317 — VPP in the Editor) > Scope: `openplc-editor` + `openplc-web` (shared surface — byte-identical), > `openplc-runtime`, `openplc-packages` +> Shipped in: openplc-editor#993 · openplc-runtime#163 · openplc-packages#28 · +> openplc-web#652 — see §8 for what each one covers. + +**Reading this later:** §1 is the design and stays true. **§2 is a snapshot of +the state _before_ this work, dated 2026-08-05** — it describes gaps the PRs +above have since closed, and is kept because the reasoning only makes sense +against what it was fixing. §8 marks what landed. Do not read §2 as a to-do +list. ## 1. The decision @@ -32,7 +40,11 @@ The accepted trade-off: the runtime _advertises_ rather than _enforces_, so a client that skips the check can still upload. This protects the real case (our editor against our runtime) and is not a security boundary — see §7. -## 2. What is missing today, per declaration +## 2. What was missing, per declaration — baseline as of 2026-08-05 + +_Historical. Every gap below is closed by the PRs listed at the top; §8 says +which. Kept verbatim because the design decisions in §1 and §3 only make sense +against the state they were correcting._ ### 2.1 VPP — the field exists, nothing checks it @@ -71,9 +83,11 @@ or scan time. via `ARG RUNTIME_VERSION`), exposed at `GET /api/version` and on the `X-OpenPLC-Runtime-Version` header (`webserver/restapi.py:46`). -There is no endpoint where the runtime states what it needs from an editor. -`handle_upload_file` accepts any ZIP that passes `analyze_zip` (path traversal, -size, ZIP-bomb ratio, denylisted executable extensions). +At the time of writing there was no endpoint where the runtime stated what it +needs from an editor, and `handle_upload_file` accepted any ZIP that passed +`analyze_zip` (path traversal, size, ZIP-bomb ratio, denylisted executable +extensions). `GET /api/capabilities` (§3.1) closes this in openplc-runtime#163; +the upload path itself is deliberately left untouched — see §7. ### 2.3 Editor — the constant exists under a narrower name @@ -148,21 +162,28 @@ Reverse case — editor **4.2.0** against the same runtime: editor 4.2.0 >= runtime's minEditorVersion (4.2.1)? NO → blocked ``` -Nothing is sent. This is the "and vice-versa" direction from the card, and it is -what does not exist today. +Nothing is sent. This is the "and vice-versa" direction from the card — the one +that had no enforcement at all before this work. -**Legacy runtime** — `GET /api/capabilities` returns `404`: +**Legacy runtime** — `GET /api/capabilities` fails. In practice with **401**, +not 404: the runtime's `restapi.py` ends in a +`@restapi_bp.route("/")` catch-all behind `@jwt_required()`, so an +unknown path under `/api/` falls into it and comes back "Missing Authorization +Header". Verified against a real pre-change container. The probe treats any +unreadable answer the same way, so both shapes fall back identically: ``` -GET /api/version → {"version": "v4.1.7"} +GET /api/capabilities → 401 (or 404) +GET /api/version → {"version": "v4.1.7"} runtime declares no floor → nothing to check in that direction runtime 4.1.7 >= MIN_RUNTIME_VERSION (4.1.0)? yes → upload proceeds user-management needs 4.1.9? no → screen hidden ``` -Identical to today's behaviour, plus one console warning that the runtime does -not publish its requirements. +Identical to the previous behaviour. The fallback is deliberately **silent** — +that 401 is the normal answer from every runtime already deployed, so warning on +it would nag on every upload. ### 3.2 VPP → Editor, at install time @@ -208,16 +229,20 @@ connected runtime reports v4.1.7 ## 4. Where each check lives -| Check | Gate | Failure surface | -| ---------------------------------- | ----------------------------------------------------------------- | ---------------------------------- | -| runtime new enough for this editor | `probe-runtime-version.ts` + `MIN_RUNTIME_VERSION` | upload blocked pre-compile | -| editor new enough for this runtime | `probe-runtime-version.ts` + `minEditorVersion` from the endpoint | upload blocked pre-compile | -| editor new enough for this VPP | `package-manager-module.ts::install` (+ on load) | install rejected; package unusable | -| runtime new enough for this VPP | compile pipeline, `runtime-v4` targets | compile blocked | -| per-feature runtime capability | existing predicates in `runtime-version-gate.ts` | UI surface hidden | +| Check | Gate | Failure surface | +| ---------------------------------- | ----------------------------------------------------------------- | ------------------------------------- | +| runtime new enough for this editor | `probe-runtime-version.ts` + `MIN_RUNTIME_VERSION` | upload blocked pre-compile | +| editor new enough for this runtime | `probe-runtime-version.ts` + `minEditorVersion` from the endpoint | upload blocked pre-compile | +| editor new enough for this VPP | `package-manager-module.ts::install` | install rejected, both versions named | +| runtime new enough for this VPP | compile pipeline, `runtime-v4` targets | compile blocked | +| per-feature runtime capability | existing predicates in `runtime-version-gate.ts` | UI surface hidden | Every row is decided in the editor. The runtime and the VPP only declare. +**Not covered: an already-installed package after an editor downgrade.** The +gate runs at install, so a package installed under 4.3 keeps loading on 4.2. +Re-checking on load would close it — see §10 for why it is still open. + ## 5. Error messages A gate that fires is a support ticket unless the message is complete. Following @@ -251,11 +276,19 @@ never "incompatible versions". Everything already in the field predates this work, so absence must be a supported state rather than an error. -| Peer state | Behaviour | -| -------------------------------------------------------- | -------------------------------------------------------------------------------------------- | -| Runtime without `/api/capabilities` | no editor floor to check; existing `MIN_RUNTIME_VERSION` gate still applies; console warning | -| Runtime version unparseable (`"v4"`, `"dev"`) | fail closed — current behaviour, unchanged | -| VPP without `minRuntimeVersion` on a `runtime-v4` target | install allowed, runtime match unverifiable, console warning at install | +| Peer state | Behaviour as implemented | +| -------------------------------------------------------- | ---------------------------------------------------------------------------------------------- | +| Runtime without `/api/capabilities` | no editor floor to check; `MIN_RUNTIME_VERSION` gate still applies; **silent** — see below | +| Runtime version unparseable (`"v4"`, `"dev"`) | fail closed — previous behaviour, unchanged | +| Runtime declares an _unreadable_ floor (`"4.2"`, junk) | floor ignored, upload proceeds, **no signal** — known gap, see §10 | +| VPP without `minRuntimeVersion` on a `runtime-v4` target | install allowed, runtime match unverifiable, no warning; blocked at package build time instead | + +The first row is silent **on purpose**: a runtime with no `/api/capabilities` is +every device currently deployed, so warning there would fire on every upload from +every editor. The third row is different — a floor that is present but malformed +is a mistake someone made, and swallowing it means a constraint the runtime +believes it is enforcing silently is not. That one deserves a warning and does +not have one yet. Because the runtime never enforces, no closing window is needed on the upload path: an old runtime simply contributes no constraint. The only future tightening @@ -276,49 +309,54 @@ not be described as one. If enforcement is ever needed, the natural follow-up is for the editor to send its version on upload and for the runtime to compare — additive, and not required by anything in this plan. -## 8. Delivery phases - -No ordering constraint between phases: the runtime never blocks, so a phase -shipping early cannot break a peer. Ordered by value delivered. - -**Phase 1 — one semver parser.** Unify `frontend/utils/semver.ts` and -`parseRuntimeVersion` into a single shared helper with one explicit policy for -malformed and pre-release inputs. Existing call sites keep their current -functions as thin wrappers so no behaviour changes and existing tests pass -untouched. Precondition for phases 2–4, which all compare version strings. -_Repos: editor + web (byte-identical) · ~half a day._ - -**Phase 2 — VPP install gate.** `package-manager-module.ts::install` checks -`minEditorVersion` against `APP_VERSION`, next to the signature verification; -same check when loading an already-installed package. `catalog-browser.tsx` -derives its state from the shared helper. Correct -`openplc-packages/docs/package-format.md:69` — it becomes true. -_Repos: editor + web · ~1 day._ - -**Phase 3 — `minRuntimeVersion` in the manifest.** Add the field to -`schema/manifest.schema.json`, conditionally required when any device targets -`runtime-v4`; enforce in `scripts/validate.ts` so a malformed package never -reaches a user; check it in the compile pipeline against the connected runtime. -_Repos: packages + editor + web · ~1 day._ - -**Phase 4 — `GET /api/capabilities`.** New unauthenticated endpoint returning -`runtimeVersion` + `minEditorVersion`, with the constant living beside -`RUNTIME_VERSION` in `webserver/version.py`. -_Repo: runtime · ~half a day._ - -**Phase 5 — editor consumes it.** `probe-runtime-version.ts` reads the endpoint, -treats `404` as "declares nothing", and blocks upload when -`APP_VERSION < minEditorVersion`. Rename `MIN_STRUCPP_RUNTIME_VERSION` to -`MIN_RUNTIME_VERSION` (keeping the old name as an alias) so the global reads as -what it is. -_Repos: editor + web · ~1 day._ - -**Phase 6 — tests.** Table of `(editorVersion, runtimeVersion, vppManifest) → -allow | block | warn` covering each of the four comparisons plus the -declares-nothing and unparseable rows. -_Repos: editor + web · ~half a day._ - -Total: **~4,5 days**. +## 8. Delivery — what landed where + +**All six phases are implemented.** There was deliberately no ordering +constraint between them: the runtime never blocks, so a phase shipping early +cannot break a peer. + +| # | Phase | Repos | Landed in | +| --- | ----------------------------------- | ----------------- | ---------------------------------------- | +| 1 | One semver parser | editor + web | openplc-editor#993 · openplc-web#652 | +| 2 | VPP install gate | editor + web | openplc-editor#993 · openplc-web#652 | +| 3 | `minRuntimeVersion` in the manifest | packages + editor | openplc-packages#28 · openplc-editor#993 | +| 4 | `GET /api/capabilities` | runtime | openplc-runtime#163 | +| 5 | Editor consumes the endpoint | editor + web | openplc-editor#993 · openplc-web#652 | +| 6 | Tests | editor + web | openplc-editor#993 · openplc-web#652 | + +**Phase 1 — one semver parser.** `frontend/utils/semver.ts` owns the parse and +the ordering; `parseRuntimeVersion` and `compareSemver` became thin wrappers, so +no call site changed and existing tests passed untouched. It lives in +`frontend/utils/` rather than `backend/shared/` because the layer rules allow +`backend-shared → utils` and not the reverse. + +**Phase 2 — VPP install gate.** `package-manager-module.ts::install` compares +`minEditorVersion` against `APP_VERSION`, next to the signature verification, so +one gate covers both the catalog and the "Add from file…" path. +`openplc-packages/docs/package-format.md:69` is now true. + +**Phase 3 — `minRuntimeVersion` in the manifest.** Field added to the schema, +conditionally required when any device targets `runtime-v4` and rejected +otherwise, enforced in `scripts/validate.ts`; compared in the compile pipeline +against the connected runtime. + +**Phase 4 — `GET /api/capabilities`.** Unauthenticated endpoint returning +`runtimeVersion` + `minEditorVersion`, constant beside `RUNTIME_VERSION` in +`webserver/version.py`. + +**Phase 5 — editor consumes it.** `probe-runtime-version.ts` prefers the +endpoint and falls back to `/api/version`. Note the correction to the original +plan: a runtime predating the endpoint answers **401**, not 404 — its +`restapi.py` ends in a `@restapi_bp.route("/")` catch-all behind +`@jwt_required()`, so an unknown path lands there. Verified against a real +pre-change container. The probe therefore keys off "can I read a version out of +this answer" rather than off a status code, and covers both shapes. +`MIN_STRUCPP_RUNTIME_VERSION` renamed to `MIN_RUNTIME_VERSION`, old name kept as +an alias. + +**Phase 6 — tests.** Automated coverage of all four comparisons plus the +declares-nothing and unparseable rows. Also verified manually end-to-end with +negative controls, including against a real Raspberry Pi reporting `v4.1.9`. ## 9. Explicitly out of scope @@ -336,15 +374,48 @@ Considered and dropped, so nobody re-derives them: releases that do not exist yet is unknowable — any value either blocks compatible future peers or does nothing. -## 10. Open questions - -1. **What value does the runtime publish as `minEditorVersion` today?** Needs a - concrete audit of when the current bundle layout stabilised. Publishing a - floor that is too high locks out working editors; too low makes the field - decorative. Safest start is the oldest editor known to work with the strucpp - pipeline. -2. **Does an installed-but-incompatible VPP get hidden or shown-as-unusable?** - Hiding is cleaner; showing explains why a board disappeared after an editor - downgrade. -3. **Warning surface for peers that declare nothing** (§6) — console only, or a - one-time notice in the UI? +## 10. Open questions and known gaps + +**Decided** + +1. ~~**What value does the runtime publish as `minEditorVersion`?**~~ → + **`4.1.0`**, set in `webserver/version.py`. That is where the STruC++ + pipeline landed, so it locks out nobody who works today; 4.0.x editors + emitted MatIEC artefacts the runtime cannot build at all. The rule for + raising it is stated on the constant itself: only when an older editor + genuinely produces a bundle this runtime would mis-compile. It is not a build + counter. +2. ~~**Warning surface for a runtime that declares nothing?**~~ → **silent**, for + the reason in §6: that is every deployed device. + +**Still open** + +3. **An unreadable floor is discarded with no signal.** `isVersionAtLeast` + returns `true` when it cannot parse the _minimum_, so + `minEditorVersion: "4.2"` — a plausible hand-written shorthand — disables the + gate entirely and says nothing. The asymmetry is the problem: the same string + is fatal as the _candidate_, with a user-visible message, and invisible as the + _floor_. The log channel is already threaded through the probe; this needs one + warning. No test can catch it today because the symptom is the absence of a + symptom. +4. **A malformed floor in a VPP manifest becomes "no floor".** + `package-manifest-schema.ts` accepts any non-empty string and the install gate + compares leniently, so `minEditorVersion: "garbage"` installs anywhere. + `openplc-packages`' `scripts/validate.ts` covers published packages, but the + install gate exists precisely because a sideloaded `.vpp` never passes through + that validator — for that entry path this schema is the only boundary. Cost of + requiring a strict `x.y.z` is nil: `"4.3"` and `"v5"` are already honoured, so + only total junk changes behaviour. +5. **An installed-but-incompatible VPP keeps loading after an editor downgrade** + (§4). Re-checking `minEditorVersion` on load would close it. Open sub-question + if we do: hide the package, or show it as unusable? Hiding is cleaner; showing + explains why a board disappeared. +6. **Three hand-rolled comparators remain** in `runtime-version-gate.ts` + (`isStrucppCompatibleRuntime`, `isUserManagementCapableRuntime`) doing their + own `if (v.major > 4) …` next to the constants they compare. Both are exactly + equivalent to `isVersionAtLeast(raw, )`, `null` handling included. + Not a bug — the same duplication this work set out to remove, one level up: + the parser got unified, the comparators did not. + +Items 3–6 were raised in review of openplc-editor#993. Items 3, 4 and 6 touch +shared-surface files, so any fix needs the mirrored commit in openplc-web. From db38d3db13a236da0f73192a16e97fa6643c2c71 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Thu, 6 Aug 2026 16:55:34 -0400 Subject: [PATCH 6/9] fix(compat): one version parser, one comparator, one board lookup (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. Every change here removes a second way of doing something that already had a first way. Version parsing — one parser, not two `parseVersionStrict` / `parseVersionLenient` kept the very split this work set out to remove: the same string meant different things depending on which one a caller reached for. `minEditorVersion: "4.3"` refused a package install and was silently ignored by the runtime gate. One `parseVersion` now applies one rule: a `v` prefix is decoration (`v4.3.2` === `4.3.2`), a missing component is zero (`4.3` === `4.3.0`, `4` === `4.0.0`), and anything else is UNKNOWN (null). Unknown never becomes 0.0.0 behind a caller's back, so a gate can still tell "I cannot read this" from "this is old" and fail closed on the first. `isCompatibleEditorVersion` delegates to `isVersionAtLeast`, so the install gate and the runtime gates cannot disagree; a table test asserts that directly. Consequence worth noting: the legacy `"v4"` header now parses as 4.0.0 instead of being rejected as junk. No gate's answer changes — 4.0.0 is below the 4.1.0 floor either way — but it is refused for the honest reason rather than because the string looked odd. An unreadable floor no longer disappears `PackageManifestSchema` rejects a `minEditorVersion` / `minRuntimeVersion` it cannot parse. `"4.3"`, `"4"`, `"v5"` all pass, so only genuine junk changes behaviour — and a sideloaded .vpp never reaches openplc-packages' validator, which makes this schema the only boundary for that path. When a *runtime* publishes an unreadable floor the probe now logs a warning: the upload still proceeds, but a constraint the runtime believes it is enforcing can no longer go missing in silence. Comparators — a constant, not a hand-rolled comparison `isStrucppCompatibleRuntime` and `isUserManagementCapableRuntime` each open-coded their own `if (v.major > 4) ... return v.minor >= 1` next to the constant they compare against. Both are now `isVersionAtLeast(raw, )`. The old bodies hardcoded the *shape* of the constant: raise MIN_RUNTIME_VERSION to a non-.0 patch and `minor >= 1` keeps answering for the old floor while every behavioural test still passes. Guard tests derive their expectations from the constants so a re-inlined comparison fails immediately. Board lookup — four copies became one `board-info-resolver`, `handleVendorPluginPackaging`, `buildVppArduinoModuleConfig` and `getRuntimeFloorForBoard` each grew their own "first installed package whose devices contain this name" loop. They agreed, which is the only reason this was latent: change the tie-break or start matching on id as well as name and three of the four keep the old behaviour, in a codebase where the symptom is a board compiling against the wrong package's HAL. `findVppDeviceByBoardName` in backend/shared is now the only implementation. It lives there because board-info-resolver is a caller and cannot reach into backend/editor, and it takes the PackageManagerPort the resolver already injects, so editor and web both satisfy it with no new plumbing. First-match-wins and skip-unreadable-manifest were implicit in all four copies and are now stated and tested. Message rendering All three describe* builders route an unreadable version through `formatVersionForDisplay`, so a blank renders as "unknown" instead of leaving a hole in the sentence ("...reports ."). The two DOPE-448 builders had no tests at all; runtime-version-gate.ts went from 66% to 100% function coverage. Also fixes the @deprecated tag on ParsedRuntimeVersion, which pointed at a module that does not exist. Verification: 5958 tests pass, 0 failures. 100% statements/branches/ functions/lines on all five touched shared files. validate:arch, tsc, eslint and prettier clean. compare-surfaces.py reports match=true across 1011 files against the mirrored openplc-web commit. Co-Authored-By: Claude Opus 5 (1M context) --- .../handle-vendor-plugin-packaging.test.ts | 23 ++- .../editor/compiler/compiler-module.ts | 45 +---- .../package-manager/package-manager-module.ts | 32 ++-- .../__tests__/runtime-version-gate.test.ts | 154 +++++++++++++++- .../shared/firmware/runtime-version-gate.ts | 64 ++++--- .../__tests__/find-vpp-device.test.ts | 75 ++++++++ .../shared/hardware/board-info-resolver.ts | 11 +- .../shared/hardware/find-vpp-device.ts | 58 ++++++ .../__tests__/probe-runtime-version.test.ts | 37 ++++ .../shared/library/probe-runtime-version.ts | 21 ++- src/frontend/utils/__tests__/semver.test.ts | 172 +++++++++++++----- src/frontend/utils/semver.ts | 145 ++++++++------- .../__tests__/package-manifest-schema.test.ts | 66 +++++++ .../shared/ports/package-manifest-schema.ts | 26 ++- 14 files changed, 719 insertions(+), 210 deletions(-) create mode 100644 src/backend/shared/hardware/__tests__/find-vpp-device.test.ts create mode 100644 src/backend/shared/hardware/find-vpp-device.ts create mode 100644 src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts diff --git a/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts b/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts index 209eb06f7..e44e95fc0 100644 --- a/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts +++ b/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts @@ -28,13 +28,28 @@ jest.mock('electron', () => ({ MessageChannelMain: class {}, })) +type FindVppDevice = typeof import('../../../shared/hardware/find-vpp-device') + const listInstalled = jest.fn() const getInstalledPackageManifest = jest.fn() jest.mock('../../package-manager', () => ({ - PackageManagerModule: jest.fn().mockImplementation(() => ({ - listInstalled, - getInstalledPackageManifest, - })), + PackageManagerModule: jest.fn().mockImplementation(() => { + const port = { listInstalled, getInstalledPackageManifest } + return { + ...port, + // Board lookup runs through the shared `findVppDeviceByBoardName`, and + // the mock runs the real one over these two stubs rather than + // re-implementing the search — a stub that resolved boards its own way + // would let the production lookup change without a test noticing. + // `require` (not a top-level import) because jest.mock factories are + // hoisted above the import block. + findDeviceByBoardName: (boardName: string) => + (jest.requireActual('../../../shared/hardware/find-vpp-device') as FindVppDevice).findVppDeviceByBoardName( + port, + boardName, + ), + } + }), })) // eslint-disable-next-line import/first diff --git a/src/backend/editor/compiler/compiler-module.ts b/src/backend/editor/compiler/compiler-module.ts index 329c5038e..8c2309eb3 100644 --- a/src/backend/editor/compiler/compiler-module.ts +++ b/src/backend/editor/compiler/compiler-module.ts @@ -120,7 +120,6 @@ import JSZip from 'jszip' import type { PlatformOption } from '../../../middleware/shared/ports/types' import { BoardInfoResolver } from '../../shared/hardware/board-info-resolver' -import type { PackageManifest } from '../package-manager' import { PackageManagerModule } from '../package-manager' import { CreateXMLFile } from '../utils' import { createDesktopLibraryBuildPort } from './desktop-library-build-port' @@ -2012,28 +2011,16 @@ class CompilerModule { handleOutputData: HandleOutputDataCallback, ): Promise { try { - const packageManager = new PackageManagerModule() - const installed = packageManager.listInstalled() - - let matchingPackagePath: string | null = null - let matchingDevice: PackageManifest['devices'][number] | null = null - - for (const pkg of installed) { - const manifest = packageManager.getInstalledPackageManifest(pkg.packageId) - if (!manifest) continue - const device = manifest.devices.find((d) => d.name === boardTarget) - if (device) { - matchingPackagePath = pkg.path - matchingDevice = device - break - } - } + const match = new PackageManagerModule().findDeviceByBoardName(boardTarget) - if (!matchingDevice || !matchingPackagePath) { + if (!match) { handleOutputData(`Board "${boardTarget}" is not from a VPP package, skipping VPP packaging`, 'info') return } + const matchingPackagePath = match.pkg.path + const matchingDevice = match.device + if (matchingDevice.target.type !== 'runtime-v4') { handleOutputData( `VPP board "${boardTarget}" is not runtime-v4 (target=${matchingDevice.target.type}), skipping VPP packaging`, @@ -2290,26 +2277,12 @@ class CompilerModule { vendorScreenData: Record, ): Promise> { try { - const packageManager = new PackageManagerModule() - const installed = packageManager.listInstalled() - - let matchingPackagePath: string | null = null - let matchingDevice: PackageManifest['devices'][number] | null = null - for (const pkg of installed) { - const manifest = packageManager.getInstalledPackageManifest(pkg.packageId) - if (!manifest) continue - const device = manifest.devices.find((d) => d.name === boardTarget) - if (device) { - matchingPackagePath = pkg.path - matchingDevice = device - break - } - } + const match = new PackageManagerModule().findDeviceByBoardName(boardTarget) - const rawModules = matchingDevice?.moduleSystem?.modules - if (!matchingDevice || !matchingPackagePath || !rawModules || rawModules.length === 0) return [] + const rawModules = match?.device.moduleSystem?.modules + if (!match || !rawModules || rawModules.length === 0) return [] - const pkgPath = matchingPackagePath + const pkgPath = match.pkg.path const modules = await Promise.all( rawModules.map(async (m) => { let configScreenDefinition: unknown diff --git a/src/backend/editor/package-manager/package-manager-module.ts b/src/backend/editor/package-manager/package-manager-module.ts index e588c826f..1b3bad1ba 100644 --- a/src/backend/editor/package-manager/package-manager-module.ts +++ b/src/backend/editor/package-manager/package-manager-module.ts @@ -6,6 +6,8 @@ import { join } from 'path' import { APP_VERSION } from '../../../frontend/data/constants/app-version' import { isCompatibleEditorVersion } from '../../../frontend/utils/semver' import { PackageManifestSchema } from '../../../middleware/shared/ports/package-manifest-schema' +import type { VppDeviceMatch } from '../../shared/hardware/find-vpp-device' +import { findVppDeviceByBoardName } from '../../shared/hardware/find-vpp-device' import { validatePathId } from '../../shared/utils/path-safety' import { TRUSTED_PACKAGE_KEYS } from '../../shared/utils/vpp/trusted-keys' import { verifyPackageSignature } from '../../shared/utils/vpp/verify-package-signature' @@ -302,16 +304,26 @@ class PackageManagerModule { return pkg?.path ?? null } + /** + * The installed VPP device named `boardName`, with its package and + * manifest — or null when no installed package provides it. + * + * `boardTarget` travels through the compile pipeline as a device + * *name*, so every consumer that needs the package behind a board + * starts here. Delegates to the shared `findVppDeviceByBoardName` so + * this and `board-info-resolver` (which cannot import this module) + * resolve a board the same way. + */ + findDeviceByBoardName(boardName: string): VppDeviceMatch | null { + return findVppDeviceByBoardName(this, boardName) + } + /** * `package.minRuntimeVersion` of the installed package that provides * `boardName`, or null when no installed package does, when the * matching device is not a `runtime-v4` target, or when the package * declares no floor (DOPE-448). * - * Board lookup is by device *name* because that is the identifier the - * compile pipeline carries as `boardTarget` — the same match - * `handleVendorPluginPackaging` performs. - * * Only runtime-v4 devices can carry a meaningful floor: their HAL is * plugin code built against the runtime's API. An `arduino-cli` * device never talks to the runtime, so a floor there would be a @@ -319,15 +331,9 @@ class PackageManagerModule { * it at authoring time, and this returns null if one slips through. */ getRuntimeFloorForBoard(boardName: string): string | null { - for (const pkg of this.listInstalled()) { - const manifest = this.getInstalledPackageManifest(pkg.packageId) - if (!manifest) continue - const device = manifest.devices.find((d) => d.name === boardName) - if (!device) continue - if (device.target.type !== 'runtime-v4') return null - return manifest.package.minRuntimeVersion ?? null - } - return null + const match = this.findDeviceByBoardName(boardName) + if (!match || match.device.target.type !== 'runtime-v4') return null + return match.manifest.package.minRuntimeVersion ?? null } private readRegistry(): PackageRegistry { diff --git a/src/backend/shared/firmware/__tests__/runtime-version-gate.test.ts b/src/backend/shared/firmware/__tests__/runtime-version-gate.test.ts index 3f8733187..f0148cc15 100644 --- a/src/backend/shared/firmware/__tests__/runtime-version-gate.test.ts +++ b/src/backend/shared/firmware/__tests__/runtime-version-gate.test.ts @@ -1,7 +1,11 @@ +import { isVersionAtLeast } from '../../../../frontend/utils/semver' import { + describeEditorTooOldForRuntime, describeIncompatibleRuntime, + describeVppRuntimeMismatch, isStrucppCompatibleRuntime, isUserManagementCapableRuntime, + MIN_RUNTIME_VERSION, MIN_STRUCPP_RUNTIME_VERSION, MIN_USER_MANAGEMENT_RUNTIME_VERSION, parseRuntimeVersion, @@ -59,13 +63,22 @@ describe('parseRuntimeVersion', () => { }) }) - it('rejects the legacy hardcoded "v4" string', () => { - expect(parseRuntimeVersion('v4')).toBeNull() + // A missing component is zero everywhere in this codebase, so the legacy + // header parses rather than being rejected as junk. The gate's answer is + // unchanged — see `isStrucppCompatibleRuntime` below — because 4.0.0 is + // genuinely below the 4.1.0 floor. The distinction matters: "I cannot read + // this" and "this is old" are different facts and only one of them is true. + it('reads the legacy hardcoded "v4" string as 4.0.0', () => { + expect(parseRuntimeVersion('v4')).toEqual({ major: 4, minor: 0, patch: 0, prerelease: undefined }) + }) + + it('fills a missing patch component with zero', () => { + expect(parseRuntimeVersion('v4.1')).toEqual({ major: 4, minor: 1, patch: 0, prerelease: undefined }) + expect(parseRuntimeVersion('v4.1')).toEqual(parseRuntimeVersion('4.1.0')) }) it('rejects ambiguous / non-numeric strings', () => { expect(parseRuntimeVersion('dev')).toBeNull() - expect(parseRuntimeVersion('v4.1')).toBeNull() expect(parseRuntimeVersion('v4.x.0')).toBeNull() expect(parseRuntimeVersion(' ')).toBeNull() }) @@ -102,10 +115,72 @@ describe('isStrucppCompatibleRuntime', () => { }) it('rejects the legacy "v4" header + any other unparseable string', () => { + // `v4` now parses (as 4.0.0) and is refused on its merits; the rest are + // unreadable and are refused because an unknown runtime never clears a + // floor. Both paths must stay closed. expect(isStrucppCompatibleRuntime('v4')).toBe(false) expect(isStrucppCompatibleRuntime('dev')).toBe(false) expect(isStrucppCompatibleRuntime(null)).toBe(false) expect(isStrucppCompatibleRuntime(undefined)).toBe(false) + expect(isStrucppCompatibleRuntime('')).toBe(false) + }) + + it('accepts a two-part version at or above the floor', () => { + expect(isStrucppCompatibleRuntime('v4.1')).toBe(true) + expect(isStrucppCompatibleRuntime('4.2')).toBe(true) + expect(isStrucppCompatibleRuntime('4.0')).toBe(false) + }) +}) + +// --------------------------------------------------------------------------- +// Every capability gate is "a constant + isVersionAtLeast", nothing more +// --------------------------------------------------------------------------- +// +// These read as tautologies today, and that is the point: they fail the moment +// someone re-inlines a comparison next to a constant. The version this replaced +// answered `v.minor >= 1` for the strucpp gate — correct only because the floor +// happened to end in `.0`. Raise the floor to `4.1.5` and that body keeps +// admitting 4.1.0 while every behavioural test still passes, because the tests +// were written against the old floor too. Deriving the expectation from the +// constant is the only assertion that survives a bump. +describe('capability gates track their constants', () => { + const CANDIDATES = [ + null, + undefined, + '', + 'dev', + 'garbage', + 'v4', + '4.0', + '4.1', + '3.9.9', + '4.0.9', + '4.1.0', + '4.1.0-rc.3', + '4.1.8', + '4.1.9', + '4.1.9-rc.1', + '4.1.10', + '4.2.0', + '4.10.0', + '5.0.0', + ] + + it.each(CANDIDATES)('isStrucppCompatibleRuntime(%p) === isVersionAtLeast(%p, MIN_RUNTIME_VERSION)', (raw) => { + expect(isStrucppCompatibleRuntime(raw)).toBe(isVersionAtLeast(raw, MIN_RUNTIME_VERSION)) + }) + + it.each(CANDIDATES)('isUserManagementCapableRuntime(%p) tracks its own constant', (raw) => { + expect(isUserManagementCapableRuntime(raw)).toBe(isVersionAtLeast(raw, MIN_USER_MANAGEMENT_RUNTIME_VERSION)) + }) + + it('puts each gate exactly on its own floor', () => { + expect(isStrucppCompatibleRuntime(MIN_RUNTIME_VERSION)).toBe(true) + expect(isUserManagementCapableRuntime(MIN_USER_MANAGEMENT_RUNTIME_VERSION)).toBe(true) + // The floors are ordered, so the lower gate must be open where the higher + // one is shut — a single `minor >= 1` body cannot express that difference. + expect(isStrucppCompatibleRuntime('4.1.0')).toBe(true) + expect(isUserManagementCapableRuntime('4.1.0')).toBe(false) }) }) @@ -123,3 +198,76 @@ describe('describeIncompatibleRuntime', () => { expect(describeIncompatibleRuntime(' ')).toContain('unknown') }) }) + +describe('describeEditorTooOldForRuntime', () => { + const args = { runtimeVersion: 'v4.3.0', minEditorVersion: '4.3.0', editorVersion: '4.2.10' } + + it('names both versions and the action that fixes it', () => { + const msg = describeEditorTooOldForRuntime(args) + expect(msg).toContain('v4.3.0') + expect(msg).toContain('4.3.0') + expect(msg).toContain('4.2.10') + expect(msg).toContain('Update the editor') + }) + + it('includes the device address when the platform knows one', () => { + expect(describeEditorTooOldForRuntime({ ...args, deviceLabel: '10.0.0.1' })).toContain('on 10.0.0.1') + }) + + it('omits the device clause when no label is available', () => { + // Web reaches the device through an orchestrator agent, so there is no + // address the user would recognise — better to say nothing than to print + // an agent id nobody can act on. + expect(describeEditorTooOldForRuntime(args)).not.toContain(' on ') + }) + + // The message must never contain a hole where a version should be. A blank + // runtime version is not "no version" — it is a version we failed to + // establish, and the user needs to be told which of the two we could not read. + const UNREADABLE: Array<[string | null | undefined, string]> = [ + ['', 'an empty string'], + [' ', 'whitespace only'], + [null, 'null'], + [undefined, 'undefined'], + ] + + it.each(UNREADABLE)('renders a %p runtime version as "unknown" (%s)', (runtimeVersion) => { + const msg = describeEditorTooOldForRuntime({ ...args, runtimeVersion }) + expect(msg).toContain('Runtime unknown requires') + }) +}) + +describe('describeVppRuntimeMismatch', () => { + const args = { boardTarget: 'SLM-RP4', minRuntimeVersion: '4.2.0', runtimeVersion: 'v4.1.7' } + + it('names the board, the floor, and the reported runtime', () => { + const msg = describeVppRuntimeMismatch(args) + expect(msg).toContain('"SLM-RP4"') + expect(msg).toContain('v4.2.0') + expect(msg).toContain('v4.1.7') + expect(msg).toContain('Upgrade the runtime') + }) + + it('names the device when the platform knows its address', () => { + expect(describeVppRuntimeMismatch({ ...args, deviceLabel: '10.0.0.1' })).toContain( + 'The runtime at 10.0.0.1 reports', + ) + }) + + it('falls back to "the connected runtime" without a label', () => { + expect(describeVppRuntimeMismatch(args)).toContain('The connected runtime reports') + }) + + const UNREADABLE: Array<[string | null | undefined, string]> = [ + ['', 'an empty string'], + [' ', 'whitespace only'], + [null, 'null'], + [undefined, 'undefined'], + ] + + it.each(UNREADABLE)('renders a %p runtime version as "unknown" (%s)', (runtimeVersion) => { + const msg = describeVppRuntimeMismatch({ ...args, runtimeVersion }) + expect(msg).toContain('reports unknown.') + expect(msg).not.toContain('reports .') + }) +}) diff --git a/src/backend/shared/firmware/runtime-version-gate.ts b/src/backend/shared/firmware/runtime-version-gate.ts index 5efedb694..8313493ad 100644 --- a/src/backend/shared/firmware/runtime-version-gate.ts +++ b/src/backend/shared/firmware/runtime-version-gate.ts @@ -14,8 +14,17 @@ * * The string is the GitHub release tag baked in at image build time * (see openplc-runtime/.github/workflows/docker.yml). Older - * runtimes that pre-date this work return a hardcoded "v4" string; - * that's intentionally unparseable here so the gate blocks them. + * runtimes that pre-date this work return a hardcoded "v4" string, + * which reads as 4.0.0 and is blocked on its merits — 4.0.0 is the + * MatIEC line. + * + * Every gate below is a MINIMUM VERSION and nothing else, so each one + * is a constant plus a call to `isVersionAtLeast`. No gate open-codes + * its own comparison: a hand-rolled `v.minor >= 1` hardcodes the shape + * of the constant beside it, and the two then drift the moment someone + * raises the constant to a version whose patch is not zero — the gate + * keeps answering for the old floor and every test still passes. + * Adding a capability gate means adding a constant, not a comparator. * * Shared by both openplc-editor (which uploads to remote runtimes) * and openplc-web (which gates push-to-device through the @@ -27,7 +36,7 @@ // `backend-shared -> utils` is allowed, and using `@root/` here would have // skipped the check rather than passed it. import type { ParsedVersion } from '../../../frontend/utils/semver' -import { parseVersionStrict } from '../../../frontend/utils/semver' +import { formatVersionForDisplay, isVersionAtLeast, parseVersion } from '../../../frontend/utils/semver' /** * Oldest runtime this editor will upload to — the editor's own @@ -44,39 +53,42 @@ export const MIN_RUNTIME_VERSION = '4.1.0' /** @deprecated Use `MIN_RUNTIME_VERSION`. */ export const MIN_STRUCPP_RUNTIME_VERSION = MIN_RUNTIME_VERSION -/** @deprecated Use `ParsedVersion` from `shared/utils/version-compare`. */ +/** @deprecated Use `ParsedVersion` from `frontend/utils/semver`. */ export type ParsedRuntimeVersion = ParsedVersion /** - * Parses a runtime version string. Returns null when the string - * doesn't carry enough information to compare — e.g. the legacy `"v4"` - * or `"dev"` builds. Callers treat null as "incompatible". + * Parses a runtime version string. Returns null when the string is not + * a version at all — `"dev"`, `"garbage"`, `""`. Callers treat null as + * "incompatible", so a runtime that cannot say what it is never clears + * a floor. + * + * Note what is NOT null: the legacy `"v4"` header parses as `4.0.0` and + * `"4.1"` as `4.1.0`, because a missing component is zero everywhere in + * this codebase. Neither changes any gate's answer — `4.0.0` is still + * below `MIN_RUNTIME_VERSION`, so the legacy header is still refused, + * now for the honest reason that 4.0.0 predates STruC++ rather than + * because the string looked odd. * - * Delegates to the shared strict parser so the VPP surface and the - * runtime gates can never drift apart on what `"v4"` or `"4.1"` means. + * Delegates to the shared parser so the VPP surface and the runtime + * gates can never drift apart on what a given string means. */ export function parseRuntimeVersion(raw: string | null | undefined): ParsedRuntimeVersion | null { - return parseVersionStrict(raw) + return parseVersion(raw) } /** * Returns true iff the runtime version string represents a runtime - * that speaks the STruC++ wire format (i.e. ≥ 4.1.0, including - * pre-release tags like `v4.1.0-rc.3`). + * that speaks the STruC++ wire format (i.e. ≥ `MIN_RUNTIME_VERSION`, + * including pre-release tags like `v4.1.0-rc.3`). * * Note: by strict semver, `4.1.0-rc.3 < 4.1.0`. We deliberately * deviate here because the rc tags on the v4.1.0 line ARE the * builds shipping STruC++ — there is no "older 4.1.0" the rc lineage - * would be a pre-release of. + * would be a pre-release of. `isVersionAtLeast` ignores pre-release + * in ordering for exactly this reason. */ export function isStrucppCompatibleRuntime(raw: string | null | undefined): boolean { - const v = parseRuntimeVersion(raw) - if (!v) return false - if (v.major > 4) return true - if (v.major < 4) return false - // major === 4: minor must be ≥ 1 (i.e. v4.1.x is the strucpp line). - // patch + prerelease don't matter past that. - return v.minor >= 1 + return isVersionAtLeast(raw, MIN_RUNTIME_VERSION) } /** Minimum runtime version that ships the user-management API @@ -92,11 +104,7 @@ export const MIN_USER_MANAGEMENT_RUNTIME_VERSION = '4.1.9' * gate's treatment of the rc lineage. */ export function isUserManagementCapableRuntime(raw: string | null | undefined): boolean { - const v = parseRuntimeVersion(raw) - if (!v) return false - if (v.major !== 4) return v.major > 4 - if (v.minor !== 1) return v.minor > 1 - return v.patch >= 9 + return isVersionAtLeast(raw, MIN_USER_MANAGEMENT_RUNTIME_VERSION) } /** @@ -105,7 +113,7 @@ export function isUserManagementCapableRuntime(raw: string | null | undefined): * "unknown") is included so the user can match it to the device. */ export function describeIncompatibleRuntime(raw: string | null | undefined): string { - const reported = raw && raw.trim().length > 0 ? raw.trim() : 'unknown' + const reported = formatVersionForDisplay(raw) return ( `Runtime version ${reported} is not compatible with this editor. ` + `Upload requires OpenPLC Runtime v${MIN_RUNTIME_VERSION} or newer (STruC++ pipeline). ` + @@ -126,7 +134,7 @@ export function describeEditorTooOldForRuntime(args: { editorVersion: string deviceLabel?: string }): string { - const runtime = args.runtimeVersion?.trim() ?? 'unknown' + const runtime = formatVersionForDisplay(args.runtimeVersion) const where = args.deviceLabel ? ` on ${args.deviceLabel}` : '' return ( `Runtime ${runtime}${where} requires OpenPLC Editor ${args.minEditorVersion} or newer. ` + @@ -148,7 +156,7 @@ export function describeVppRuntimeMismatch(args: { runtimeVersion: string | null | undefined deviceLabel?: string }): string { - const runtime = args.runtimeVersion?.trim() ?? 'unknown' + const runtime = formatVersionForDisplay(args.runtimeVersion) const where = args.deviceLabel ? `The runtime at ${args.deviceLabel} reports` : 'The connected runtime reports' return ( `Board "${args.boardTarget}" requires OpenPLC Runtime v${args.minRuntimeVersion} or newer. ` + diff --git a/src/backend/shared/hardware/__tests__/find-vpp-device.test.ts b/src/backend/shared/hardware/__tests__/find-vpp-device.test.ts new file mode 100644 index 000000000..b82307924 --- /dev/null +++ b/src/backend/shared/hardware/__tests__/find-vpp-device.test.ts @@ -0,0 +1,75 @@ +import type { InstalledPackage, PackageManifest } from '../../../../middleware/shared/ports/types' +import type { PackageManagerPort } from '../board-info-resolver' +import { findVppDeviceByBoardName } from '../find-vpp-device' + +const pkg = (packageId: string): InstalledPackage => + ({ packageId, path: `/packages/${packageId}` }) as unknown as InstalledPackage + +const manifest = (packageId: string, deviceNames: string[]): PackageManifest => + ({ + package: { id: packageId, name: packageId, version: '1.0.0' }, + devices: deviceNames.map((name) => ({ id: name.toLowerCase(), name, target: { type: 'runtime-v4' } })), + }) as unknown as PackageManifest + +/** A package source backed by plain in-memory maps. */ +const source = ( + installed: InstalledPackage[], + manifests: Record, +): PackageManagerPort => ({ + listInstalled: () => installed, + getInstalledPackageManifest: (packageId) => manifests[packageId] ?? null, +}) + +describe('findVppDeviceByBoardName', () => { + it('returns the package, manifest and device for a board a VPP provides', () => { + const port = source([pkg('vendor.a')], { 'vendor.a': manifest('vendor.a', ['SLM-RP4', 'P2-722']) }) + const match = findVppDeviceByBoardName(port, 'P2-722') + expect(match?.pkg.packageId).toBe('vendor.a') + expect(match?.manifest.package.id).toBe('vendor.a') + expect(match?.device.name).toBe('P2-722') + }) + + it('searches every installed package, not just the first', () => { + const port = source([pkg('vendor.a'), pkg('vendor.b')], { + 'vendor.a': manifest('vendor.a', ['SLM-RP4']), + 'vendor.b': manifest('vendor.b', ['P2-722']), + }) + expect(findVppDeviceByBoardName(port, 'P2-722')?.pkg.packageId).toBe('vendor.b') + }) + + it('returns null for a board no installed package provides', () => { + const port = source([pkg('vendor.a')], { 'vendor.a': manifest('vendor.a', ['SLM-RP4']) }) + // The ordinary case: a built-in hals.json board with no VPP behind it. + expect(findVppDeviceByBoardName(port, 'Arduino Uno')).toBeNull() + }) + + it('returns null when nothing is installed', () => { + expect(findVppDeviceByBoardName(source([], {}), 'SLM-RP4')).toBeNull() + }) + + // The behaviour every copy of this loop shared and none of them stated. + // Pinned here so a future change to it is a deliberate, single edit rather + // than three sites drifting apart. + it('takes the first match in listInstalled order when two packages collide', () => { + const port = source([pkg('vendor.a'), pkg('vendor.b')], { + 'vendor.a': manifest('vendor.a', ['SLM-RP4']), + 'vendor.b': manifest('vendor.b', ['SLM-RP4']), + }) + expect(findVppDeviceByBoardName(port, 'SLM-RP4')?.pkg.packageId).toBe('vendor.a') + }) + + it('skips a package whose manifest cannot be read and keeps searching', () => { + // A single corrupt install must not hide a board another package provides. + const port = source([pkg('vendor.broken'), pkg('vendor.b')], { + 'vendor.broken': null, + 'vendor.b': manifest('vendor.b', ['SLM-RP4']), + }) + expect(findVppDeviceByBoardName(port, 'SLM-RP4')?.pkg.packageId).toBe('vendor.b') + }) + + it('matches on device name exactly', () => { + const port = source([pkg('vendor.a')], { 'vendor.a': manifest('vendor.a', ['SLM-RP4']) }) + expect(findVppDeviceByBoardName(port, 'slm-rp4')).toBeNull() + expect(findVppDeviceByBoardName(port, 'SLM-RP4 ')).toBeNull() + }) +}) diff --git a/src/backend/shared/hardware/board-info-resolver.ts b/src/backend/shared/hardware/board-info-resolver.ts index c241b2f9d..148fcf61f 100644 --- a/src/backend/shared/hardware/board-info-resolver.ts +++ b/src/backend/shared/hardware/board-info-resolver.ts @@ -26,6 +26,7 @@ import type { DebugSpec } from '../../../middleware/shared/ports/debug-spec-types' import type { InstalledPackage, PackageManifest, PlatformOption } from '../../../middleware/shared/ports/types' import type { TargetCapabilities } from '../../../middleware/shared/utils/target-capabilities/types' +import { findVppDeviceByBoardName } from './find-vpp-device' // --------------------------------------------------------------------------- // Public shapes @@ -257,14 +258,8 @@ export class BoardInfoResolver { #tryVppLookup( boardName: string, ): Omit | null { - for (const pkg of this.config.packageManager.listInstalled()) { - const manifest = this.config.packageManager.getInstalledPackageManifest(pkg.packageId) - if (!manifest) continue - const device = manifest.devices.find((d) => d.name === boardName) - if (!device) continue - return this.#fromVppDevice(device, pkg, manifest) - } - return null + const match = findVppDeviceByBoardName(this.config.packageManager, boardName) + return match ? this.#fromVppDevice(match.device, match.pkg, match.manifest) : null } #fromVppDevice( diff --git a/src/backend/shared/hardware/find-vpp-device.ts b/src/backend/shared/hardware/find-vpp-device.ts new file mode 100644 index 000000000..28cfe07a1 --- /dev/null +++ b/src/backend/shared/hardware/find-vpp-device.ts @@ -0,0 +1,58 @@ +/** + * The one way to answer "which installed VPP provides this board?". + * + * `boardTarget` travels through the compile pipeline as a plain device + * *name* — the string the user picked in the board dropdown. Four call + * sites needed to turn it back into the package and manifest entry it + * came from (board build info, VPP plugin packaging, module config + * screens, the runtime-version floor), and each had grown its own copy + * of the same loop. + * + * They agreed, which is the only reason this was a latent bug and not + * an open one: four copies of "first package whose `devices` contains a + * matching name wins" that nothing forced to stay in agreement. Change + * the tie-break, add a namespacing rule, start matching on `id` as well + * as `name` — and three of the four would keep the old behaviour, in a + * codebase where the symptom is a board that compiles against the wrong + * package's HAL. + * + * Lives in `backend/shared` because `board-info-resolver` (also shared) + * is one of the callers and cannot reach into `backend/editor`. It takes + * the same narrow `PackageManagerPort` the resolver already injects, so + * editor and web both satisfy it without new plumbing. + */ + +import type { InstalledPackage, PackageManifest } from '../../../middleware/shared/ports/types' +import type { PackageManagerPort } from './board-info-resolver' + +/** An installed VPP device, with the package and manifest it came from. */ +export interface VppDeviceMatch { + /** Registry entry — carries `packageId` and the on-disk `path`. */ + pkg: InstalledPackage + /** The full manifest, for package-level fields (`minRuntimeVersion`, …). */ + manifest: PackageManifest + /** The matched `devices[]` entry. */ + device: PackageManifest['devices'][number] +} + +/** + * Find the installed VPP device whose name is `boardName`, or null when + * no installed package provides it (the ordinary case for a built-in + * hals.json board). + * + * **First match wins**, in `listInstalled()` order. Two packages + * shipping a device of the same name is an authoring collision, not a + * situation with a right answer; resolving it consistently everywhere + * matters more than which one is picked. Packages whose manifest cannot + * be read are skipped rather than treated as empty, so a single corrupt + * install cannot hide a board another package provides. + */ +export function findVppDeviceByBoardName(packageManager: PackageManagerPort, boardName: string): VppDeviceMatch | null { + for (const pkg of packageManager.listInstalled()) { + const manifest = packageManager.getInstalledPackageManifest(pkg.packageId) + if (!manifest) continue + const device = manifest.devices.find((d) => d.name === boardName) + if (device) return { pkg, manifest, device } + } + return null +} diff --git a/src/backend/shared/library/__tests__/probe-runtime-version.test.ts b/src/backend/shared/library/__tests__/probe-runtime-version.test.ts index 27030b35a..c5d8b9c88 100644 --- a/src/backend/shared/library/__tests__/probe-runtime-version.test.ts +++ b/src/backend/shared/library/__tests__/probe-runtime-version.test.ts @@ -133,6 +133,43 @@ describe('probeRuntimeVersion — capabilities endpoint', () => { expect(log).not.toHaveBeenCalled() }) + // The shorthands a runtime is likely to publish by hand all parse, and are + // enforced as their zero-filled equivalent — no warning, because nothing is + // being dropped. + it.each([['4.2'], ['4'], ['v5'], ['4.2.1-rc.1']])( + 'passes a %p floor through without complaint', + async (minEditorVersion) => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ success: true, body: { runtimeVersion: 'v4.2.0', minEditorVersion } }), + fetchVersion: versionMustNotBeCalled(), + log, + }) + expect(result).toEqual({ version: 'v4.2.0', minEditorVersion }) + expect(log).not.toHaveBeenCalled() + }, + ) + + // A floor that is present but unreadable declares nothing, which is the safe + // answer for the upload and the wrong one for whoever wrote it: the runtime + // believes it is enforcing a constraint that is not being applied. The + // upload still proceeds — refusing to talk to a device over a typo in its + // metadata would be worse — but it can no longer happen in silence. + it('warns when the runtime declares a floor nobody can read, and still returns it', async () => { + const log = jest.fn() + const result = await probeRuntimeVersion({ + fetchCapabilities: async () => ({ + success: true, + body: { runtimeVersion: 'v4.2.0', minEditorVersion: 'garbage' }, + }), + fetchVersion: versionMustNotBeCalled(), + log, + }) + expect(result).toEqual({ version: 'v4.2.0', minEditorVersion: 'garbage' }) + expect(log).toHaveBeenCalledWith(expect.stringContaining('unreadable minEditorVersion ("garbage")'), 'warning') + expect(log).toHaveBeenCalledWith(expect.stringContaining('not being enforced'), 'warning') + }) + it('reports minEditorVersion=null when the endpoint answers without that field', async () => { const log = jest.fn() const result = await probeRuntimeVersion({ diff --git a/src/backend/shared/library/probe-runtime-version.ts b/src/backend/shared/library/probe-runtime-version.ts index 1190265c0..5a2f46209 100644 --- a/src/backend/shared/library/probe-runtime-version.ts +++ b/src/backend/shared/library/probe-runtime-version.ts @@ -27,6 +27,11 @@ * Pure: no I/O. Caller supplies the transport via `fetchVersion`. */ +// Relative import on purpose: `npm run validate:arch` only inspects relative +// specifiers, so this path is actually checked against the layer rules — +// `backend-shared -> utils` is allowed. +import { isValidVersion } from '../../../frontend/utils/semver' + /** Outcome of the transport-level fetch. Adapters return this from * their HTTPS / orchestrator round-trip; the shared helper takes * it from here. */ @@ -146,7 +151,21 @@ async function tryFetchCapabilities(opts: ProbeRuntimeVersionOptions): Promise

{ +describe('parseVersion', () => { it('parses a plain three-part version', () => { - expect(parseVersionStrict('4.1.9')).toEqual({ major: 4, minor: 1, patch: 9, prerelease: undefined }) + expect(parseVersion('4.1.9')).toEqual({ major: 4, minor: 1, patch: 9, prerelease: undefined }) }) - it('accepts the tag-style v prefix the runtime reports', () => { - expect(parseVersionStrict('v4.2.0')).toEqual({ major: 4, minor: 2, patch: 0, prerelease: undefined }) + // A `v` prefix is decoration, not meaning: the runtime reports `v4.2.0`, git + // tags carry `v`, and hand-written manifests use both. They must compare the + // same or the same release is two different versions depending on who typed it. + it('treats a v prefix as identical to no prefix', () => { + expect(parseVersion('v4.3.2')).toEqual(parseVersion('4.3.2')) + expect(compareSemver('v4.3.2', '4.3.2')).toBe(0) + expect(isVersionAtLeast('v4.3.2', '4.3.2')).toBe(true) + expect(isVersionAtLeast('4.3.2', 'v4.3.2')).toBe(true) + }) + + // A floor written as `"4.3"` means 4.3.0 and is enforced as 4.3.0. Anything + // else and the same shorthand is honoured by one gate and ignored by another. + it('fills missing components with zero', () => { + expect(parseVersion('4.3')).toEqual(parseVersion('4.3.0')) + expect(parseVersion('4')).toEqual(parseVersion('4.0.0')) + expect(parseVersion('v4')).toEqual(parseVersion('4.0.0')) + expect(parseVersion('4.3')).toEqual({ major: 4, minor: 3, patch: 0, prerelease: undefined }) + expect(parseVersion('4')).toEqual({ major: 4, minor: 0, patch: 0, prerelease: undefined }) }) it('captures pre-release and build suffixes without failing', () => { - expect(parseVersionStrict('4.1.0-rc.3')?.prerelease).toBe('rc.3') - expect(parseVersionStrict('4.1.0+build.5')?.prerelease).toBe('build.5') + expect(parseVersion('4.1.0-rc.3')?.prerelease).toBe('rc.3') + expect(parseVersion('4.1.0+build.5')?.prerelease).toBe('build.5') + expect(parseVersion('4.1-rc.3')).toEqual({ major: 4, minor: 1, patch: 0, prerelease: 'rc.3' }) }) it('tolerates surrounding whitespace', () => { - expect(parseVersionStrict(' 4.1.9 ')).toEqual({ major: 4, minor: 1, patch: 9, prerelease: undefined }) + expect(parseVersion(' 4.1.9 ')).toEqual({ major: 4, minor: 1, patch: 9, prerelease: undefined }) }) - // The whole point of the strict parser: these are the values a runtime in - // the field actually reports when it cannot identify itself, and every one - // of them must stay unparseable so a gate fails closed instead of guessing. + // Unknown is not a version. It must not silently become 0.0.0 inside the + // parser, because a caller that cannot tell "unknown" from "0.0.0" cannot + // fail closed on the first and pass on the second. it.each([ - ['v4', 'the legacy hardcoded header'], - ['4.1', 'a two-part version'], ['dev', 'a source build with no CI tag'], ['garbage', 'anything else'], ['', 'an empty string'], + [' ', 'whitespace only'], + ['4.1 beta', 'trailing garbage after a valid prefix'], + ['abc.def.ghi', 'non-numeric components'], + ['4,3,0', 'the wrong separator'], + ['.1.2', 'a missing major'], ])('returns null for %p (%s)', (input) => { - expect(parseVersionStrict(input)).toBeNull() + expect(parseVersion(input)).toBeNull() }) it('returns null for null and undefined', () => { - expect(parseVersionStrict(null)).toBeNull() - expect(parseVersionStrict(undefined)).toBeNull() + expect(parseVersion(null)).toBeNull() + expect(parseVersion(undefined)).toBeNull() }) }) -describe('parseVersionLenient', () => { - it('parses a plain three-part version', () => { - expect(parseVersionLenient('4.1.9')).toEqual({ major: 4, minor: 1, patch: 9 }) +describe('isValidVersion', () => { + it('accepts every shorthand the parser accepts', () => { + for (const raw of ['4.3.2', 'v4.3.2', '4.3', '4', 'v5', '4.1.0-rc.1', '4.1.0+build.5']) { + expect(isValidVersion(raw)).toBe(true) + } }) - it('fills missing components with zero', () => { - expect(parseVersionLenient('4.1')).toEqual({ major: 4, minor: 1, patch: 0 }) - expect(parseVersionLenient('4')).toEqual({ major: 4, minor: 0, patch: 0 }) + it('rejects what the parser cannot read', () => { + for (const raw of ['garbage', 'next', '', ' ', '4,3,0', null, undefined]) { + expect(isValidVersion(raw)).toBe(false) + } }) +}) - it('strips the v prefix and any suffix before parsing', () => { - expect(parseVersionLenient('v4.1.9')).toEqual({ major: 4, minor: 1, patch: 9 }) - expect(parseVersionLenient('4.1.9-rc.1')).toEqual({ major: 4, minor: 1, patch: 9 }) - expect(parseVersionLenient('4.1.9+build.5')).toEqual({ major: 4, minor: 1, patch: 9 }) +describe('formatVersionForDisplay', () => { + it('trims a readable version', () => { + expect(formatVersionForDisplay(' v4.2.0 ')).toBe('v4.2.0') }) - // Degrading to the lowest possible version means a corrupt manifest field - // loses every comparison rather than winning one. - it.each([ - ['garbage', 'a non-numeric string'], - ['abc.def.ghi', 'non-numeric components'], + // Every "incompatible versions" message must render an unestablished version + // the same way. A blank leaves a hole in the sentence and tells the user + // nothing about which of the two versions could not be read. + const UNREADABLE: Array<[string | null | undefined, string]> = [ ['', 'an empty string'], - ])('degrades %p to 0.0.0 (%s)', (input) => { - expect(parseVersionLenient(input)).toEqual({ major: 0, minor: 0, patch: 0 }) - }) + [' ', 'whitespace only'], + [null, 'null'], + [undefined, 'undefined'], + ] - it('degrades null and undefined to 0.0.0', () => { - expect(parseVersionLenient(null)).toEqual({ major: 0, minor: 0, patch: 0 }) - expect(parseVersionLenient(undefined)).toEqual({ major: 0, minor: 0, patch: 0 }) + it.each(UNREADABLE)('renders %p as "unknown" (%s)', (raw) => { + expect(formatVersionForDisplay(raw)).toBe('unknown') }) }) @@ -113,6 +136,11 @@ describe('isVersionAtLeast', () => { expect(isVersionAtLeast('4.2.10', '4.2.1')).toBe(true) }) + it('compares numerically, not lexicographically', () => { + expect(isVersionAtLeast('4.10.0', '4.9.0')).toBe(true) + expect(isVersionAtLeast('4.9.0', '4.10.0')).toBe(false) + }) + it('passes when the candidate sits exactly on the floor', () => { expect(isVersionAtLeast('4.2.1', '4.2.1')).toBe(true) }) @@ -125,6 +153,18 @@ describe('isVersionAtLeast', () => { expect(isVersionAtLeast('v4.1.9-rc.1', '4.1.9')).toBe(true) }) + // The shorthand case. `"4.3"` as a floor must block a 4.2.10 editor exactly + // as `"4.3.0"` would — this is the asymmetry that let a runtime publish a + // floor nobody enforced. + it('enforces a partial floor exactly as its zero-filled equivalent', () => { + expect(isVersionAtLeast('4.2.10', '4.3')).toBe(false) + expect(isVersionAtLeast('4.2.10', '4.3.0')).toBe(false) + expect(isVersionAtLeast('4.3.0', '4.3')).toBe(true) + expect(isVersionAtLeast('4.2.10', '5')).toBe(false) + expect(isVersionAtLeast('5.0.0', '5')).toBe(true) + expect(isVersionAtLeast('4.2.10', 'v4.3')).toBe(false) + }) + // A peer that asks for nothing gets nothing enforced — this is what keeps // runtimes predating /api/capabilities working unchanged. const NOTHING_DECLARED: Array<[string | null | undefined, string]> = [ @@ -137,16 +177,20 @@ describe('isVersionAtLeast', () => { expect(isVersionAtLeast('4.2.0', floor)).toBe(true) }) - it('passes when the floor itself is unparseable, since it declares nothing', () => { + // An unreadable floor is worth 0.0.0 and everything clears 0.0.0. It is not + // silent, though: the manifest schema refuses it outright and the runtime + // probe logs a warning, so nobody believes a constraint is applying when it + // is not. + it('passes when the floor itself is unreadable, since it declares nothing', () => { expect(isVersionAtLeast('4.2.0', 'garbage')).toBe(true) - expect(isVersionAtLeast('4.2.0', 'v4')).toBe(true) + expect(isVersionAtLeast('4.2.0', 'next')).toBe(true) }) // Fails closed: an unidentifiable peer never clears a real floor. const UNIDENTIFIABLE: Array<[string | null | undefined, string]> = [ - ['v4', 'the legacy header'], ['dev', 'a source build'], ['garbage', 'a corrupt value'], + ['', 'a blank answer'], [null, 'an unreachable peer'], [undefined, 'a missing value'], ] @@ -154,6 +198,15 @@ describe('isVersionAtLeast', () => { it.each(UNIDENTIFIABLE)('fails when the candidate is %p (%s) and a real floor exists', (candidate) => { expect(isVersionAtLeast(candidate, '4.1.0')).toBe(false) }) + + // The legacy hardcoded header now parses (as 4.0.0) instead of being + // rejected as junk, and still loses — for the honest reason that 4.0.0 + // predates the floor rather than because the string looked odd. + it('reads the legacy "v4" header as 4.0.0, which still fails a 4.1.0 floor', () => { + expect(parseVersion('v4')).toEqual({ major: 4, minor: 0, patch: 0, prerelease: undefined }) + expect(isVersionAtLeast('v4', '4.1.0')).toBe(false) + expect(isVersionAtLeast('v4', '4.0.0')).toBe(true) + }) }) describe('compareSemver', () => { @@ -181,18 +234,28 @@ describe('compareSemver', () => { }) it('strips pre-release suffix before comparing', () => { - // The function intentionally ignores pre-release ordering; both compare - // as the same `4.1.1` triple. If we ever ship pre-releases for real this - // contract needs revisiting, but ignoring is the safer default today. expect(compareSemver('4.1.1-rc.1', '4.1.1')).toBe(0) expect(compareSemver('4.1.1+build.5', '4.1.1-rc.1')).toBe(0) }) - it('treats malformed inputs as 0.0.0 (defensive against corrupt manifests)', () => { + // A v-prefixed catalog version used to parse as 0.0.0 and sort below every + // plain-numbered release, so "update available" was wrong for it. + it('ranks a v-prefixed version by its number, not below everything', () => { + expect(compareSemver('v2.0.0', '1.0.0')).toBe(1) + expect(compareSemver('1.0.0', 'v2.0.0')).toBe(-1) + expect(compareSemver('v1.2.3', '1.2.3')).toBe(0) + }) + + // Sorting needs a total order, so this is the one place unknown becomes + // 0.0.0 — a corrupt `version` in somebody else's manifest sorts to the + // bottom instead of breaking the catalog. Nothing is gated on the result. + it('sorts an unreadable version as the lowest possible one', () => { expect(compareSemver('not-a-version', '0.0.0')).toBe(0) expect(compareSemver('', '0.0.0')).toBe(0) - expect(compareSemver('1.2', '1.2.0')).toBe(0) // missing patch defaults to 0 - expect(compareSemver('abc.def.ghi', '0.0.1')).toBe(-1) // bogus < 0.0.1 + expect(compareSemver('1.2', '1.2.0')).toBe(0) + expect(compareSemver('abc.def.ghi', '0.0.1')).toBe(-1) + expect(compareSemver('0.0.1', 'abc.def.ghi')).toBe(1) + expect(compareSemver('garbage', 'nonsense')).toBe(0) }) }) @@ -215,4 +278,19 @@ describe('isCompatibleEditorVersion', () => { expect(isCompatibleEditorVersion('5.0.0', '4.1.1')).toBe(false) expect(isCompatibleEditorVersion('4.2.0', '4.1.1')).toBe(false) }) + + // The install gate and the runtime gates must answer identically for every + // string, or the same floor is enforced in one place and ignored in the + // other. This is that contract, asserted directly. + it.each([ + ['4.3', '4.2.10'], + ['4', '4.2.10'], + ['v5', '4.2.10'], + ['4.2.10', '4.2.10'], + ['garbage', '4.2.10'], + ['', '4.2.10'], + ['99.0.0', '4.2.10'], + ])('agrees with isVersionAtLeast for floor %p against editor %p', (floor, current) => { + expect(isCompatibleEditorVersion(floor, current)).toBe(isVersionAtLeast(current, floor)) + }) }) diff --git a/src/frontend/utils/semver.ts b/src/frontend/utils/semver.ts index 39a14f2c6..846f09481 100644 --- a/src/frontend/utils/semver.ts +++ b/src/frontend/utils/semver.ts @@ -12,25 +12,23 @@ * * This file used to answer only #3, with `firmware/runtime-version-gate.ts` * carrying its own parser for the runtime side. The two disagreed on exactly - * the inputs that show up in the field: + * the inputs that show up in the field — `"v4"` and `"4.1"` parsed in one and + * were rejected by the other — and a first pass at unifying them kept that + * split alive as a lenient parser and a strict parser chosen by name. * - * input | catalog parser | runtime parser - * -------------|------------------|---------------- - * "v4" | 4.0.0 | rejected - * "4.1" | 4.1.0 | rejected - * "garbage" | 0.0.0 (lowest) | rejected + * That was still one parser too many. The same string has to mean the same + * thing everywhere, or a floor is enforced in one place and ignored in + * another: `minEditorVersion: "4.3"` refused an install while the identical + * value from a runtime sailed through unnoticed. So there is now exactly + * ONE parser, and it applies one rule: * - * Neither behaviour was wrong for its own caller. A package manifest carrying - * a corrupt version should not crash the catalog UI, and an unidentifiable - * runtime must not receive an upload. What was wrong is that the DIFFERENCE - * lived in two separate parsers, where nothing named it and nothing tested it - * side by side. + * - a `v` prefix is decoration: `v4.3.2` === `4.3.2` + * - a missing component is zero: `4.3` === `4.3.0`, `4` === `4.0.0` + * - anything else is UNKNOWN: `"dev"`, `"garbage"`, `""` → null * - * So: one parse, one comparison, and the lenient-vs-strict choice made - * explicitly by name at the call site. `parseVersionStrict` returns null for - * anything it cannot fully identify — callers that must fail closed use it. - * `parseVersionLenient` fills missing components with 0 and degrades garbage to - * 0.0.0 — callers rendering untrusted metadata use it. + * "Unknown" is never a version. It does not become 0.0.0 behind the caller's + * back and it never satisfies a declared floor — a peer that cannot say what + * it is does not get to claim it is new enough. * * Pre-release and build suffixes (`-rc.1`, `+build.5`) are parsed but do NOT * affect ordering: `4.1.0-rc.3` compares equal to `4.1.0`. This is deliberate @@ -52,48 +50,55 @@ export interface ParsedVersion { prerelease?: string } -/** `v4.1.0-rc.3` / `4.1.0` — all three numeric components required. */ -const STRICT_RE = /^v?(\d+)\.(\d+)\.(\d+)(?:[-+](.+))?$/ +/** + * `4`, `4.3`, `4.3.2`, `v4.3.2`, `4.3.2-rc.1`, `4.3.2+build.5`. + * + * Anchored at both ends on purpose: a trailing-garbage input like `"4.1 beta"` + * must fail rather than silently parse as `4.1.0`, because a value nobody can + * read is a mistake worth surfacing, not a version worth guessing. + */ +const VERSION_RE = /^v?(\d+)(?:\.(\d+))?(?:\.(\d+))?(?:[-+](.+))?$/ + +/** Lowest possible version — what an unknown string is worth in a total order. */ +const ZERO: ParsedVersion = { major: 0, minor: 0, patch: 0 } /** - * Parse a version string, requiring all three numeric components. + * Parse a version string, or return null when the string is not a version. * - * Returns null for anything else — `"v4"`, `"4.1"`, `"dev"`, `""`, null. Use - * this when an unidentifiable version must block an action: the caller cannot - * accidentally treat "I don't know" as "old enough" or "new enough", because - * there is no number to compare. + * Missing trailing components are zero, so a floor written as `"4.3"` means + * exactly what `"4.3.0"` means and is enforced identically. A leading `v` is + * stripped. Everything else — `"dev"`, `"garbage"`, `""`, `"4.1 beta"`, null — + * is UNKNOWN, and callers decide what unknown costs them. */ -export function parseVersionStrict(raw: string | null | undefined): ParsedVersion | null { +export function parseVersion(raw: string | null | undefined): ParsedVersion | null { if (!raw) return null - const match = raw.trim().match(STRICT_RE) + const match = raw.trim().match(VERSION_RE) if (!match) return null return { major: Number.parseInt(match[1], 10), - minor: Number.parseInt(match[2], 10), - patch: Number.parseInt(match[3], 10), + minor: match[2] === undefined ? 0 : Number.parseInt(match[2], 10), + patch: match[3] === undefined ? 0 : Number.parseInt(match[3], 10), prerelease: match[4], } } +/** True when `raw` is a version this codebase can compare. */ +export function isValidVersion(raw: string | null | undefined): boolean { + return parseVersion(raw) !== null +} + /** - * Parse a version string, filling in whatever is missing with zero. + * A version string as it should appear in a message to the user: trimmed, or + * the word `unknown` when there is nothing readable to show. * - * `"4.1"` becomes 4.1.0; `"garbage"` and `""` become 0.0.0 — the lowest - * possible version, so a corrupt value loses every comparison instead of - * winning one. Use this for untrusted metadata being rendered rather than - * enforced, where a malformed field should degrade the display and not throw. + * Exists so that every "incompatible versions" message renders an unreadable + * peer the same way. Printing an empty string leaves a hole in the sentence + * ("Runtime on 10.0.0.1 requires…") and tells the user nothing about which + * of the two versions the editor failed to establish. */ -export function parseVersionLenient(raw: string | null | undefined): ParsedVersion { - // Deliberately unanchored at the end: it consumes as many leading numeric - // components as it finds and ignores whatever follows, so `4.1.9-rc.1` and - // `4.1` both parse without a separate suffix-stripping pass. - const match = (raw ?? '').trim().match(/^v?(\d+)(?:\.(\d+))?(?:\.(\d+))?/) - if (!match) return { major: 0, minor: 0, patch: 0 } - const toInt = (value: string | undefined): number => { - const parsed = Number.parseInt(value ?? '', 10) - return Number.isFinite(parsed) ? parsed : 0 - } - return { major: toInt(match[1]), minor: toInt(match[2]), patch: toInt(match[3]) } +export function formatVersionForDisplay(raw: string | null | undefined): string { + const trimmed = raw?.trim() ?? '' + return trimmed.length > 0 ? trimmed : 'unknown' } /** @@ -108,44 +113,48 @@ export function compareParsedVersions(a: ParsedVersion, b: ParsedVersion): -1 | } /** - * `candidate >= minimum`, where an unparseable `candidate` fails closed. + * `candidate >= minimum` — the one comparison every DOPE-448 gate asks. + * + * Three inputs and what each costs: * - * This is the shape every DOPE-448 gate wants: "may I proceed?" answered - * `false` when the peer cannot be identified. An absent `minimum` means no - * constraint was declared, which is a pass — a peer that asks for nothing gets - * nothing enforced, which is what keeps runtimes predating - * `/api/capabilities` working unchanged. + * - `minimum` absent or empty → **pass**. A peer that declares no floor + * constrains nothing; this is every runtime predating `/api/capabilities` + * and it is what makes shipping the gates safe. + * - `minimum` present but unreadable → **pass**, because an unknown floor is + * worth 0.0.0 and everything clears 0.0.0. Callers that can see the string + * should say so out loud rather than let it vanish — the manifest schema + * rejects such a value outright, and the runtime probe logs a warning. + * - `candidate` unreadable against a real floor → **fail**. An unknown + * version never satisfies a declared minimum. */ export function isVersionAtLeast(candidate: string | null | undefined, minimum: string | null | undefined): boolean { - if (!minimum) return true - const min = parseVersionStrict(minimum) - if (!min) return true // a floor we cannot read declares nothing - const version = parseVersionStrict(candidate) - if (!version) return false // an unidentifiable peer never clears a real floor + const min = parseVersion(minimum) + if (!min) return true + const version = parseVersion(candidate) + if (!version) return false return compareParsedVersions(version, min) >= 0 } -// --------------------------------------------------------------------------- -// Lenient VPP-surface helpers -// --------------------------------------------------------------------------- - /** - * Lenient comparison, used by the VPP catalog and the package install gate. + * Order two version strings for display purposes — sorting catalog rows, + * deciding whether an available version is newer than the installed one. * - * Lenient is right *here* specifically: a package manifest is untrusted - * third-party metadata, and a corrupt `version` string should sort as the - * lowest possible version rather than break a card in the catalog UI. Gates - * deciding whether to talk to a runtime use `isVersionAtLeast` instead. + * This is the ONE place an unknown version is coerced to 0.0.0, because a + * sortable list needs a total order and a corrupt `version` field in somebody + * else's manifest should sort to the bottom rather than break the UI. Nothing + * is gated on the result. Every gate uses `isVersionAtLeast`. */ export function compareSemver(a: string, b: string): -1 | 0 | 1 { - return compareParsedVersions(parseVersionLenient(a), parseVersionLenient(b)) + return compareParsedVersions(parseVersion(a) ?? ZERO, parseVersion(b) ?? ZERO) } /** - * True when `current` satisfies `minRequired`. An absent or empty minimum - * means the package declared no floor, which is a pass. + * True when `current` satisfies the `minRequired` a package declares. + * + * Delegates to `isVersionAtLeast` so the install gate and the runtime gates + * cannot disagree about what a given string means — the bug this consolidation + * exists to prevent. */ export function isCompatibleEditorVersion(minRequired: string | undefined, current: string): boolean { - if (!minRequired) return true - return compareSemver(current, minRequired) >= 0 + return isVersionAtLeast(current, minRequired) } diff --git a/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts b/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts new file mode 100644 index 000000000..b9fa4458d --- /dev/null +++ b/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts @@ -0,0 +1,66 @@ +import { PackageManifestSchema } from '../package-manifest-schema' + +/** A minimal manifest the schema accepts, with room to override `package`. */ +const manifest = (pkg: Record = {}) => ({ + formatVersion: '1.0', + package: { id: 'vendor.board', name: 'Vendor Board', version: '1.0.0', ...pkg }, + devices: [{ id: 'slm-rp4', name: 'SLM-RP4' }], +}) + +describe('PackageManifestSchema — compatibility floors', () => { + it('accepts a manifest that declares no floors at all', () => { + // Packages built before DOPE-448 must keep installing. + expect(PackageManifestSchema.safeParse(manifest()).success).toBe(true) + }) + + // These are the shorthands a human writes by hand. Each means exactly its + // zero-filled equivalent and is enforced as such, so accepting them here is + // not leniency — it is the same rule the comparator applies. + it.each([ + ['4.3.2', 'a full triple'], + ['v4.3.2', 'a tag-style v prefix'], + ['4.3', 'a two-part shorthand'], + ['4', 'a bare major'], + ['v5', 'a v-prefixed bare major'], + ['4.3.2-rc.1', 'a pre-release'], + ['4.3.2+build.5', 'a build suffix'], + ])('accepts minEditorVersion %p (%s)', (minEditorVersion) => { + expect(PackageManifestSchema.safeParse(manifest({ minEditorVersion })).success).toBe(true) + }) + + // The reason this check exists: an unreadable floor is not inert. The + // comparator treats it as "declares nothing", so the package installs + // everywhere while its author believes a constraint is being enforced. + // A sideloaded `.vpp` never passes through openplc-packages' validator, so + // for that entry path this schema is the only boundary there is. + it.each([ + ['garbage', 'a non-version word'], + ['next', 'a channel name mistaken for a version'], + ['4,3,0', 'the wrong separator'], + ['4.3 or newer', 'prose'], + [' ', 'whitespace only'], + ['', 'an empty string'], + ])('rejects minEditorVersion %p (%s)', (minEditorVersion) => { + expect(PackageManifestSchema.safeParse(manifest({ minEditorVersion })).success).toBe(false) + }) + + it('applies the same rule to minRuntimeVersion', () => { + expect(PackageManifestSchema.safeParse(manifest({ minRuntimeVersion: '4.2' })).success).toBe(true) + expect(PackageManifestSchema.safeParse(manifest({ minRuntimeVersion: 'garbage' })).success).toBe(false) + }) + + it('names the offending field so the install error is actionable', () => { + const result = PackageManifestSchema.safeParse(manifest({ minEditorVersion: 'garbage' })) + expect(result.success).toBe(false) + if (result.success) return + expect(result.error.issues[0].path).toEqual(['package', 'minEditorVersion']) + expect(result.error.issues[0].message).toContain('must be a version') + }) + + it('still lets unknown fields through untouched', () => { + // The editor stays agnostic to manifest contents; the floors are the + // deliberate exception, not a new general policy. + const result = PackageManifestSchema.safeParse(manifest({ vendorExtension: { anything: true } })) + expect(result.success).toBe(true) + }) +}) diff --git a/src/middleware/shared/ports/package-manifest-schema.ts b/src/middleware/shared/ports/package-manifest-schema.ts index ef23107b9..ba9cd4d86 100644 --- a/src/middleware/shared/ports/package-manifest-schema.ts +++ b/src/middleware/shared/ports/package-manifest-schema.ts @@ -34,8 +34,28 @@ import { z } from 'zod' +import { isValidVersion } from '../../../frontend/utils/semver' import type { PackageManifest } from './types' +/** + * A compatibility floor must be a version this codebase can compare. + * + * This is the exception to the "the editor is agnostic to manifest + * contents" rule above, and it earns it: an unreadable floor is not + * inert, it is a constraint the package author believes is being + * enforced and which silently is not. `"4.3"`, `"4"` and `"v5"` are all + * accepted — they mean 4.3.0 / 4.0.0 / 5.0.0 — so in practice only + * genuine junk (`"garbage"`, `"next"`, `"4,3,0"`) is refused. + * + * It matters most for the path this gate exists for: a `.vpp` added + * from disk never passes through openplc-packages' `scripts/validate.ts`, + * so for sideloaded packages this schema is the only boundary there is. + */ +const versionFloor = z + .string() + .min(1) + .refine(isValidVersion, { message: 'must be a version like "4.3.2", "4.3", "4" or "v4.3.2"' }) + export const PackageManifestSchema = z .object({ formatVersion: z.string().min(1), @@ -53,8 +73,10 @@ export const PackageManifestSchema = z // Authoring-side rules (minRuntimeVersion required iff a device targets // runtime-v4, rejected otherwise) live in openplc-packages' // `scripts/validate.ts`, per this file's split of responsibilities. - minEditorVersion: z.string().min(1).optional(), - minRuntimeVersion: z.string().min(1).optional(), + // The *format* is checked here because a floor nobody can parse is a + // constraint that silently does not apply — see `versionFloor`. + minEditorVersion: versionFloor.optional(), + minRuntimeVersion: versionFloor.optional(), }) .passthrough(), devices: z.array(z.object({}).passthrough()).min(1), From 4629e64581ffc1ae84d73efa9f8ecca4f4499a64 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Thu, 6 Aug 2026 17:44:29 -0400 Subject: [PATCH 7/9] docs(compat): mark review items 3, 4 and 6 resolved (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §10 items 3 (unreadable floor discarded silently), 4 (manifest accepts a malformed floor) and 6 (hand-rolled comparators) were fixed in the same PR; strike them through with what actually landed rather than leaving a "still open" list that is no longer true. Item 6 said "three" and named two — there were two. Also records the sharper reason for 6 that only surfaced while fixing it: the hand-rolled bodies hardcoded the shape of the constant beside them, so raising a floor to a non-.0 patch would have left them answering for the old one with every test still green. §6's edge-case table gains the two rows the fix created — a partial floor (`"4.3"`) is enforced as `4.3.0` rather than being ignored, and an unreadable floor now warns — and drops the claim that no warning exists. §8 phase 1 records that the first cut shipped two parsers and why one was too many, plus the `"v4"` parsing change. Splits out item 7: `minRuntimeVersion` is still unenforced at install time for sideloaded packages. Items 5 and 7 remain open and need tickets. Co-Authored-By: Claude Opus 5 (1M context) --- docs/version-compatibility-strategy.md | 113 ++++++++++++++++--------- 1 file changed, 75 insertions(+), 38 deletions(-) diff --git a/docs/version-compatibility-strategy.md b/docs/version-compatibility-strategy.md index 29fb91ad1..a70e9bb0d 100644 --- a/docs/version-compatibility-strategy.md +++ b/docs/version-compatibility-strategy.md @@ -279,22 +279,29 @@ supported state rather than an error. | Peer state | Behaviour as implemented | | -------------------------------------------------------- | ---------------------------------------------------------------------------------------------- | | Runtime without `/api/capabilities` | no editor floor to check; `MIN_RUNTIME_VERSION` gate still applies; **silent** — see below | -| Runtime version unparseable (`"v4"`, `"dev"`) | fail closed — previous behaviour, unchanged | -| Runtime declares an _unreadable_ floor (`"4.2"`, junk) | floor ignored, upload proceeds, **no signal** — known gap, see §10 | +| Runtime version unreadable (`"dev"`, junk) | fail closed — previous behaviour, unchanged | +| Runtime declares a partial floor (`"4.3"`, `"4"`) | enforced as `4.3.0` / `4.0.0` — a missing component is zero everywhere | +| Runtime declares an _unreadable_ floor (junk) | floor ignored, upload proceeds, **warning logged** to the compile console | | VPP without `minRuntimeVersion` on a `runtime-v4` target | install allowed, runtime match unverifiable, no warning; blocked at package build time instead | +| VPP with an _unreadable_ floor | manifest rejected at install — the schema refuses a floor it cannot parse | The first row is silent **on purpose**: a runtime with no `/api/capabilities` is every device currently deployed, so warning there would fire on every upload from -every editor. The third row is different — a floor that is present but malformed +every editor. The fourth row is different — a floor that is present but malformed is a mistake someone made, and swallowing it means a constraint the runtime -believes it is enforcing silently is not. That one deserves a warning and does -not have one yet. +believes it is enforcing silently is not. That one now warns. + +Note the third row: `"4.3"` is **not** an unreadable floor. A missing component +is zero throughout the codebase, so a hand-written shorthand is enforced exactly +as its zero-filled equivalent. Only genuine junk reaches the fourth row, which is +why refusing it in a manifest costs nothing. Because the runtime never enforces, no closing window is needed on the upload -path: an old runtime simply contributes no constraint. The only future tightening -worth scheduling is making `minRuntimeVersion` mandatory for `runtime-v4` -packages, which is enforced at **package build time** (`scripts/validate.ts`) and -so never breaks an installed package. +path: an old runtime simply contributes no constraint. Making `minRuntimeVersion` +mandatory for `runtime-v4` packages already ships, enforced at **package build +time** (`scripts/validate.ts`), so it never breaks an installed package. The +remaining gap is install-time enforcement for sideloaded packages, which never +pass through that validator — see §10. ## 7. Accepted limitation @@ -325,10 +332,25 @@ cannot break a peer. | 6 | Tests | editor + web | openplc-editor#993 · openplc-web#652 | **Phase 1 — one semver parser.** `frontend/utils/semver.ts` owns the parse and -the ordering; `parseRuntimeVersion` and `compareSemver` became thin wrappers, so -no call site changed and existing tests passed untouched. It lives in -`frontend/utils/` rather than `backend/shared/` because the layer rules allow -`backend-shared → utils` and not the reverse. +the ordering; `parseRuntimeVersion` and `compareSemver` are thin wrappers over +it. It lives in `frontend/utils/` rather than `backend/shared/` because the layer +rules allow `backend-shared → utils` and not the reverse. + +The first cut of this kept _two_ parsers — a strict one and a lenient one, chosen +by name at the call site — which review showed was still one too many: the same +string meant different things depending on which a caller reached for, and that +is the defect the phase existed to remove (see §10 items 3 and 6). There is now +exactly one `parseVersion`, applying one rule: a `v` prefix is decoration, a +missing component is zero, anything else is UNKNOWN (`null`). Unknown never +becomes `0.0.0` behind a caller's back, so a gate can still distinguish "I cannot +read this" from "this is old" and fail closed on the first. The one place a total +order is genuinely required — sorting catalog rows — coerces unknown to `0.0.0` +explicitly, inside `compareSemver`, where nothing is gated on the result. + +Consequence worth recording: the legacy `"v4"` header now parses as `4.0.0` +instead of being rejected as unreadable. No gate's answer changes — `4.0.0` is +below `MIN_RUNTIME_VERSION` either way — but it is refused because it is old, +not because the string looked odd. **Phase 2 — VPP install gate.** `package-manager-module.ts::install` compares `minEditorVersion` against `APP_VERSION`, next to the signature verification, so @@ -388,34 +410,49 @@ Considered and dropped, so nobody re-derives them: 2. ~~**Warning surface for a runtime that declares nothing?**~~ → **silent**, for the reason in §6: that is every deployed device. +3. ~~**An unreadable floor is discarded with no signal.**~~ → **fixed** + (openplc-editor#993 · openplc-web#652). Two halves. The one that mattered in + practice was that `"4.2"` was not "unreadable" in one gate and was in another: + the install gate honoured it as 4.2.0 while `isVersionAtLeast` dropped it. A + missing component is now zero everywhere, so a partial floor is enforced + exactly as its zero-filled equivalent, and `isCompatibleEditorVersion` + delegates to `isVersionAtLeast` so the two gates cannot disagree at all. What + remains genuinely unreadable is junk, and `probe-runtime-version.ts` logs a + warning when a runtime publishes it. The upload still proceeds — refusing a + device over a typo in its metadata would be worse — but a constraint the + runtime believes it is enforcing can no longer go missing in silence. +4. ~~**A malformed floor in a VPP manifest becomes "no floor".**~~ → **fixed** + (openplc-editor#993 · openplc-web#652). `package-manifest-schema.ts` refuses a + `minEditorVersion` / `minRuntimeVersion` it cannot parse, naming the field in + the error. As predicted, the cost is nil: `"4.3"`, `"4"`, `"v5"` and + pre-release suffixes all pass, so only genuine junk changes behaviour. This is + the only boundary a sideloaded `.vpp` crosses, which is the entry path the + install gate exists for. +5. ~~**Hand-rolled comparators remain**~~ → **fixed** (openplc-editor#993 · + openplc-web#652). `isStrucppCompatibleRuntime` and + `isUserManagementCapableRuntime` are now `isVersionAtLeast(raw, )`. + (The item said "three" and named two — there were two.) The equivalence was + verified exhaustively before the swap, but the reason to do it turned out to + be sharper than duplication: the hand-rolled bodies hardcoded the _shape_ of + the constant beside them. `return v.minor >= 1` is correct only because the + floor ends in `.0`; raise `MIN_RUNTIME_VERSION` to `4.1.5` and it keeps + admitting 4.1.0 while every behavioural test still passes, because those tests + were written against the old floor too. Guard tests now derive their + expectations from the constants, so a re-inlined comparison fails immediately. + **Still open** -3. **An unreadable floor is discarded with no signal.** `isVersionAtLeast` - returns `true` when it cannot parse the _minimum_, so - `minEditorVersion: "4.2"` — a plausible hand-written shorthand — disables the - gate entirely and says nothing. The asymmetry is the problem: the same string - is fatal as the _candidate_, with a user-visible message, and invisible as the - _floor_. The log channel is already threaded through the probe; this needs one - warning. No test can catch it today because the symptom is the absence of a - symptom. -4. **A malformed floor in a VPP manifest becomes "no floor".** - `package-manifest-schema.ts` accepts any non-empty string and the install gate - compares leniently, so `minEditorVersion: "garbage"` installs anywhere. - `openplc-packages`' `scripts/validate.ts` covers published packages, but the - install gate exists precisely because a sideloaded `.vpp` never passes through - that validator — for that entry path this schema is the only boundary. Cost of - requiring a strict `x.y.z` is nil: `"4.3"` and `"v5"` are already honoured, so - only total junk changes behaviour. 5. **An installed-but-incompatible VPP keeps loading after an editor downgrade** (§4). Re-checking `minEditorVersion` on load would close it. Open sub-question if we do: hide the package, or show it as unusable? Hiding is cleaner; showing explains why a board disappeared. -6. **Three hand-rolled comparators remain** in `runtime-version-gate.ts` - (`isStrucppCompatibleRuntime`, `isUserManagementCapableRuntime`) doing their - own `if (v.major > 4) …` next to the constants they compare. Both are exactly - equivalent to `isVersionAtLeast(raw, )`, `null` handling included. - Not a bug — the same duplication this work set out to remove, one level up: - the parser got unified, the comparators did not. - -Items 3–6 were raised in review of openplc-editor#993. Items 3, 4 and 6 touch -shared-surface files, so any fix needs the mirrored commit in openplc-web. +6. **`minRuntimeVersion` is not enforced at install time for sideloaded + packages.** `scripts/validate.ts` requires it for `runtime-v4` packages at + authoring time, and the compile pipeline compares it against the connected + runtime — but a `.vpp` added from disk declaring no floor installs and only + fails later, at compile. Lower priority than 5: the compile-time gate catches + the mismatch before anything reaches a device. + +Items 3–6 were raised in review of openplc-editor#993; 3, 4 and 6 were fixed in +the same PR (mirrored in openplc-web#652, both on shared-surface files). Item 7 +was split out of CodeRabbit's read of §6. Items 5 and 7 need their own tickets. From 32d5636e01ec0a4ee38f63c19d551090fa4cc420 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Thu, 6 Aug 2026 17:48:01 -0400 Subject: [PATCH 8/9] fix(compat): reject version components too large to hold exactly (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Number.parseInt` answers a *finite* number for a 17-digit component and quietly rounds it: `"9007199254740993"` comes back as `…992`. The parser would then hand callers a version that no longer matches the string it came from, and `isVersionAtLeast` would compare against it. That breaks the only promise this parser makes — readable means exact, anything else is UNKNOWN. A component we can only approximate is not readable, so `parseVersion` now returns null for it, and such a version gets the same treatment as any other unreadable string: fails closed as a candidate, declares nothing as a floor. Unreachable with any real version string; taken because the rule is cheaper to state without an exception than with one. Raised by CodeRabbit on openplc-editor#993. Co-Authored-By: Claude Opus 5 (1M context) --- src/frontend/utils/__tests__/semver.test.ts | 20 ++++++++++++++ src/frontend/utils/semver.ts | 30 ++++++++++++++++----- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/frontend/utils/__tests__/semver.test.ts b/src/frontend/utils/__tests__/semver.test.ts index ff0dc5b33..ff301864d 100644 --- a/src/frontend/utils/__tests__/semver.test.ts +++ b/src/frontend/utils/__tests__/semver.test.ts @@ -63,6 +63,26 @@ describe('parseVersion', () => { expect(parseVersion(null)).toBeNull() expect(parseVersion(undefined)).toBeNull() }) + + // `Number.parseInt` answers a finite number here and quietly rounds it — + // 9007199254740993 comes back as …992. A component we can only approximate is + // not readable, and returning a number that no longer matches the string it + // came from would break the one promise the parser makes. + it('rejects a component too large to hold exactly', () => { + expect(parseVersion('9007199254740993.0.0')).toBeNull() + expect(parseVersion('4.9007199254740993.0')).toBeNull() + expect(parseVersion('4.1.9007199254740993')).toBeNull() + // The largest value that IS exact still parses. + expect(parseVersion('9007199254740991.0.0')?.major).toBe(9007199254740991) + }) + + it('leaves an oversized version unable to clear any floor', () => { + // Fails closed as a candidate, declares nothing as a floor — the same + // treatment every other unreadable string gets. + expect(isVersionAtLeast('9007199254740993.0.0', '4.1.0')).toBe(false) + expect(isVersionAtLeast('4.1.0', '9007199254740993.0.0')).toBe(true) + expect(isValidVersion('9007199254740993.0.0')).toBe(false) + }) }) describe('isValidVersion', () => { diff --git a/src/frontend/utils/semver.ts b/src/frontend/utils/semver.ts index 846f09481..62770e383 100644 --- a/src/frontend/utils/semver.ts +++ b/src/frontend/utils/semver.ts @@ -26,6 +26,10 @@ * - a missing component is zero: `4.3` === `4.3.0`, `4` === `4.0.0` * - anything else is UNKNOWN: `"dev"`, `"garbage"`, `""` → null * + * "Readable" means *exactly* readable: a component too large to hold without + * rounding is UNKNOWN too, rather than a number that no longer matches the + * string it came from. + * * "Unknown" is never a version. It does not become 0.0.0 behind the caller's * back and it never satisfies a declared floor — a peer that cannot say what * it is does not get to claim it is new enough. @@ -62,6 +66,21 @@ const VERSION_RE = /^v?(\d+)(?:\.(\d+))?(?:\.(\d+))?(?:[-+](.+))?$/ /** Lowest possible version — what an unknown string is worth in a total order. */ const ZERO: ParsedVersion = { major: 0, minor: 0, patch: 0 } +/** + * A single numeric component, or null when it is not one we can hold exactly. + * + * `Number.parseInt` answers a *finite* number for a 17-digit component and + * quietly rounds it: `"9007199254740993"` comes back as `…992`. That breaks the + * only promise this parser makes — readable means exact, and anything else is + * UNKNOWN. A component we can only approximate is not readable, so it is null + * rather than a number that no longer matches what was written. + */ +function parseComponent(raw: string | undefined): number | null { + if (raw === undefined) return 0 + const value = Number.parseInt(raw, 10) + return Number.isSafeInteger(value) ? value : null +} + /** * Parse a version string, or return null when the string is not a version. * @@ -74,12 +93,11 @@ export function parseVersion(raw: string | null | undefined): ParsedVersion | nu if (!raw) return null const match = raw.trim().match(VERSION_RE) if (!match) return null - return { - major: Number.parseInt(match[1], 10), - minor: match[2] === undefined ? 0 : Number.parseInt(match[2], 10), - patch: match[3] === undefined ? 0 : Number.parseInt(match[3], 10), - prerelease: match[4], - } + const major = parseComponent(match[1]) + const minor = parseComponent(match[2]) + const patch = parseComponent(match[3]) + if (major === null || minor === null || patch === null) return null + return { major, minor, patch, prerelease: match[4] } } /** True when `raw` is a version this codebase can compare. */ From 8a7ae518814c0e397c8956aabaf2ffa45fc7424e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Fri, 7 Aug 2026 17:58:00 +0200 Subject: [PATCH 9/9] fix(compat): tolerate an unreadable version floor on the load path (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The floor-format rule added to `PackageManifestSchema` guards the boundary where a package ENTERS the editor. The same schema is also used to read the `manifest.json` of an already-installed package, and there the rule had a second-order effect nobody wants: a package installed by an older editor, carrying a floor only genuine junk could produce, would stop parsing on load. `getInstalledPackageManifest` returns null, the boards it provides fall out of the board lookup, and they do so silently — on an upgrade where the user did nothing. Split the two: `parseInstalledPackageManifest` drops a floor this codebase cannot compare and logs it; `importFromFile` still refuses one outright. Dropping costs nothing that was not already lost, since an unreadable floor never gated anything (`isVersionAtLeast` treats it as "declares nothing"), and the log keeps the cause visible instead of trading one invisible outcome for another. Tolerance is scoped to the floors — a document that is not a manifest still rejects. Applied at both read sites: the main process (`getInstalledPackageManifest`) and the renderer adapter that re-validates what comes back over IPC. Raised in re-review of #993; recorded in §10 item 4 of the strategy doc. Mirrored byte-identically in openplc-web. Co-Authored-By: Claude Opus 5 --- docs/version-compatibility-strategy.md | 17 +++- .../get-installed-package-manifest.test.ts | 88 +++++++++++++++++++ .../package-manager/package-manager-module.ts | 14 ++- src/frontend/utils/semver.ts | 3 +- .../editor/__tests__/package-adapter.test.ts | 18 ++++ .../adapters/editor/package-adapter.ts | 9 +- .../__tests__/package-manifest-schema.test.ts | 76 +++++++++++++++- .../shared/ports/package-manifest-schema.ts | 65 ++++++++++++++ 8 files changed, 282 insertions(+), 8 deletions(-) create mode 100644 src/backend/editor/package-manager/__tests__/get-installed-package-manifest.test.ts diff --git a/docs/version-compatibility-strategy.md b/docs/version-compatibility-strategy.md index a70e9bb0d..99304a838 100644 --- a/docs/version-compatibility-strategy.md +++ b/docs/version-compatibility-strategy.md @@ -355,7 +355,9 @@ not because the string looked odd. **Phase 2 — VPP install gate.** `package-manager-module.ts::install` compares `minEditorVersion` against `APP_VERSION`, next to the signature verification, so one gate covers both the catalog and the "Add from file…" path. -`openplc-packages/docs/package-format.md:69` is now true. +`openplc-packages/docs/package-format.md:69` is now true. Reading an installed +package's manifest back off disk goes through `parseInstalledPackageManifest` +instead, which tolerates a floor it cannot compare — see §10 item 4. **Phase 3 — `minRuntimeVersion` in the manifest.** Field added to the schema, conditionally required when any device targets `runtime-v4` and rejected @@ -428,6 +430,19 @@ Considered and dropped, so nobody re-derives them: pre-release suffixes all pass, so only genuine junk changes behaviour. This is the only boundary a sideloaded `.vpp` crosses, which is the entry path the install gate exists for. + + Second-order effect, raised in re-review and fixed with it: the same schema is + also used to read the `manifest.json` of an **already-installed** package, so + the new format rule would have made a package installed before this change — + carrying a floor only genuine junk could produce — stop resolving on load, and + its boards disappear from the board lookup with no message, on an upgrade + where the user did nothing. The rule is therefore strict where the artefact + **enters** and tolerant where we are only **reading** what is already on disk: + `parseInstalledPackageManifest` drops an uncomparable floor and logs it, the + install path still refuses it. Dropping costs nothing that was not already + lost — an unreadable floor never gated anything — and refusing stays where + refusing belongs. + 5. ~~**Hand-rolled comparators remain**~~ → **fixed** (openplc-editor#993 · openplc-web#652). `isStrucppCompatibleRuntime` and `isUserManagementCapableRuntime` are now `isVersionAtLeast(raw, )`. diff --git a/src/backend/editor/package-manager/__tests__/get-installed-package-manifest.test.ts b/src/backend/editor/package-manager/__tests__/get-installed-package-manifest.test.ts new file mode 100644 index 000000000..0cf94653a --- /dev/null +++ b/src/backend/editor/package-manager/__tests__/get-installed-package-manifest.test.ts @@ -0,0 +1,88 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { app } from 'electron' + +// Same trimming as the signature suite: this module pulls in winston transports +// and extract-zip's ESM entry point that this suite has no use for. +jest.mock('electron', () => ({ app: { getPath: jest.fn(() => '/mock/path') } })) +jest.mock('extract-zip', () => ({ __esModule: true, default: jest.fn() })) +jest.mock('../../services/logger-service', () => ({ + logger: { warn: jest.fn(), info: jest.fn(), error: jest.fn() }, +})) + +import { PackageManagerModule } from '../package-manager-module' + +/** + * `getInstalledPackageManifest` is the LOAD path, and every board provided by a + * VPP resolves through it (`find-vpp-device` → `board-info-resolver` → the + * compile pipeline). The install gate is allowed to refuse; this is not, beyond + * a document that is not a manifest at all — a rejection here is a board + * disappearing from the lookup with nothing said to the user. + */ +describe('PackageManagerModule.getInstalledPackageManifest — reading an installed package', () => { + let userDataDir: string + let packagesDir: string + let warnSpy: jest.SpyInstance + + const manifestJson = (pkg: Record) => + JSON.stringify({ + formatVersion: '1.0', + package: { id: 'vendor.board', name: 'Vendor Board', version: '1.0.0', ...pkg }, + devices: [{ id: 'slm-rp4', name: 'SLM-RP4' }], + }) + + /** Write a package directory + registry entry, as an install would leave it. */ + function install(packageId: string, manifest: string): void { + const dir = join(packagesDir, packageId) + mkdirSync(dir, { recursive: true }) + writeFileSync(join(dir, 'manifest.json'), manifest) + writeFileSync( + join(packagesDir, 'registry.json'), + JSON.stringify({ + formatVersion: '1.0', + packages: { + [packageId]: { version: '1.0.0', installedAt: '2026-01-01T00:00:00Z', path: dir, devices: ['slm-rp4'] }, + }, + }), + ) + } + + beforeEach(() => { + userDataDir = mkdtempSync(join(tmpdir(), 'pkg-read-')) + packagesDir = join(userDataDir, 'packages') + ;(app.getPath as jest.Mock).mockReturnValue(userDataDir) + warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + warnSpy.mockRestore() + jest.clearAllMocks() + rmSync(userDataDir, { recursive: true, force: true }) + }) + + it('returns the manifest of a package installed with a well-formed floor', () => { + install('vendor.board', manifestJson({ minEditorVersion: '4.3' })) + const manifest = new PackageManagerModule().getInstalledPackageManifest('vendor.board') + expect(manifest?.package.minEditorVersion).toBe('4.3') + }) + + it('keeps resolving a package whose stored floor this editor cannot compare', () => { + // The regression this guards: such a package was installed by an editor + // predating the format check (DOPE-448). Rejecting its manifest on load + // would unresolve its boards on upgrade, with no message anywhere. + install('vendor.board', manifestJson({ minEditorVersion: 'nightly' })) + + const manifest = new PackageManagerModule().getInstalledPackageManifest('vendor.board') + + expect(manifest?.package.id).toBe('vendor.board') + expect(manifest?.package).not.toHaveProperty('minEditorVersion') + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('minEditorVersion')) + }) + + it('still returns null for a document that is not a manifest', () => { + install('vendor.board', JSON.stringify({ nothing: 'useful' })) + expect(new PackageManagerModule().getInstalledPackageManifest('vendor.board')).toBeNull() + }) +}) diff --git a/src/backend/editor/package-manager/package-manager-module.ts b/src/backend/editor/package-manager/package-manager-module.ts index 1b3bad1ba..8df5fde12 100644 --- a/src/backend/editor/package-manager/package-manager-module.ts +++ b/src/backend/editor/package-manager/package-manager-module.ts @@ -5,7 +5,10 @@ import { join } from 'path' import { APP_VERSION } from '../../../frontend/data/constants/app-version' import { isCompatibleEditorVersion } from '../../../frontend/utils/semver' -import { PackageManifestSchema } from '../../../middleware/shared/ports/package-manifest-schema' +import { + PackageManifestSchema, + parseInstalledPackageManifest, +} from '../../../middleware/shared/ports/package-manifest-schema' import type { VppDeviceMatch } from '../../shared/hardware/find-vpp-device' import { findVppDeviceByBoardName } from '../../shared/hardware/find-vpp-device' import { validatePathId } from '../../shared/utils/path-safety' @@ -294,8 +297,13 @@ class PackageManagerModule { } catch { return null } - const parsed = PackageManifestSchema.safeParse(raw) - return parsed.success ? (parsed.data as unknown as PackageManifest) : null + // Read path, not the trust boundary: `importFromFile` above is where a + // manifest is refused. Here the package is already installed, and a + // manifest that was accepted by an older editor — one whose schema did + // not yet check the floor format (DOPE-448) — must keep resolving, or the + // boards it provides vanish from the board lookup with no message. An + // unreadable floor is dropped and logged; everything else still rejects. + return parseInstalledPackageManifest(raw) } getPackagePath(packageId: string): string | null { diff --git a/src/frontend/utils/semver.ts b/src/frontend/utils/semver.ts index 62770e383..70e0716de 100644 --- a/src/frontend/utils/semver.ts +++ b/src/frontend/utils/semver.ts @@ -141,7 +141,8 @@ export function compareParsedVersions(a: ParsedVersion, b: ParsedVersion): -1 | * - `minimum` present but unreadable → **pass**, because an unknown floor is * worth 0.0.0 and everything clears 0.0.0. Callers that can see the string * should say so out loud rather than let it vanish — the manifest schema - * rejects such a value outright, and the runtime probe logs a warning. + * rejects such a value when a package is installed and logs it when one + * already installed is read back, and the runtime probe logs a warning. * - `candidate` unreadable against a real floor → **fail**. An unknown * version never satisfies a declared minimum. */ diff --git a/src/middleware/adapters/editor/__tests__/package-adapter.test.ts b/src/middleware/adapters/editor/__tests__/package-adapter.test.ts index 2e28e163e..b75d7748d 100644 --- a/src/middleware/adapters/editor/__tests__/package-adapter.test.ts +++ b/src/middleware/adapters/editor/__tests__/package-adapter.test.ts @@ -126,6 +126,24 @@ describe('createEditorPackageAdapter', () => { expect(await adapter.getManifest('missing')).toBeNull() }) + it('keeps the manifest usable when its stored compatibility floor is unreadable', async () => { + // Read path, not the install boundary (DOPE-448): a package installed + // before the floor format was checked must still render and still + // provide its boards. The floor is dropped with a log, not the manifest. + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}) + ;(window.bridge.getPackageManifest as jest.Mock).mockResolvedValue({ + ...validManifest, + package: { ...validManifest.package, minEditorVersion: 'nightly' }, + }) + + const result = await adapter.getManifest('acme-controller') + + expect(result?.package.id).toBe('acme-controller') + expect(result?.package).not.toHaveProperty('minEditorVersion') + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('minEditorVersion')) + warnSpy.mockRestore() + }) + it('returns null when the bridge returns a malformed manifest (zod rejection)', async () => { // Suppress the validation warning the schema emits; we want to // assert the rejection, not the noise. diff --git a/src/middleware/adapters/editor/package-adapter.ts b/src/middleware/adapters/editor/package-adapter.ts index 7d4672dea..0d3caeded 100644 --- a/src/middleware/adapters/editor/package-adapter.ts +++ b/src/middleware/adapters/editor/package-adapter.ts @@ -20,7 +20,7 @@ * importFromFile`) can run unchanged. */ -import { parsePackageManifest } from '../../shared/ports/package-manifest-schema' +import { parseInstalledPackageManifest } from '../../shared/ports/package-manifest-schema' import type { PackagePort } from '../../shared/ports/package-port' import type { ImportResult, @@ -92,9 +92,14 @@ export function createEditorPackageAdapter(): PackagePort { // manifest.json. Validate the shape here before handing it to UI // code — drift between port type and on-disk JSON is a real risk // that an unchecked cast would silently absorb. + // + // Same read-path tolerance as the main process applies (DOPE-448): this + // manifest belongs to an installed package, so an unreadable + // compatibility floor is dropped rather than taking the whole manifest — + // and the package's boards — down with it. const raw = await window.bridge.getPackageManifest(packageId) if (raw === null || raw === undefined) return null - return parsePackageManifest(raw) + return parseInstalledPackageManifest(raw) }, async listRemoteCatalog(): Promise { diff --git a/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts b/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts index b9fa4458d..e1ae083c3 100644 --- a/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts +++ b/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts @@ -1,4 +1,4 @@ -import { PackageManifestSchema } from '../package-manifest-schema' +import { PackageManifestSchema, parseInstalledPackageManifest } from '../package-manifest-schema' /** A minimal manifest the schema accepts, with room to override `package`. */ const manifest = (pkg: Record = {}) => ({ @@ -64,3 +64,77 @@ describe('PackageManifestSchema — compatibility floors', () => { expect(result.success).toBe(true) }) }) + +// The other half of the same rule: strict where a package ENTERS the editor, +// tolerant where an installed one is READ BACK. Without this split, the format +// check above would retroactively unresolve a package installed by an older +// editor — its boards would vanish from the board lookup with no message, on an +// upgrade where the user did nothing. +describe('parseInstalledPackageManifest — the load path', () => { + let warnSpy: jest.SpyInstance + + beforeEach(() => { + warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + warnSpy.mockRestore() + }) + + it('reads a well-formed manifest exactly as the strict parser does', () => { + const parsed = parseInstalledPackageManifest(manifest({ minEditorVersion: '4.3' })) + expect(parsed?.package.minEditorVersion).toBe('4.3') + expect(warnSpy).not.toHaveBeenCalled() + }) + + it.each([ + ['minEditorVersion', 'garbage'], + ['minRuntimeVersion', '4,3,0'], + ])('drops an uncomparable %s and keeps the package readable', (field, value) => { + const parsed = parseInstalledPackageManifest(manifest({ [field]: value })) + + // The package still resolves — this is the whole point — and the floor it + // could never have enforced is simply gone. + expect(parsed?.package.id).toBe('vendor.board') + expect(parsed?.package).not.toHaveProperty(field) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining(field)) + }) + + it('drops a floor that is not even a string', () => { + const parsed = parseInstalledPackageManifest(manifest({ minEditorVersion: 43 })) + expect(parsed?.package).not.toHaveProperty('minEditorVersion') + }) + + it('drops only the unreadable floor, leaving the readable one enforced', () => { + const parsed = parseInstalledPackageManifest(manifest({ minEditorVersion: 'garbage', minRuntimeVersion: '4.2' })) + expect(parsed?.package).not.toHaveProperty('minEditorVersion') + expect(parsed?.package.minRuntimeVersion).toBe('4.2') + }) + + it('leaves every other field of the package untouched', () => { + const parsed = parseInstalledPackageManifest( + manifest({ minEditorVersion: 'garbage', vendorExtension: { anything: true } }), + ) + expect(parsed?.package).toMatchObject({ + id: 'vendor.board', + name: 'Vendor Board', + version: '1.0.0', + vendorExtension: { anything: true }, + }) + }) + + it('still rejects a manifest that is malformed for any other reason', () => { + // Tolerance is scoped to the floors. A document missing `devices` is not a + // manifest, and reading it as one would crash deeper in the loader. + expect(parseInstalledPackageManifest({ formatVersion: '1.0', package: { id: 'x' } })).toBeNull() + }) + + it.each([ + ['a non-object', 'not a manifest'], + ['null', null], + ['an array', []], + ['a manifest whose package is not an object', { formatVersion: '1.0', package: 'nope', devices: [{ id: 'a' }] }], + ])('passes %s straight to the schema rather than guessing at it', (_label, value) => { + expect(parseInstalledPackageManifest(value)).toBeNull() + }) +}) diff --git a/src/middleware/shared/ports/package-manifest-schema.ts b/src/middleware/shared/ports/package-manifest-schema.ts index ba9cd4d86..fb89d961f 100644 --- a/src/middleware/shared/ports/package-manifest-schema.ts +++ b/src/middleware/shared/ports/package-manifest-schema.ts @@ -50,6 +50,11 @@ import type { PackageManifest } from './types' * It matters most for the path this gate exists for: a `.vpp` added * from disk never passes through openplc-packages' `scripts/validate.ts`, * so for sideloaded packages this schema is the only boundary there is. + * + * Refusing applies to the artefact ENTERING the editor. Reading a + * package that is already installed goes through + * `parseInstalledPackageManifest` below, which drops such a floor + * instead of rejecting the manifest around it. */ const versionFloor = z .string() @@ -102,3 +107,63 @@ export function parsePackageManifest(value: unknown): PackageManifest | null { // validation in openplc-packages for the deeper fields. return parsed.data as unknown as PackageManifest } + +/** The manifest fields `versionFloor` guards, as read by the load path. */ +const FLOOR_FIELDS: readonly string[] = ['minEditorVersion', 'minRuntimeVersion'] + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +/** A floor that is absent constrains nothing; one that is present must be comparable. */ +function isUsableFloor(value: unknown): boolean { + return value === undefined || (typeof value === 'string' && isValidVersion(value)) +} + +/** + * Return `value` with any compatibility floor this codebase cannot + * compare removed, logging each one. Anything else is passed through + * untouched, including a shape that is not a manifest at all — deciding + * that is the schema's job, not this function's. + */ +function withComparableFloorsOnly(value: unknown): unknown { + if (!isRecord(value) || !isRecord(value.package)) return value + + const kept: Record = {} + let droppedAny = false + for (const [field, fieldValue] of Object.entries(value.package)) { + if (FLOOR_FIELDS.includes(field) && !isUsableFloor(fieldValue)) { + console.warn( + `[package-manifest] installed package declares an unreadable ${field} (${JSON.stringify(fieldValue)}); ` + + `ignoring it — the compatibility floor it intends cannot be enforced`, + ) + droppedAny = true + continue + } + kept[field] = fieldValue + } + + return droppedAny ? { ...value, package: kept } : value +} + +/** + * Validate a manifest read back from a package that is ALREADY + * INSTALLED, dropping a floor this codebase cannot compare rather than + * rejecting the whole document. + * + * Strict where the artefact enters, tolerant where we are only reading + * what is already on disk. `importFromFile` refuses an unreadable floor + * — that is the boundary, and refusing there is what makes the promise + * in `docs/package-format.md` true. But a package installed BEFORE that + * boundary existed can carry such a floor, and rejecting its manifest on + * load would make the boards it provides disappear from the board lookup + * with no message, on an upgrade where the user did nothing. + * + * Dropping the field leaves the package exactly as unconstrained as it + * already was — an unreadable floor never gated anything (see + * `isVersionAtLeast`) — while the log keeps the cause visible instead of + * silently trading one invisible outcome for another. + */ +export function parseInstalledPackageManifest(value: unknown): PackageManifest | null { + return parsePackageManifest(withComparableFloorsOnly(value)) +}