diff --git a/docs/version-compatibility-strategy.md b/docs/version-compatibility-strategy.md new file mode 100644 index 000000000..99304a838 --- /dev/null +++ b/docs/version-compatibility-strategy.md @@ -0,0 +1,473 @@ +# Editor + Runtime + VPP Version Compatibility — Strategy + +> 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 + +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 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 + +`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`). + +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 + +`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 — the one +that had no enforcement at all before this work. + +**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/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 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 + +```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` | 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 +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 as implemented | +| -------------------------------------------------------- | ---------------------------------------------------------------------------------------------- | +| Runtime without `/api/capabilities` | no editor floor to check; `MIN_RUNTIME_VERSION` gate still applies; **silent** — see below | +| 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 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 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. 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 + +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 — 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` 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 +one gate covers both the catalog and the "Add from file…" path. +`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 +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 + +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 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. + +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. + + 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, )`. + (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** + +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. **`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. 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/__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 0aaf17a3e..8c2309eb3 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' @@ -119,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' @@ -2011,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`, @@ -2289,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 @@ -2634,6 +2608,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, @@ -2716,6 +2695,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 8c78488b2..2a5e88beb 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. */ @@ -503,8 +519,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 @@ -513,19 +535,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 } }, /** @@ -568,7 +592,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 { @@ -598,6 +627,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/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 7319b5837..8df5fde12 100644 --- a/src/backend/editor/package-manager/package-manager-module.ts +++ b/src/backend/editor/package-manager/package-manager-module.ts @@ -3,7 +3,14 @@ import extract from 'extract-zip' import { existsSync, mkdirSync, mkdtempSync, readFileSync, renameSync, rmSync, writeFileSync } from 'fs' import { join } from 'path' -import { PackageManifestSchema } from '../../../middleware/shared/ports/package-manifest-schema' +import { APP_VERSION } from '../../../frontend/data/constants/app-version' +import { isCompatibleEditorVersion } from '../../../frontend/utils/semver' +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' import { TRUSTED_PACKAGE_KEYS } from '../../shared/utils/vpp/trusted-keys' import { verifyPackageSignature } from '../../shared/utils/vpp/verify-package-signature' @@ -79,6 +86,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 @@ -268,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 { @@ -278,6 +312,38 @@ 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). + * + * 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 { + const match = this.findDeviceByBoardName(boardName) + if (!match || match.device.target.type !== 'runtime-v4') return null + return match.manifest.package.minRuntimeVersion ?? null + } + private readRegistry(): PackageRegistry { if (!existsSync(this.registryPath)) { return { formatVersion: '1.0', packages: {} } 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/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 179010a1a..8313493ad 100644 --- a/src/backend/shared/firmware/runtime-version-gate.ts +++ b/src/backend/shared/firmware/runtime-version-gate.ts @@ -14,65 +14,81 @@ * * 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 * 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 { formatVersionForDisplay, isVersionAtLeast, parseVersion } 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 `frontend/utils/semver`. */ +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". + * 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 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 { - 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 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 @@ -88,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) } /** @@ -101,10 +113,54 @@ 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_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 = formatVersionForDisplay(args.runtimeVersion) + 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 = 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. ` + + `${where} ${runtime}. ` + + `Upgrade the runtime on that device, or select a board supported by ${runtime}.` + ) +} 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 e090b5c77..c5d8b9c88 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,188 @@ 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() + }) + + // 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({ + 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..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. */ @@ -50,6 +55,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 +86,88 @@ 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 + const minEditorVersion = extractStringField(result.body, 'minEditorVersion') + // A floor that is present but unreadable is the one case that must not + // pass in silence. `isVersionAtLeast` treats it as "declares nothing", + // which is the safe answer for an upload but the wrong one for whoever + // wrote it: the runtime believes it is enforcing a constraint that is + // not being applied, and without this line the only way to find out is a + // field incident. Accepted shorthands (`"4.3"`, `"4"`, `"v5"`) parse, so + // this fires on genuine junk only. + if (minEditorVersion !== null && !isValidVersion(minEditorVersion)) { + opts.log( + `Runtime declared an unreadable minEditorVersion ("${minEditorVersion}") — the editor-version floor is not being enforced.`, + 'warning', + ) + } + return { version, minEditorVersion } + } catch { + return null } } @@ -91,8 +179,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/frontend/utils/__tests__/semver.test.ts b/src/frontend/utils/__tests__/semver.test.ts index 97ec6d22f..ff301864d 100644 --- a/src/frontend/utils/__tests__/semver.test.ts +++ b/src/frontend/utils/__tests__/semver.test.ts @@ -1,4 +1,233 @@ -import { compareSemver, isCompatibleEditorVersion } from '../semver' +import { + compareParsedVersions, + compareSemver, + formatVersionForDisplay, + isCompatibleEditorVersion, + isValidVersion, + isVersionAtLeast, + parseVersion, +} from '../semver' + +describe('parseVersion', () => { + it('parses a plain three-part version', () => { + expect(parseVersion('4.1.9')).toEqual({ major: 4, minor: 1, patch: 9, 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(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(parseVersion(' 4.1.9 ')).toEqual({ major: 4, minor: 1, patch: 9, prerelease: undefined }) + }) + + // 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([ + ['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(parseVersion(input)).toBeNull() + }) + + it('returns null for null and undefined', () => { + 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', () => { + 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('rejects what the parser cannot read', () => { + for (const raw of ['garbage', 'next', '', ' ', '4,3,0', null, undefined]) { + expect(isValidVersion(raw)).toBe(false) + } + }) +}) + +describe('formatVersionForDisplay', () => { + it('trims a readable version', () => { + expect(formatVersionForDisplay(' v4.2.0 ')).toBe('v4.2.0') + }) + + // 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'], + [' ', 'whitespace only'], + [null, 'null'], + [undefined, 'undefined'], + ] + + it.each(UNREADABLE)('renders %p as "unknown" (%s)', (raw) => { + expect(formatVersionForDisplay(raw)).toBe('unknown') + }) +}) + +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('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) + }) + + 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) + }) + + // 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]> = [ + [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) + }) + + // 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', 'next')).toBe(true) + }) + + // Fails closed: an unidentifiable peer never clears a real floor. + const UNIDENTIFIABLE: Array<[string | null | undefined, string]> = [ + ['dev', 'a source build'], + ['garbage', 'a corrupt value'], + ['', 'a blank answer'], + [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) + }) + + // 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', () => { it('returns 0 when versions are identical', () => { @@ -25,18 +254,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) }) }) @@ -59,4 +298,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 673a026eb..70e0716de 100644 --- a/src/frontend/utils/semver.ts +++ b/src/frontend/utils/semver.ts @@ -1,38 +1,179 @@ /** - * 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 — `"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. + * + * 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: + * + * - 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 + * + * "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. + * + * 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 +} + +/** + * `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+))?(?:[-+](.+))?$/ -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] +/** 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 } -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, or return null when the string is not a version. + * + * 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 parseVersion(raw: string | null | undefined): ParsedVersion | null { + if (!raw) return null + const match = raw.trim().match(VERSION_RE) + if (!match) return null + 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. */ +export function isValidVersion(raw: string | null | undefined): boolean { + return parseVersion(raw) !== null +} + +/** + * 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. + * + * 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 formatVersionForDisplay(raw: string | null | undefined): string { + const trimmed = raw?.trim() ?? '' + return trimmed.length > 0 ? trimmed : 'unknown' +} + +/** + * 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` — the one comparison every DOPE-448 gate asks. + * + * Three inputs and what each costs: + * + * - `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 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. + */ +export function isVersionAtLeast(candidate: string | null | undefined, minimum: string | null | undefined): boolean { + const min = parseVersion(minimum) + if (!min) return true + const version = parseVersion(candidate) + if (!version) return false + return compareParsedVersions(version, min) >= 0 +} + +/** + * Order two version strings for display purposes — sorting catalog rows, + * deciding whether an available version is newer than the installed one. + * + * 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(parseVersion(a) ?? ZERO, parseVersion(b) ?? ZERO) +} + +/** + * 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/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 new file mode 100644 index 000000000..e1ae083c3 --- /dev/null +++ b/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts @@ -0,0 +1,140 @@ +import { PackageManifestSchema, parseInstalledPackageManifest } 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) + }) +}) + +// 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/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 } // --------------------------------------------------------------------------- diff --git a/src/middleware/shared/ports/package-manifest-schema.ts b/src/middleware/shared/ports/package-manifest-schema.ts index 815230e3e..fb89d961f 100644 --- a/src/middleware/shared/ports/package-manifest-schema.ts +++ b/src/middleware/shared/ports/package-manifest-schema.ts @@ -34,8 +34,33 @@ 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. + * + * 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() + .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), @@ -44,6 +69,19 @@ 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. + // 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), @@ -69,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)) +} diff --git a/src/middleware/shared/ports/types.ts b/src/middleware/shared/ports/types.ts index eb644ceb4..c40fe5f20 100644 --- a/src/middleware/shared/ports/types.ts +++ b/src/middleware/shared/ports/types.ts @@ -730,7 +730,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