Skip to content

fix: reject incompatible Managed Build architectures - #1502

Open
benceruleanlu wants to merge 1 commit into
mainfrom
bl/fix-managed-build-architecture
Open

benceruleanlu wants to merge 1 commit into
mainfrom
bl/fix-managed-build-architecture

Conversation

@benceruleanlu

Copy link
Copy Markdown
Member

Native ARM64 Desktop could offer a Managed Build with the same OS and GPU even though the archive contains x64 Python. This change includes process.arch in host resolution and checks it before treating an artifact as compatible. Catalog targets, default installs, explicit artifact selections, and version changes now reject incompatible architectures, including CPU fallbacks. Saved selections are checked again before an install retry releases terminals or changes the environment.

Builder's current API does not expose artifact architecture. This guard follows its verified build contract: Windows and Linux archives use x64 Python, and macOS is not currently a buildable target. See the curated interpreter images and buildable targets. Supporting native ARM64 Managed Builds will require Builder to publish matching artifacts with an architecture contract. The standalone runtime catalog is separate from this check.

Test coverage adds architecture rejection cases, verifies that catalog and all three artifact resolvers reject Windows/Linux ARM64, and checks that retrying an incompatible saved selection does not start downloads or modify the environment. Existing x64 GPU preference and CPU fallback tests continue to pass; fixtures now specify their host architecture explicitly.

Validation:

  • Focused Vitest run: 156 tests passed across 4 files (targets, builds, ComfyBuilder source, and dev-platform IPC handlers).
  • Required pre-commit checks: all four TypeScript projects, ESLint, and Prettier passed.
  • git diff --check passed.

Change breakdown (added + deleted lines; no merge-only changes):

Category Files Added Deleted Changed Share
Product code 4 28 7 35 19.77%
Test code 4 123 17 140 79.10%
Documentation (inline usage example) 1 1 1 2 1.13%
Total 9 152 25 177 100%

Product code paths:

  • src/main/comfybuilder/targets.ts (+17 / -3)
  • src/main/comfybuilder/types.ts (+1 / -0)
  • src/main/devplatform/builds.ts (+3 / -3)
  • src/main/sources/comfybuilder/index.ts (+7 / -1)

Test code paths:

  • src/main/comfybuilder/targets.test.ts (+44 / -12)
  • src/main/devplatform/builds.test.ts (+55 / -1)
  • src/main/lib/ipc/registerDevPlatformHandlers.test.ts (+2 / -2)
  • src/main/sources/comfybuilder/index.test.ts (+22 / -2)

Documentation path:

  • src/main/comfybuilder/index.ts (+1 / -1; updates the inline usage example only)

No configuration, generated files, lockfiles, or vendored code changes.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c27e1164-a7e0-4d30-a7a7-d24a16142ae5

📥 Commits

Reviewing files that changed from the base of the PR and between b9b50cd and fe18a9c.

📒 Files selected for processing (9)
  • src/main/comfybuilder/index.ts
  • src/main/comfybuilder/targets.test.ts
  • src/main/comfybuilder/targets.ts
  • src/main/comfybuilder/types.ts
  • src/main/devplatform/builds.test.ts
  • src/main/devplatform/builds.ts
  • src/main/lib/ipc/registerDevPlatformHandlers.test.ts
  • src/main/sources/comfybuilder/index.test.ts
  • src/main/sources/comfybuilder/index.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The change adds arch to host data, filters artifacts by OS architecture, reports process.arch, blocks incompatible platform resolutions, and validates artifacts before installation side effects occur.

Changes

Host Architecture Compatibility

Layer / File(s) Summary
Architecture-aware artifact selection
src/main/comfybuilder/types.ts, src/main/comfybuilder/targets.ts, src/main/comfybuilder/targets.test.ts, src/main/comfybuilder/index.ts
Host now requires arch. Artifact selection matches supported OS architectures before evaluating GPU compatibility. Tests cover supported and unverified architectures.
Host resolution and platform gating
src/main/devplatform/builds.ts, src/main/devplatform/builds.test.ts, src/main/lib/ipc/registerDevPlatformHandlers.test.ts
resolveHost returns process.arch. ARM64 Windows and Linux hosts produce no compatible artifacts and block managed build operations.
Installation compatibility validation
src/main/sources/comfybuilder/index.ts, src/main/sources/comfybuilder/index.test.ts
installEnvironment validates the artifact against the resolved host before releasing the install terminal or performing installation-related operations.

Sequence Diagram(s)

sequenceDiagram
  participant installEnvironment
  participant resolveHost
  participant selectArtifactForHost
  installEnvironment->>resolveHost: Resolve OS, architecture, and GPU
  installEnvironment->>selectArtifactForHost: Validate artifact against host
  selectArtifactForHost-->>installEnvironment: Return compatible artifact or null
  installEnvironment-->>installEnvironment: Reject incompatible artifact before side effects
Loading

Suggested reviewers: kosinkadink

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to fe18a

Managed Builds now reject unsupported host architectures during selection and before installation side effects, while preserving supported x64 Windows and Linux behavior. The change is ready to merge.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bl/fix-managed-build-architecture
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch bl/fix-managed-build-architecture

Comment @coderabbitai help to get the list of available commands.

@benceruleanlu
benceruleanlu marked this pull request as ready for review September 9, 2026 01:17
@coderabbitai
coderabbitai Bot requested a review from Kosinkadink September 9, 2026 01:18

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants