fix(compile): install vendor cores from the VPP's board manager URL - #1001
Conversation
A VPP that declares `target.boardManagerUrl` could not be compiled when
its core lives outside arduino-cli's built-in index. Selecting an
IndustrialShields board failed with:
Invalid argument passed: Platform 'industrialshields:esp32' not found
Arduino CLI process exited with code 7
The URL was resolved onto `BoardBuildInfo.boardManagerUrl` and then
dropped: `resolveBoardSelection` did not copy it onto `boardEntry`,
`InstallArduinoCoreArgs` had no field to carry it, and
`handleCoreInstallation` spawned `core install` without
`--additional-urls`. Vendor indexes shipped in VPPs were dead data.
It went unnoticed because every other VPP targets a core that is either
built into arduino-cli (arduino:avr, arduino:samd, arduino:mbed_edge) or
hardcoded in the editor's `ARDUINO_DATA` (esp32, STM32, rp2040, FACTS).
IndustrialShields is the first VPP core in neither list.
Changes:
- `InstallArduinoCoreArgs` gains `boardManagerUrl`, forwarded from
`boardEntry` by the pipeline and populated by `resolveBoardSelection`.
- `handleCoreInstallation` accepts the URL and passes `--additional-urls`
to `core install`.
- `handleCoreUpdateIndex` takes the URL too and is now actually called -
it was dead code. `core install --additional-urls` alone is not enough,
because the CLI resolves the platform against its cached index. The
refresh is best-effort: a network failure there must not mask the real
install error.
- The arduino-cli config was written with `{ flag: 'wx' }` and skipped on
EEXIST, so any URL added to `ARDUINO_DATA` never reached an existing
install. Missing URLs are now merged into the existing file, preserving
user-added entries and other settings.
Vendors now ship their board index in the VPP instead of needing a patch
to the editor's hardcoded list.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe change validates vendor Arduino board-manager URLs, reconciles them into Arduino CLI configuration, propagates them through board resolution and compilation, and uses them for index refresh and core installation. Tests cover valid and invalid URLs, propagation, pinned installs, and refresh failures. ChangesVendor board-manager support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant BoardSelection
participant CompilePipeline
participant EditorCompilerPlatformPort
participant CompilerModule
participant ArduinoCLI
BoardSelection->>CompilePipeline: Resolve board with boardManagerUrl
CompilePipeline->>EditorCompilerPlatformPort: Pass boardManagerUrl
EditorCompilerPlatformPort->>CompilerModule: Start core installation
CompilerModule->>ArduinoCLI: Refresh index with --additional-urls
CompilerModule->>ArduinoCLI: Install core with --additional-urls
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 3
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)
961-984: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject when the index-refresh process cannot start.
Line 961 creates a promise that only settles on
close. Ifspawnemitserror, such as for a missing binary or permission failure,handleCoreUpdateIndexstays pending. Thetry/catchat Line 1031 cannot continue to core installation.Add an
errorlistener that rejects the promise. Add a test that emitserror.Proposed fix
executeCommand.stderr?.on('data', (data: Buffer) => { stderrData += data.toString() }) + executeCommand.once('error', reject) executeCommand.on('close', (code) => {🤖 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 961 - 984, Update the promise in handleCoreUpdateIndex around executeCommand to listen for the child process error event and reject with that error, while preserving the existing close handling. Add or update a test for handleCoreUpdateIndex that emits an error from the spawned process and verifies the promise rejects so core installation can continue through the existing try/catch.
🤖 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 `@src/backend/editor/compiler/compiler-module.ts`:
- Around line 1029-1057: Validate device.target.boardManagerUrl during manifest
loading via PackageManifestSchema, accepting only HTTPS URLs whose path ends in
.json and rejecting local or file schemes and other invalid values. Ensure
invalid values cannot reach handleCoreUpdateIndex or the Arduino core install
command, while preserving the existing behavior for valid URLs.
In `@src/backend/editor/services/user-service/index.ts`:
- Around line 174-195: Update the Arduino CLI configuration update logic around
the additional_urls anchor to recognize valid inline empty sequences such as
`additional_urls: []`, then insert missing shipped URLs while preserving valid
YAML formatting. Keep the existing multiline handling unchanged, and add a
regression test covering an `additional_urls: []` configuration.
In `@src/backend/shared/compile/__tests__/resolve-board-selection.test.ts`:
- Around line 256-283: Remove the unnecessary as unknown as PackageManifest
assertion from the manifest fixture in
src/backend/shared/compile/__tests__/resolve-board-selection.test.ts:256-283 and
construct a type-compatible fixture directly. In
src/backend/editor/compiler/__tests__/handle-core-installation.test.ts:143-147,
replace unchecked assertion casts in the mocked module and fixture helpers with
properly typed mocks or object shapes, covering the referenced cast sites.
---
Outside diff comments:
In `@src/backend/editor/compiler/compiler-module.ts`:
- Around line 961-984: Update the promise in handleCoreUpdateIndex around
executeCommand to listen for the child process error event and reject with that
error, while preserving the existing close handling. Add or update a test for
handleCoreUpdateIndex that emits an error from the spawned process and verifies
the promise rejects so core installation can continue through the existing
try/catch.
🪄 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: 50a5a388-9025-44bd-8e9b-92d403dea99e
📒 Files selected for processing (9)
src/backend/editor/compiler/__tests__/handle-core-installation.test.tssrc/backend/editor/compiler/compiler-module.tssrc/backend/editor/compiler/editor-compiler-platform-port.tssrc/backend/editor/services/user-service/index.tssrc/backend/shared/compile/__tests__/pipeline.test.tssrc/backend/shared/compile/__tests__/resolve-board-selection.test.tssrc/backend/shared/compile/pipeline.tssrc/backend/shared/compile/steps/resolve-board-selection.tssrc/middleware/shared/ports/compiler-platform-port.ts
Review — the fix is right; one red CI check and two things to decideReviewed together with the web counterpart Autonomy-Logic/openplc-web#660 — the 5 shared files are byte-identical between the repos. Every finding below is on this (editor) side; the web PR carries none of its own. The diagnosis is precise and checkable: the URL was resolved onto The security question this kind of change demands, and the answer.
One ordering detail that's easy to get wrong and is right here: the "already installed and no pinned version" short-circuit returns before the index refresh, so an offline user with the core present still compiles without touching the network. Verification: 5/5 shared files byte-identical ( 🔴
|
Addresses the review on #1001. - **Format Check.** `prettier --write` on `user-service/index.ts`; the CI log named exactly that file and it was the one statement flagged. - **`boardManagerUrl` is now validated before it can reach the subprocess.** It rides through `PackageManifestSchema` on `.passthrough()` today, so it is typed in `types.ts` but entirely unchecked at runtime — and it becomes an `--additional-urls` argument to arduino-cli, which downloads a board package full of toolchain executables that later builds run. Signing a VPP vouches for the manifest, not for what the URL serves: arduino-cli's package checksums live INSIDE the index it fetches, so a plaintext index can be intercepted and the package that lands replaced. Requiring https closes that. Only the scheme is constrained — arduino-cli reads compressed indexes (.json.gz, .zip, .bz2) too, so pinning the path suffix would refuse valid vendors for no security gain. This follows the decision already recorded in this file for the version floors: a field a gate reads should not reach it as `unknown`. Strict where the artefact enters, tolerant where we only read what is already on disk — the same split the floors use, and for the same reason. A package installed before this constraint existed has its URL dropped with a warning rather than its whole manifest rejected, which would make every board it provides vanish from the board lookup on an upgrade the user never asked for. Dropping the field leaves it exactly as capable as it was before vendor indexes existed. openplc-packages carries the matching `"pattern": "^https://"`, so a package refused here cannot be built there either. - **Dropped an unnecessary `as unknown as PackageManifest`** in `resolve-board-selection.test.ts`. Verified genuinely redundant: the file typechecks without it. The remaining casts CodeRabbit flagged in `handle-core-installation.test.ts` are pre-existing jest scaffolding and that file's established fixture pattern; diverging from it there would make the file less consistent, not more. Tests: 13 new covering the accepted/refused URL shapes, the compressed-index case, and the drop-not-reject behaviour on the installed-read path. All 12 packages in openplc-packages still validate — every boardManagerUrl in the repo is already https. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review addressed — pushed in
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/middleware/shared/ports/package-manifest-schema.ts`:
- Line 214: Remove the unnecessary type assertion from value.devices in the
devices mapping expression, relying on the existing Array.isArray narrowing
while keeping device: unknown and the rest of the guard unchanged.
🪄 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: 2d25a377-57fd-469a-916e-fc6ed703102a
📒 Files selected for processing (4)
src/backend/editor/services/user-service/index.tssrc/backend/shared/compile/__tests__/resolve-board-selection.test.tssrc/middleware/shared/ports/__tests__/package-manifest-schema.test.tssrc/middleware/shared/ports/package-manifest-schema.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/editor/services/user-service/index.ts
- src/backend/shared/compile/tests/resolve-board-selection.test.ts
`Array.isArray(value.devices)` already makes `.map` available, and annotating the callback parameter as `unknown` is what preserves the runtime element checking — the `as unknown[]` cast added nothing. tsc and eslint are both clean without it. Raised by CodeRabbit on #1001. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/editor/compiler/compiler-module.ts (1)
1045-1058: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
#executeArduinoCliCommandfor the install spawn.
#executeArduinoCliCommandalready appends the.exesuffix on Windows and spawns the binary.handleCoreInstallationrepeats that logic at Lines 1045-1050. The same duplication exists inhandleCoreUpdateIndex,handleLibraryInstallation, andhandleLibraryUpdateIndex. Reuse of the private helper removes four copies of the platform check.♻️ Proposed refactor for the install spawn
- let binaryPath = this.arduinoCliBinaryPath - - if (CompilerModule.HOST_PLATFORM === 'win32') { - // INFO: On Windows, we need to add the .exe extension to the binary path. - binaryPath += '.exe' - } return new Promise<MethodsResult<string | Buffer>>((resolve, reject) => { - const executeCommand = spawn(binaryPath, [ + const executeCommand = this.#executeArduinoCliCommand([ 'core', 'install', coreRef, ...(boardManagerUrl ? ['--additional-urls', boardManagerUrl] : []), ...this.arduinoCliBaseParameters, ])🤖 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 1045 - 1058, Update handleCoreInstallation to invoke the existing `#executeArduinoCliCommand` helper for the core install arguments instead of constructing binaryPath and spawning directly. Apply the same reuse in handleCoreUpdateIndex, handleLibraryInstallation, and handleLibraryUpdateIndex, removing their duplicated Windows .exe handling while preserving each command’s existing arguments and result behavior.
🤖 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.
Nitpick comments:
In `@src/backend/editor/compiler/compiler-module.ts`:
- Around line 1045-1058: Update handleCoreInstallation to invoke the existing
`#executeArduinoCliCommand` helper for the core install arguments instead of
constructing binaryPath and spawning directly. Apply the same reuse in
handleCoreUpdateIndex, handleLibraryInstallation, and handleLibraryUpdateIndex,
removing their duplicated Windows .exe handling while preserving each command’s
existing arguments and result behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b6443c9e-ec15-470b-9bfd-c71d8672cb4a
📒 Files selected for processing (2)
src/backend/editor/compiler/compiler-module.tssrc/middleware/shared/ports/package-manifest-schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/middleware/shared/ports/package-manifest-schema.ts
Gustavohsdp
left a comment
Gustavohsdp
left a comment
There was a problem hiding this comment.
Approved
CI is green on both PRs and two of the three review items landed.
Fixed:
- Format Check — green (
326e97a3a). boardManagerUrlvalidation — done properly, and the docblock records the reasoning better than my comment did: "The signature on a VPP vouches for the manifest, not for what the URL serves. arduino-cli's package checksums live INSIDE the index it fetches", plus the note that this field goes further than a gate because it becomes a subprocess argument that downloads toolchain executables. 87 lines of new tests with it.
Still open — carried, not resolved:
additional_urls: []is not handled.user-service/index.ts:182still anchors on/^(\s*)additional_urls:\s*$/m, and there's noyamlimport. A config written asadditional_urls: []— which is whatarduino-cli config initproduces — still takes the warn-and-do-nothing path, so for those users the write-once config bug this PR also set out to fix stays as it was. It's not a regression and it doesn't block the IndustrialShields fix, which is why I'm not holding the approval for it;yamlis already a dependency here if someone wants to close it in a follow-up.
Everything I verified for the main fix still stands: the trust chain closes (REQUIRE_SIGNATURE refuses unsigned imports before any field is used; the URL is passed as an array argument, no shell), the already-installed short-circuit runs before the index refresh so offline builds keep working, and the shared surface is byte-identical across the two repos.
Merge the pair in short sequence — the sync gate in each repo only tolerates divergence while an open PR exists in the other.
Review assisted by Claude Code.
Resolves the expected conflict in `#checkIfArduinoCliConfigExists`, where #1001 (now on development) and this branch both rewrote the same method. Took this branch's version: `reconcileArduinoCliConfig` is a superset of the regex #1001 introduced — it backfills missing board-manager URLs *and* retires the obsolete `output.no_color`, using the `yaml` Document API rather than anchored regexes. That also settles the review finding on #1001 that the regex could not see `additional_urls: []`, the shape `arduino-cli config init` writes, along with the `existing.includes(url)` whole-file substring match. The doc comment is merged rather than replaced, so the board-manager-URL rationale from #1001 survives alongside the no_color one. Everything else auto-merged. #1001's boardManagerUrl schema validation and pipeline plumbing are intact; so is this branch's console work. Editor suite: 6386 passing, typecheck / lint / prettier clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Problem
Selecting an IndustrialShields board failed the build outright:
The VPP declares
target.boardManagerUrl, andBoardInfoResolveralreadyresolved it onto
BoardBuildInfo.boardManagerUrl— then it was dropped on thefloor.
resolveBoardSelectiondid not copy it ontoboardEntry,InstallArduinoCoreArgshad no field to carry it, andhandleCoreInstallationspawned
core installwithout--additional-urls. Every vendor board indexshipped in a VPP was dead data.
It went unnoticed because every other VPP targets a core that is either built
into arduino-cli (
arduino:avr,arduino:samd,arduino:mbed_edge) orhardcoded in the editor's
ARDUINO_DATAlist (esp32, STM32, rp2040, FACTS).IndustrialShields is the first VPP core in neither.
Changes
InstallArduinoCoreArgsgainsboardManagerUrl; the pipeline forwards itfrom
boardEntry, andresolveBoardSelectionpopulates it.handleCoreInstallationaccepts the URL and passes--additional-urlstocore install.handleCoreUpdateIndextakes the URL and is now actually called — it wasdead code.
core install --additional-urlsalone is not enough: the CLIresolves the platform against its cached index. The refresh is best-effort,
so a network blip there cannot mask the real install error.
{ flag: 'wx' }and skipped onEEXIST, making it effectively write-once — any URL added toARDUINO_DATAnever reached an existing install. Missing URLs are now merged into the
existing file, preserving user-added entries and other settings.
Vendors now ship their board index in the VPP instead of needing a patch to the
editor's hardcoded list.
Verification
Against the real bundled arduino-cli:
Confirmed end-to-end by the reporter: the IndustrialShields VPP now compiles.
Tests
7 new cases — index-refresh-before-install ordering,
--additional-urlswithand without a pinned core version, the no-URL regression case, refresh-failure
tolerance, plus pipeline and resolver forwarding. Suite goes 542 -> 549.
Checked they are not vacuous: reverting the two source hunks fails 3 of them.
Paired with openplc-web (shared-core parity).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests