feat(compat): editor + runtime + VPP version compatibility (DOPE-448) - #993
Conversation
The codebase answered "is X at least Y" two different ways, and they disagreed on exactly the inputs that show up in the field: input | semver.ts | runtime-version-gate.ts ------------|----------------|------------------------ "v4" | 4.0.0 | rejected "4.1" | 4.1.0 | rejected "garbage" | 0.0.0 (lowest) | rejected Neither was wrong for its own caller: a corrupt package manifest should not break a card in the catalog UI, and an unidentifiable runtime must not receive an upload. What was wrong is that the DIFFERENCE lived in two separate parsers, where nothing named it and nothing tested it side by side — and DOPE-448 adds three more comparisons on top. Now one parse, one ordering, and the lenient-vs-strict choice made by name at the call site: parseVersionStrict fails closed, parseVersionLenient degrades to 0.0.0. parseRuntimeVersion and compareSemver become thin wrappers, so no call site changes and their tests pass untouched. It lives in frontend/utils/ rather than backend/shared/ because the layer rules let backend-shared import utils and not the reverse, and both the VPP surface and the runtime gates need it. No layer exception. Also adds the two message builders the gates will use, and renames MIN_STRUCPP_RUNTIME_VERSION to MIN_RUNTIME_VERSION (old name kept as an alias) so the constant says what it is rather than why it appeared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
manifest.package.minEditorVersion has been required by the schema all along, and openplc-packages/docs/package-format.md:69 promised the editor refuses to install a package requiring a newer editor. It did not. The only consumer was the catalog UI (catalog-browser.tsx), which renders an "Editor outdated" button state. package-manager-module.ts::install — the trust boundary that both the remote install and the local "Add from file…" flow converge on, which already validates the schema, verifies the signature and hardens the path — never looked at the field. So a .vpp dragged in from disk ignored it completely, and a package installed before an editor downgrade kept loading. The check now sits next to the signature verification, so one gate covers both entry paths. This is also the mechanism that answers "what about a VPP needing a UI engine the editor may not have": the engine shipped in some release, the package declares that release as its floor, an older editor cannot install it. No capability enumeration needed. minEditorVersion and minRuntimeVersion are typed in the zod schema rather than left to .passthrough() — a field a gate reads should not reach it as unknown. Both stay optional so packages built before they existed keep installing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(DOPE-448) Closes the two directions that had no enforcement at all. Runtime -> editor: probe-runtime-version.ts now prefers GET /api/capabilities, which carries the runtime version and its minEditorVersion in one round-trip, and falls back to /api/version otherwise. A runtime predating the endpoint answers 401 from the /<command> catch-all (not 404) — both land in the same fallback, silently, because that 401 is the normal answer from every device in the field and a warning there would nag on every upload. VPP -> runtime: packageVppPlugin surfaces the package's minRuntimeVersion, compared against the connected runtime before the upload. It cannot be checked at install time — the target device is unknown until the user connects — and it must be checked before sending, because the failure it prevents is vendor plugin code that loads on a live PLC and dies at scan time. Both gates are inert against everything currently deployed, in two independent ways: a runtime that declares no floor passes, and a caller that passes no editorVersion passes. editorVersion is injected through RunCompilePipelineArgs rather than imported: APP_VERSION lives in frontend/data/, which the layer rules keep out of backend/shared/ — correctly, since which build is running is a fact about the host app, not about the compile. Same reasoning for getVppRuntimeFloor on the adapter context, which additionally avoids pulling the Electron-dependent logger in at module load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…OPE-448) Three artefacts on independent release cadences, five arcs that can be mismatched in both directions, and four disconnected checks that were each invented separately. Documents the agreed design — each component declares a minimum, the editor is the only component that compares — plus what exists today with file:line references, the error-message rules, and the delivery phases. Section 9 records what was considered and dropped, so nobody re-derives it: monotonic integer contracts, a bundle-manifest.json inside the upload ZIP, an editor->runtime advertising handshake, and max* bounds (an upper bound naming releases that do not exist yet is unknowable). Section 7 records the accepted limitation in its own section: the runtime advertises rather than enforces, so a client that skips the check can still upload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds shared semver handling and compatibility metadata. It checks editor versions during package installation and runtime v4 uploads. Runtime capabilities use legacy fallback. VPP runtime floors and diagnostic messages flow through compilation. ChangesCompatibility enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant CompilerModule
participant Runtime
participant CompilePipeline
Editor->>CompilerModule: start compilation
CompilerModule->>Runtime: GET /api/capabilities
Runtime-->>CompilerModule: runtimeVersion and minEditorVersion
CompilerModule->>CompilePipeline: editorVersion and VPP runtime floor
CompilePipeline-->>Editor: allow or reject runtime v4 upload
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
docs/version-compatibility-strategy.md (1)
126-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to the fenced code blocks.
markdownlint-cli2reports MD040 for these fences. Add identifiers such ashttp,json, ortextafter each opening fence so the document passes the configured Markdown lint.Also applies to: 139-139, 147-147, 156-156, 182-182, 203-203, 228-228, 234-234, 240-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/version-compatibility-strategy.md` at line 126, Update the fenced code blocks in docs/version-compatibility-strategy.md, including the blocks at the referenced locations, by adding an appropriate language identifier such as http, json, or text after every opening fence so the document passes markdownlint MD040.Source: Linters/SAST tools
src/backend/shared/library/__tests__/probe-runtime-version.test.ts (1)
113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
versionMustNotBeCalledenforce its name.The helper returns a stub that resolves successfully. It does not fail when the fallback runs. At lines 140 and 153 the returned spy is discarded, so nothing checks the call count there. The
toEqualassertions still catch an unwanted fallback through the'FALLBACK'version, so no test is wrong today. A throwing stub states the contract at every call site and removes the need to capture the spy.♻️ Proposed helper change
- /** 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 - } + /** A `fetchVersion` that fails the test if the fallback is reached. */ + const versionMustNotBeCalled = () => + jest.fn((): Promise<FetchVersionResult> => { + throw new Error('fetchVersion must not be called when capabilities answered') + })Note: the probe catches throws from
fetchVersion, so keep the resulttoEqualassertions as the primary check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/shared/library/__tests__/probe-runtime-version.test.ts` around lines 113 - 117, Update the versionMustNotBeCalled helper to return a fetchVersion stub that throws when invoked instead of resolving with a FALLBACK result. Keep the existing result toEqual assertions unchanged, since they remain the primary checks for probe behavior.src/backend/shared/library/probe-runtime-version.ts (1)
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead the field without a type assertion.
Line 175 uses
as Record<string, unknown>, which the coding guidelines forbid.Object.getOwnPropertyDescriptorreads the value with no cast. It also restricts the lookup to own properties, so a prototype key such asconstructorcannot satisfy theincheck.♻️ Proposed cast-free implementation
function extractStringField(body: unknown, field: string): string | null { if (typeof body !== 'object' || body === null) return null - if (!(field in body)) return null - const value = (body as Record<string, unknown>)[field] + const value: unknown = Object.getOwnPropertyDescriptor(body, field)?.value return typeof value === 'string' ? value : null }As per coding guidelines: "Do not use type assertions, except
as const;as unknown as Tis forbidden."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/shared/library/probe-runtime-version.ts` around lines 172 - 177, Update extractStringField to remove the Record<string, unknown> type assertion and read the requested field via Object.getOwnPropertyDescriptor. Preserve returning the string value when present, null for non-string values, and restrict matches to own properties rather than inherited prototype keys.Source: Coding guidelines
src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts (1)
359-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
as unknown ascasts on the bridge stubs.The coding guidelines forbid
as unknown as T. Both stubs need the double cast only because the arrow function returns concrete object literals instead of the genericTthatmakeRuntimeApiRequestdeclares. Define one typed helper that maps endpoints to bodies and returns the bridge signature, then reuse it in both tests.♻️ Proposed helper to remove the double casts
type RuntimeApiRequest = EditorCompilerPlatformPortContext['mainProcessBridge']['makeRuntimeApiRequest'] /** Answer each endpoint with a pre-parsed body, typed as the bridge expects. */ function makeApiStub(bodies: Record<string, unknown>, fallback: unknown): RuntimeApiRequest { const impl: RuntimeApiRequest = (_ip, endpoint) => { const body = endpoint in bodies ? bodies[endpoint] : fallback if (body === undefined) return Promise.resolve({ success: false, error: '404 Not Found' }) // The bridge's generic is resolved by the caller's parser; tests hand back // the already-parsed body for the single `unknown` instantiation in use. return Promise.resolve({ success: true, data: body } as ReturnType<RuntimeApiRequest>) } return jest.fn(impl) }As per coding guidelines: "Do not use type assertions, except
as const;as unknown as Tis forbidden."Also applies to: 377-380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts` around lines 359 - 364, Replace the `as unknown as` casts on both `makeRuntimeApiRequest` stubs with one shared typed helper. Define a `RuntimeApiRequest` alias and a `makeApiStub` helper that maps endpoints to response bodies, returns the bridge-compatible signature, and is reused by both tests; preserve the existing endpoint responses and avoid type assertions except `as const`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/version-compatibility-strategy.md`:
- Around line 39-76: Update the compatibility strategy document so the baseline
sections are explicitly labeled as pre-implementation historical context, and
revise the delivery-plan sections around the referenced later content to reflect
the shipped implementation. Mark completed phases and remove or update obsolete
future rollout instructions, ensuring maintainers are not directed to implement
features that already exist.
- Around line 154-165: Update the legacy-runtime section to describe the
no-version fallback for any failed capabilities request or response lacking
runtimeVersion, including 401 and 404 responses, rather than hard-coding 404.
State that the runtime version check proceeds without a declared floor and no
warning is emitted for this normal fallback, matching probeRuntimeVersion and
the editor compiler platform-port tests.
In `@src/backend/shared/firmware/runtime-version-gate.ts`:
- Around line 47-48: Update the deprecation notice for ParsedRuntimeVersion to
reference the actual migration target module src/frontend/utils/semver.ts, where
ParsedVersion is imported from, instead of the nonexistent
shared/utils/version-compare path.
In `@src/frontend/utils/semver.ts`:
- Around line 70-74: Update the strict parser’s parsing flow near the returned
version object to validate major, minor, and patch with Number.isSafeInteger
after conversion. Reject the version when any component is non-safe, while
preserving existing behavior for valid safe integer components and prerelease
handling.
In `@src/middleware/shared/ports/package-manifest-schema.ts`:
- Around line 47-57: Update the minEditorVersion and minRuntimeVersion schemas
in the package manifest schema to validate present values as strict x.y.z
versions, rejecting malformed or whitespace-only inputs while keeping omitted
fields valid. Reuse the existing version-validation symbol if one is available;
otherwise apply equivalent strict validation directly to both zod fields.
---
Nitpick comments:
In `@docs/version-compatibility-strategy.md`:
- Line 126: Update the fenced code blocks in
docs/version-compatibility-strategy.md, including the blocks at the referenced
locations, by adding an appropriate language identifier such as http, json, or
text after every opening fence so the document passes markdownlint MD040.
In `@src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts`:
- Around line 359-364: Replace the `as unknown as` casts on both
`makeRuntimeApiRequest` stubs with one shared typed helper. Define a
`RuntimeApiRequest` alias and a `makeApiStub` helper that maps endpoints to
response bodies, returns the bridge-compatible signature, and is reused by both
tests; preserve the existing endpoint responses and avoid type assertions except
`as const`.
In `@src/backend/shared/library/__tests__/probe-runtime-version.test.ts`:
- Around line 113-117: Update the versionMustNotBeCalled helper to return a
fetchVersion stub that throws when invoked instead of resolving with a FALLBACK
result. Keep the existing result toEqual assertions unchanged, since they remain
the primary checks for probe behavior.
In `@src/backend/shared/library/probe-runtime-version.ts`:
- Around line 172-177: Update extractStringField to remove the Record<string,
unknown> type assertion and read the requested field via
Object.getOwnPropertyDescriptor. Preserve returning the string value when
present, null for non-string values, and restrict matches to own properties
rather than inherited prototype keys.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 641001ed-c3c1-45a5-bab6-9746728654ab
📒 Files selected for processing (15)
docs/version-compatibility-strategy.mdsrc/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.tssrc/backend/editor/compiler/compiler-module.tssrc/backend/editor/compiler/editor-compiler-platform-port.tssrc/backend/editor/package-manager/package-manager-module.tssrc/backend/shared/compile/__tests__/pipeline.test.tssrc/backend/shared/compile/pipeline.tssrc/backend/shared/firmware/runtime-version-gate.tssrc/backend/shared/library/__tests__/probe-runtime-version.test.tssrc/backend/shared/library/probe-runtime-version.tssrc/frontend/utils/__tests__/semver.test.tssrc/frontend/utils/semver.tssrc/middleware/shared/ports/compiler-platform-port.tssrc/middleware/shared/ports/package-manifest-schema.tssrc/middleware/shared/ports/types.ts
Review — no blockers; four things worth fixing, one of them before mergeThe web half (Autonomy-Logic/openplc-web#652) is approved — verified byte-identical (10/10 via How I verified this side: 145 tests passing across All four arcs of the compatibility matrix are wired, and the editor is the only decider:
Checking the VPP↔runtime arc before upload rather than at install is the right call and the comment says exactly why: the target device isn't known until the user connects. I measured the comparator's decision table, since that's where this kind of code goes wrong: The first three are exactly right. The fourth is deliberate and documented — just worth keeping the process assumption true: if rc tags ever start being cut before the feature lands rather than after, this gate silently admits an editor without it.
|
| Finding | File | Also in web#652? |
|---|---|---|
| 🟡 1 unreadable floor dropped silently | frontend/utils/semver.ts / backend/shared/library/probe-runtime-version.ts |
yes |
| 🟡 2 manifest schema accepts malformed floor | middleware/shared/ports/package-manifest-schema.ts |
yes |
| 🟡 4 hand-rolled comparators remain | backend/shared/firmware/runtime-version-gate.ts |
yes |
| 🟡 3 strategy doc | docs/version-compatibility-strategy.md |
no — editor only |
Whatever you change in the first three has to land in web#652 in the same shape, or ci-sync breaks for every other PR touching shared surface. Worth doing both commits together.
🟡 1 — A floor that is present but unreadable is discarded with no signal
Measured:
false isVersionAtLeast("4.2", "4.2.0") ← "4.2" as the CANDIDATE: fatal, blocks the upload
true isVersionAtLeast("4.2.0", "4.2") ← "4.2" as the FLOOR: ignored entirely
true isVersionAtLeast("4.2.0", "garbage") ← same
What I understood. semver.ts documents this ("a floor we cannot read declares nothing") and the choice is defensible — failing closed on a typo in a peer's metadata would be worse. What strikes me is the asymmetry: the same string is fatal in one direction, with a user-visible message, and invisible in the other.
Why it needs fixing. If the runtime team ships minEditorVersion: "4.2" — a plausible shorthand, hand-written in restapi.py — the gate simply doesn't run, and nothing tells anyone. There's no test on this side that could catch it, because the symptom is the absence of a symptom. The log channel is already threaded through the probe:
if (minimum && !min) log(`Runtime declared an unreadable minEditorVersion ("${minimum}") — floor ignored`, 'warning')Impact if left. A compatibility constraint the runtime believes it is enforcing is silently not enforced, and the only way to discover it is a field incident.
🟡 2 — CodeRabbit's Major holds: a malformed floor becomes "no floor"
package-manifest-schema.ts:56-57 accepts any non-empty string, and the install gate compares leniently. Measured:
true isCompatibleEditorVersion("garbage", "4.0.0") ← installs despite declaring a floor
false isCompatibleEditorVersion("4.3", "4.2.9") ← a 2-part floor IS honoured (4.3.0)
false isCompatibleEditorVersion("v5", "4.2.9") ← the v prefix IS honoured (5.0.0)
Why it needs fixing. The schema comment points at openplc-packages' scripts/validate.ts for format rules, and that's true for published packages. But your own comment on the install gate says it exists because a .vpp dragged in from disk bypassed the constraint entirely — and a sideloaded package never passes through that validator. For exactly the entry path this gate was added to protect, this schema is the only boundary, and it lets a malformed floor through.
Worth noting too: isCompatibleEditorVersion is a gate, and it's on the lenient path, while semver.ts sets the rule that gates use isVersionAtLeast. By the letter it's fine (the install doesn't talk to a runtime); by the spirit, enforcement should fail closed.
Impact if left. A package that needs editor 4.3 installs on 4.0 and, per your own rationale, "produces a board that renders wrong rather than an error" — the exact failure the gate was written to prevent. The fix costs nothing in practice: since "4.3" and "v5" are already honoured, requiring a strict x.y.z when the field is present only changes behaviour for total garbage.
🟡 3 — The strategy doc presents shipped work as future (editor-only, cheapest fix)
- §2.2: "There is no endpoint where the runtime states what it needs from an editor" — no longer true once openplc-runtime#163 lands.
- Delivery plan: "Phase 1 — one semver parser… ~half a day", "Phase 2 — VPP install gate… ~1 day", "Phase 3 — minRuntimeVersion in the manifest" — phases 1-4 are this PR.
Why it needs fixing. This doc is the only place the four-arc matrix and the lenient-vs-strict rationale are written down, which makes it genuinely valuable — and that's exactly why a maintainer opening it in three months shouldn't read a roadmap of work that already exists and risk redoing or reverting it. Labelling the baseline as "as of DOPE-448 kickoff" and marking phases 1-4 landed (with PR links) is enough.
🟡 4 — Three hand-rolled comparisons survive in the PR whose thesis is unifying them
runtime-version-gate.ts keeps isStrucppCompatibleRuntime and isUserManagementCapableRuntime doing their own if (v.major > 4) … return v.minor >= 1 next to the constants they compare against. I checked both are exactly equivalent — null handling included — to:
isVersionAtLeast(raw, MIN_RUNTIME_VERSION)
isVersionAtLeast(raw, MIN_USER_MANAGEMENT_RUNTIME_VERSION)Why it's worth folding in now. Not a bug — it's the same duplication the PR set out to remove, one level up: the parser got unified, the comparators didn't. Your own module header argues the original problem was that "the DIFFERENCE lived in two separate parsers, where nothing named it and nothing tested it side by side". Leaving three comparators next to a shared one invites the fourth in the next capability gate.
🟢 Smaller
runtime-version-gate.ts:48— the@deprecatednote points atshared/utils/version-compare, which doesn't exist; the type comes fromfrontend/utils/semver. Anyone following it imports from nowhere. (CodeRabbit's, correct.)Number.parseIntloses precision above 2^53 in a version component — unrealistic, noted only to close that thread.- Seconding the
.gitattributesgap you documented yourself: worth the shared card, and openplc-runtime has the same hole.
What's good
- The two-parser divergence table in the
semver.tsheader is the real find: neither behaviour was wrong for its own caller, the wrong thing was that the difference had no name and no side-by-side test. Naming a root cause that precisely is rare. - Lenient vs strict chosen by name at the call site, with the reason for each — not a boolean flag the reader has to trace.
- The 401 field discovery: runtimes predating the endpoint don't 404, they fall into the
@restapi_bp.route("/<command>")catch-all behind@jwt_required()and come back 401 — verified against a real container, and it's why the probe keys off "can I read a version out of this" instead of a status code. That's the difference between code that works in a lab and code that works in the field. - Partial answers deliberately refused: capabilities without
runtimeVersionfalls back to/api/versionwholesale rather than trusting theminEditorVersionnext to it. - Layer reasoning explicit and right where it mattered:
APP_VERSIONinjected rather than imported (the rules keepbackend/sharedout offrontend/data), and the relative import in the gate on purpose, becausevalidate:archonly inspects relative specifiers — a@root/path would have skipped the check instead of passing it. git hash-objectfor the blob comparison — EOL-normalising, which is the correct tool given the CRLF problem you hit.- The install gate closes the sideload path, and all three message builders name both versions and the action that fixes it, including the alternative ("or connect to a runtime that accepts X").
Of the four, 🟡 3 is the one I'd want before merge — cheapest, editor-only, and the cost of leaving it is a misleading artefact. 🟡 2 is the only real enforcement gap I found. 🟡 1 and 🟡 4 can ride along or follow up; both make the system harder to silence by accident. Remember the first three need the mirrored commit on web#652.
Review assisted by Claude Code.
Review of #993 caught that the doc reads as a roadmap for work this PR already contains: §2.2 stated there is no endpoint where the runtime declares what it needs, and §8 listed phases 1-6 with day estimates. A maintainer opening this in three months could reasonably redo or revert what already exists. - Header states "implemented", lists the four shipping PRs, and tells the reader §2 is a dated snapshot of the state BEFORE this work, kept because the design rationale only makes sense against what it was fixing. - §8 becomes a landed-where table plus per-phase notes, no estimates. Cross-checking every claim against the code turned up three the doc asserted and the code does not do — all now corrected to describe what actually ships: - §4 claimed the VPP editor-floor check also runs "on load". It does not; it runs at install only, so a package installed under a newer editor keeps loading after a downgrade. Recorded as a known gap instead of a feature. - §6 promised a console warning for a runtime with no /api/capabilities. The fallback is deliberately silent, and the section now says why: that is every deployed device, so warning there would fire on every upload. - §6 promised a console warning when a runtime-v4 VPP declares no minRuntimeVersion. There is none; that case is caught at package build time by validate.ts instead. Also corrects 404 → 401 for the legacy-runtime fallback (the /<command> catch-all behind @jwt_required() swallows unknown paths), and turns §10 into decided-vs-open: the runtime's published floor is settled at 4.1.0, and the four findings from review are recorded with the note that three touch shared surface and need a mirrored commit in openplc-web. Docs only — no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/version-compatibility-strategy.md (1)
293-297: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the remaining
minRuntimeVersiongap as install-time enforcement.Line 293 calls this a future requirement, but Lines 338-341 state that build-time enforcement already shipped. The unresolved gap is installation-time validation for sideloaded packages, as described in Lines 284 and 401-408. Update this wording to distinguish the shipped build check from the remaining install check.
Proposed fix
- The only future tightening worth scheduling is making `minRuntimeVersion` mandatory for - `runtime-v4` packages, which is enforced at **package build time** (`scripts/validate.ts`) - and so never breaks an installed package. + The remaining gap is enforcing `minRuntimeVersion` for `runtime-v4` packages during + installation. Published packages already require it at **package build time** + (`scripts/validate.ts`), so this install-time check does not affect installed packages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/version-compatibility-strategy.md` around lines 293 - 297, Update the paragraph describing the future minRuntimeVersion tightening to state that build-time validation for runtime-v4 packages is already shipped, while the remaining gap is install-time validation for sideloaded packages. Keep the distinction consistent with the installation behavior described elsewhere in the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/version-compatibility-strategy.md`:
- Around line 413-418: Update the comparator count in the documentation to match
the listed symbols: either identify the third hand-rolled comparator alongside
isStrucppCompatibleRuntime and isUserManagementCapableRuntime, or change “Three”
to “Two” if only those two remain.
- Around line 175-182: Add the text language identifier to the fenced example
containing the API capability and version responses, changing its opening fence
to use text while leaving the example content unchanged.
---
Outside diff comments:
In `@docs/version-compatibility-strategy.md`:
- Around line 293-297: Update the paragraph describing the future
minRuntimeVersion tightening to state that build-time validation for runtime-v4
packages is already shipped, while the remaining gap is install-time validation
for sideloaded packages. Keep the distinction consistent with the installation
behavior described elsewhere in the document.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d62f3394-2e39-4508-b6e6-de0833f94cd9
📒 Files selected for processing (1)
docs/version-compatibility-strategy.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/editor/compiler/compiler-module.ts (1)
2672-2673: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate persisted Modbus data instead of asserting its shape.
vendorScreenDatacomes fromdevices/configuration.json, but these assertions provide no runtime validation. Invalidserialornetworkvalues can reach the shared Modbus define generator as a validVppModbusScreenStateand produce incorrect firmware configuration. Parse the project data asunknownand validate the complete Modbus state with a Zod schema or a type guard before assigning it.As per coding guidelines, files matching
src/**/*.{ts,tsx}must not use type assertions and must validate external data at boundaries with Zod schemas or type guards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/editor/compiler/compiler-module.ts` around lines 2672 - 2673, Replace the type assertions on vendorScreenData.serial and vendorScreenData.network in the Modbus state construction with runtime validation: treat persisted project data as unknown, validate the complete VppModbusScreenState using an existing or dedicated Zod schema/type guard, and only pass the validated result to the shared Modbus define generator. Remove the assertions while preserving valid-data behavior and rejecting invalid persisted values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/backend/editor/compiler/compiler-module.ts`:
- Around line 2672-2673: Replace the type assertions on vendorScreenData.serial
and vendorScreenData.network in the Modbus state construction with runtime
validation: treat persisted project data as unknown, validate the complete
VppModbusScreenState using an existing or dedicated Zod schema/type guard, and
only pass the validated result to the shared Modbus define generator. Remove the
assertions while preserving valid-data behavior and rejecting invalid persisted
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 166fd9f4-543c-4e86-9b58-2ef0c6f3a52c
📒 Files selected for processing (2)
src/backend/editor/compiler/compiler-module.tssrc/middleware/shared/ports/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/middleware/shared/ports/types.ts
…PE-448)
Review follow-up. Every change here removes a second way of doing
something that already had a first way.
Version parsing — one parser, not two
`parseVersionStrict` / `parseVersionLenient` kept the very split this
work set out to remove: the same string meant different things depending
on which one a caller reached for. `minEditorVersion: "4.3"` refused a
package install and was silently ignored by the runtime gate.
One `parseVersion` now applies one rule: a `v` prefix is decoration
(`v4.3.2` === `4.3.2`), a missing component is zero (`4.3` === `4.3.0`,
`4` === `4.0.0`), and anything else is UNKNOWN (null). Unknown never
becomes 0.0.0 behind a caller's back, so a gate can still tell "I cannot
read this" from "this is old" and fail closed on the first.
`isCompatibleEditorVersion` delegates to `isVersionAtLeast`, so the
install gate and the runtime gates cannot disagree; a table test asserts
that directly.
Consequence worth noting: the legacy `"v4"` header now parses as 4.0.0
instead of being rejected as junk. No gate's answer changes — 4.0.0 is
below the 4.1.0 floor either way — but it is refused for the honest
reason rather than because the string looked odd.
An unreadable floor no longer disappears
`PackageManifestSchema` rejects a `minEditorVersion` / `minRuntimeVersion`
it cannot parse. `"4.3"`, `"4"`, `"v5"` all pass, so only genuine junk
changes behaviour — and a sideloaded .vpp never reaches openplc-packages'
validator, which makes this schema the only boundary for that path. When
a *runtime* publishes an unreadable floor the probe now logs a warning:
the upload still proceeds, but a constraint the runtime believes it is
enforcing can no longer go missing in silence.
Comparators — a constant, not a hand-rolled comparison
`isStrucppCompatibleRuntime` and `isUserManagementCapableRuntime` each
open-coded their own `if (v.major > 4) ... return v.minor >= 1` next to
the constant they compare against. Both are now `isVersionAtLeast(raw,
<constant>)`. The old bodies hardcoded the *shape* of the constant:
raise MIN_RUNTIME_VERSION to a non-.0 patch and `minor >= 1` keeps
answering for the old floor while every behavioural test still passes.
Guard tests derive their expectations from the constants so a re-inlined
comparison fails immediately.
Board lookup — four copies became one
`board-info-resolver`, `handleVendorPluginPackaging`,
`buildVppArduinoModuleConfig` and `getRuntimeFloorForBoard` each grew
their own "first installed package whose devices contain this name"
loop. They agreed, which is the only reason this was latent: change the
tie-break or start matching on id as well as name and three of the four
keep the old behaviour, in a codebase where the symptom is a board
compiling against the wrong package's HAL.
`findVppDeviceByBoardName` in backend/shared is now the only
implementation. It lives there because board-info-resolver is a caller
and cannot reach into backend/editor, and it takes the PackageManagerPort
the resolver already injects, so editor and web both satisfy it with no
new plumbing. First-match-wins and skip-unreadable-manifest were implicit
in all four copies and are now stated and tested.
Message rendering
All three describe* builders route an unreadable version through
`formatVersionForDisplay`, so a blank renders as "unknown" instead of
leaving a hole in the sentence ("...reports ."). The two DOPE-448
builders had no tests at all; runtime-version-gate.ts went from 66% to
100% function coverage.
Also fixes the @deprecated tag on ParsedRuntimeVersion, which pointed at
a module that does not exist.
Verification: 5958 tests pass, 0 failures. 100% statements/branches/
functions/lines on all five touched shared files. validate:arch, tsc,
eslint and prettier clean. compare-surfaces.py reports match=true across
1011 files against the mirrored openplc-web commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§10 items 3 (unreadable floor discarded silently), 4 (manifest accepts a malformed floor) and 6 (hand-rolled comparators) were fixed in the same PR; strike them through with what actually landed rather than leaving a "still open" list that is no longer true. Item 6 said "three" and named two — there were two. Also records the sharper reason for 6 that only surfaced while fixing it: the hand-rolled bodies hardcoded the shape of the constant beside them, so raising a floor to a non-.0 patch would have left them answering for the old one with every test still green. §6's edge-case table gains the two rows the fix created — a partial floor (`"4.3"`) is enforced as `4.3.0` rather than being ignored, and an unreadable floor now warns — and drops the claim that no warning exists. §8 phase 1 records that the first cut shipped two parsers and why one was too many, plus the `"v4"` parsing change. Splits out item 7: `minRuntimeVersion` is still unenforced at install time for sideloaded packages. Items 5 and 7 remain open and need tickets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up — items 1, 2, 3, 4 and the two duplication findings are fixedPushed as Thanks @Gustavohsdp — 🟡1 and 🟡2 turned out to be two ends of the same defect, and chasing that produced a better fix than either item asked for. The thing both findings were pointing at
That is the exact bug this PR set out to remove, one level up: the parser got unified, but as a strict one and a lenient one chosen by name at the call site, so the divergence simply moved. A runtime team writing There is now exactly one
Unknown never becomes One behaviour change worth flagging in review: the legacy 🟡1 — the residual halfWhat is left genuinely unreadable is junk, and
The upload still proceeds. Refusing to talk to a device over a typo in its metadata would be worse than the mismatch the gate exists to catch — but it can no longer happen in silence, which was your point. 🟡2 — schema (also CodeRabbit's Major)
🟡4 — comparatorsBoth are now The reason to do it turned out sharper than duplication. The hand-rolled bodies hardcoded the shape of the constant beside them. Someone raises the floor, every behavioural test still passes (they were written against the old floor too), and 4.1.0 runtimes keep getting uploads. Guard tests now derive their expectations from the constants, plus a check that the two gates sit on different floors — 4.1.0 passes strucpp and fails user-management, which a single (Minor: the item said "three" and named two. There were two.) 🟢
|
| Site | Needed it for |
|---|---|
board-info-resolver.ts:260 |
board build info (backend/shared) |
compiler-module.ts:2021 |
VPP HAL packaging |
compiler-module.ts:2294 |
module config screens |
package-manager-module.ts:322 |
the new DOPE-448 runtime floor |
All four resolved a board identically, which is the only reason this was latent rather than open. The risk is the next edit: change the tie-break, add namespacing, start matching on id as well as name — and three of the four keep the old behaviour, in a codebase where the symptom is a board compiling against the wrong package's HAL.
One implementation now: backend/shared/hardware/find-vpp-device.ts. It lives in backend/shared because board-info-resolver is a caller and cannot reach into backend/editor, and it takes the PackageManagerPort the resolver already injects — so web satisfies it with its existing no-op stub, no new plumbing. Two behaviours that were implicit in all four copies are now stated and tested: first match wins in listInstalled() order, and an unreadable manifest is skipped rather than treated as empty, so one corrupt install cannot hide a board another package provides.
Net −45 lines across the two modules; grep for the loop returns one hit, the definition.
Messages, and the coverage hole underneath them
All three describe* builders route an unreadable version through formatVersionForDisplay, so a blank renders as unknown:
before: Board "SLM-RP4" requires OpenPLC Runtime v4.2.0 or newer. The runtime at 10.0.0.1 reports .
after: Board "SLM-RP4" requires OpenPLC Runtime v4.2.0 or newer. The runtime at 10.0.0.1 reports unknown.
describeIncompatibleRuntime three lines above already handled this correctly; the two DOPE-448 builders used ?? 'unknown', which '' slips straight past. It went unnoticed because they were the only exported functions in the file with no tests — pipeline.test.ts mocks them, so runtime-version-gate.ts sat at 66.66% function coverage. Now 100%.
CodeRabbit — status of each item
| Item | Status |
|---|---|
@deprecated → nonexistent shared/utils/version-compare |
✅ fixed |
| Manifest schema should require a strict version | ✅ fixed (as 🟡2) |
| §2 should read as historical / §10 hard-codes 404 | ✅ already fixed in 97a1239 before this round |
| §6 "future tightening" vs §8 "shipped" | ✅ fixed — split out as open item 7 |
| §10 "Three hand-rolled comparators" names two | ✅ fixed (item is now struck through) |
Number.isSafeInteger in the parser |
⏭️ not taken — needs a 16-digit version component; measured parseVersion("9007199254740993.0.0") → …992 |
| MD040 language identifiers on fences | ⏭️ not taken — no .markdownlint* config and no lint job in this repo, so nothing enforces it |
versionMustNotBeCalled should throw |
⏭️ not taken — the toEqual assertions already catch an unwanted fallback via the 'FALLBACK' sentinel |
as Record<string, unknown> in extractStringField |
⏭️ not taken — real convention violation, but no behavioural risk for the three literal field names in play |
as unknown as on the bridge stubs |
⏭️ not taken — the file already had 5 pre-existing instances; worth one cleanup pass, not in a compat PR |
compiler-module.ts:2672 vendorScreenData assertions |
⏭️ out of scope — pre-existing, outside this diff. Worth its own ticket |
Doc
§10 items 3, 4 and 6 struck through with what landed. §6's edge-case table gains the two rows the fix created (a partial floor is enforced as its zero-filled equivalent; an unreadable floor warns) and drops the "does not have one yet" claim. §8 phase 1 records that the first cut shipped two parsers and why one was too many.
Still open, both needing their own tickets:
- 5 — an installed-but-incompatible VPP keeps loading after an editor downgrade.
- 7 (new, split from CodeRabbit's §6 read) —
minRuntimeVersionis not enforced at install time for sideloaded packages. Lower priority than 5: the compile-time gate catches the mismatch before anything reaches a device.
One thing I looked at and am deliberately not changing
The /api/capabilities → /api/version fallback costs a legacy runtime more than one extra GET. makeRuntimeApiRequest is authenticated, and isTokenExpiredError treats any 401 as an expired token — so if the /<command> catch-all answers 401 (as this PR's own field note says it does), withAuth fires a full re-login and retries before falling back. That is GET → POST /api/login → GET → GET per upload, not one extra call.
@thiago-alves called it: transitional, and it shrinks to zero as the fleet picks up openplc-runtime#163. Recording it so nobody rediscovers it as a mystery. It is a redundant re-login, not a JWT invalidation — the old token is never revoked, just replaced in the editor's cache.
If anyone wants to settle it: log the status code of request 1 with a valid bearer token attached. If it is 404 rather than 401, no refresh fires and it really is a single extra GET. The original verification used curl, i.e. unauthenticated, which is a different request than the one the code makes.
Verification
Editor — 5958 tests, 0 failures. 100% statements/branches/functions/lines on all five touched shared files (runtime-version-gate.ts was 66.66% functions). validate:arch, tsc, eslint, prettier clean.
Web — 212 tests across the six touched suites, all green. vite build succeeds. validate:arch, eslint, prettier --check clean.
Mirror — compare-surfaces.py reports "match": true across 1011 files, 0 diffs (frontend 780 · middleware/shared 87 · backend/shared 104 · __architecture__ 1 · bare-metal-runtime 39). I checked the baseline before touching anything to be sure the repos already agreed. Six program files plus five test files crossed; the test copies were byte-identical at HEAD, so they were mirrored rather than left to drift.
ci-unit-tests runs jest --collectCoverage, and the 100% thresholds for src/backend/shared/ are already red on development (73% statements). Pre-existing, unrelated to this PR — these commits move the number up, not down — but the job will fail for the same reason it fails on the base branch. Please don't read it as a regression from this work.
Follow-up assisted by Claude Code.
…E-448) `Number.parseInt` answers a *finite* number for a 17-digit component and quietly rounds it: `"9007199254740993"` comes back as `…992`. The parser would then hand callers a version that no longer matches the string it came from, and `isVersionAtLeast` would compare against it. That breaks the only promise this parser makes — readable means exact, anything else is UNKNOWN. A component we can only approximate is not readable, so `parseVersion` now returns null for it, and such a version gets the same treatment as any other unreadable string: fails closed as a candidate, declares nothing as a floor. Unreachable with any real version string; taken because the rule is cheaper to state without an exception than with one. Raised by CodeRabbit on openplc-editor#993. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/version-compatibility-strategy.md`:
- Around line 456-458: Align the item numbering and statuses between the “Still
open” section, the preceding sideloaded minRuntimeVersion enforcement entry, and
this closing paragraph. Ensure the paragraph references the currently open items
and their correct ticket assignments, while retaining the already-fixed items
and their existing review/PR references.
In
`@src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts`:
- Around line 46-50: Update the Jest mock’s jest.requireActual call in
findDeviceByBoardName to supply FindVppDevice as its generic type parameter, and
remove the `as FindVppDevice` type assertion while preserving the existing
findVppDeviceByBoardName invocation.
In `@src/backend/shared/hardware/__tests__/find-vpp-device.test.ts`:
- Around line 5-12: Update the pkg and manifest test fixture helpers to remove
the double type assertions, replacing them with complete fixtures that conform
to InstalledPackage and PackageManifest or using satisfies for validation.
Preserve the existing fixture values and helper behavior while ensuring
TypeScript checks their contracts directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c2c1749-7dd6-46e8-a2d7-cbc123fd5241
📒 Files selected for processing (15)
docs/version-compatibility-strategy.mdsrc/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.tssrc/backend/editor/compiler/compiler-module.tssrc/backend/editor/package-manager/package-manager-module.tssrc/backend/shared/firmware/__tests__/runtime-version-gate.test.tssrc/backend/shared/firmware/runtime-version-gate.tssrc/backend/shared/hardware/__tests__/find-vpp-device.test.tssrc/backend/shared/hardware/board-info-resolver.tssrc/backend/shared/hardware/find-vpp-device.tssrc/backend/shared/library/__tests__/probe-runtime-version.test.tssrc/backend/shared/library/probe-runtime-version.tssrc/frontend/utils/__tests__/semver.test.tssrc/frontend/utils/semver.tssrc/middleware/shared/ports/__tests__/package-manifest-schema.test.tssrc/middleware/shared/ports/package-manifest-schema.ts
Re-review — all five items verified fixed; two things left before I approveWent through each item independently rather than taking the summary. Everything you claimed holds, and on two of them the fix is better than what I asked for. Verified, measured against the new code
And you're right about the count: I wrote "three hand-rolled comparators" and named two. There were two. 1. The 🟡2 fix has a second-order effect on the load path
Why this needs a decision. With Reachability is low and your own generosity is why: Suggested shape, following the same logic your install-gate comment already uses — strict at the boundary where the artefact enters, tolerant where we're just reading what's already here: at line 297, drop an unreadable floor and log it rather than rejecting the whole manifest. The install path keeps refusing it, which is where refusing belongs. 2. The current head has no CIWhen I reviewed yesterday this PR reported 12 green jobs — but those belong to the previous head. The three fix commits (20:55, 21:44, 21:48) have no build, lint, format or Note on the web PRMy approval on Autonomy-Logic/openplc-web#652 predates the two mirrored commits ( With the line-297 decision and CI on the current head, I'm happy to approve both. The fix pass here went past what was asked — including a fourth board-lookup duplicate my review never found. Review assisted by Claude Code. |
…OPE-448) The floor-format rule added to `PackageManifestSchema` guards the boundary where a package ENTERS the editor. The same schema is also used to read the `manifest.json` of an already-installed package, and there the rule had a second-order effect nobody wants: a package installed by an older editor, carrying a floor only genuine junk could produce, would stop parsing on load. `getInstalledPackageManifest` returns null, the boards it provides fall out of the board lookup, and they do so silently — on an upgrade where the user did nothing. Split the two: `parseInstalledPackageManifest` drops a floor this codebase cannot compare and logs it; `importFromFile` still refuses one outright. Dropping costs nothing that was not already lost, since an unreadable floor never gated anything (`isVersionAtLeast` treats it as "declares nothing"), and the log keeps the cause visible instead of trading one invisible outcome for another. Tolerance is scoped to the floors — a document that is not a manifest still rejects. Applied at both read sites: the main process (`getInstalledPackageManifest`) and the renderer adapter that re-validates what comes back over IPC. Raised in re-review of #993; recorded in §10 item 4 of the strategy doc. Mirrored byte-identically in openplc-web. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of #993 caught that the doc reads as a roadmap for work this PR already contains: §2.2 stated there is no endpoint where the runtime declares what it needs, and §8 listed phases 1-6 with day estimates. A maintainer opening this in three months could reasonably redo or revert what already exists. - Header states "implemented", lists the four shipping PRs, and tells the reader §2 is a dated snapshot of the state BEFORE this work, kept because the design rationale only makes sense against what it was fixing. - §8 becomes a landed-where table plus per-phase notes, no estimates. Cross-checking every claim against the code turned up three the doc asserted and the code does not do — all now corrected to describe what actually ships: - §4 claimed the VPP editor-floor check also runs "on load". It does not; it runs at install only, so a package installed under a newer editor keeps loading after a downgrade. Recorded as a known gap instead of a feature. - §6 promised a console warning for a runtime with no /api/capabilities. The fallback is deliberately silent, and the section now says why: that is every deployed device, so warning there would fire on every upload. - §6 promised a console warning when a runtime-v4 VPP declares no minRuntimeVersion. There is none; that case is caught at package build time by validate.ts instead. Also corrects 404 → 401 for the legacy-runtime fallback (the /<command> catch-all behind @jwt_required() swallows unknown paths), and turns §10 into decided-vs-open: the runtime's published floor is settled at 4.1.0, and the four findings from review are recorded with the note that three touch shared surface and need a mirrored commit in openplc-web. Docs only — no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 participants
Implements the agreed strategy for DOPE-448: three components on independent release cadences declare a minimum they need from the others, and the editor is the only component that compares.
Companion PRs — all four must land together:
minEditorVersionatGET /api/capabilitiesminRuntimeVersionin the manifest + validator ruleThe four comparisons
pipeline.ts, pre-uploadpipeline.ts, pre-uploadpackage-manager-module::installpipeline.ts, pre-uploadThree findings that motivated the shape
docs/package-format.md:69documented a guarantee that was not implemented. It promised the editor refuses to install a package requiring a newer editor. The only consumer was the catalog UI;install— the trust boundary that both the remote and the local "Add from file…" flow pass through, which already verifies the package signature — never looked at the field. A.vppfrom disk ignored it entirely.Two semver parsers disagreed on exactly the field inputs.
"v4"→4.0.0in one, rejected in the other; same for"4.1"and malformed strings. Neither was wrong for its own caller — a corrupt manifest should not break a catalog card, an unidentifiable runtime must not receive an upload — but the difference lived in two parsers where nothing named it. Now one parse, one ordering, and the lenient-vs-strict choice made by name at the call site.runtime-v4-pluginHAL is code that runs inside the runtime process, built against its plugin API, with no version negotiation at all. An older runtime loaded it and failed at scan time, on a live PLC.Backward compatibility
Both new gates are inert against everything currently deployed, in two independent ways: a runtime that declares no floor passes, and a caller that passes no
editorVersionpasses. A runtime predating/api/capabilitiesanswers 401 from the/<command>catch-all — not 404 — and falls back to/api/versionsilently, because that 401 is the normal answer from every device in the field.Verification
Manual, 5 scenarios each with a negative control, including a real Raspberry Pi on
v4.1.9:.vppabove the editor's version → install refused (this is the path that silently installed before)Automated:
5411tests, no new failures, coverage thresholds met,validate:archclean. Two pre-existingtscerrors ingraphical-editorand two pre-existing failing suites (locale-dependent) confirmed identical against a clean tree.Not in scope
Monotonic integer contracts, a
bundle-manifest.jsoninside the upload ZIP, an editor→runtime advertising handshake, andmax*bounds — all considered and dropped, with reasons, in §9 ofdocs/version-compatibility-strategy.md. §7 records the accepted limitation: the runtime advertises rather than enforces, so a client that skips the check can still upload. That is deliberate and is not a security control.Found while testing this: DOPE-539 — VPP signatures are verified only at startup, so installed package contents are mutable and trusted for the rest of the session. Pre-existing; filed to the backlog, not fixed here.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation