task: unify release hosts, native runtimes, and product bundles - #1106
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR moves releases to a backend-neutral host, separately packaged native runtimes, and composed product bundles. It adds product manifests, dependency validation, runtime discovery and bundle precedence, transactional installer support, conditional startup initialization, and dynamically loaded feature-gated Metal support. ChangesRelease artifact contract
Host, runtime, and product build pipeline
Product schema and installer composition
Runtime discovery and startup integration
Feature-gated dynamic Metal loading
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseTarget
participant HostBuild
participant RuntimePackaging
participant ProductComposer
participant Installer
ReleaseTarget->>HostBuild: build backend-neutral host
ReleaseTarget->>RuntimePackaging: package selected native runtime
RuntimePackaging-->>ProductComposer: runtime manifest and files
HostBuild-->>ProductComposer: host binary and imports report
ProductComposer-->>Installer: product bundle and product-manifest.json
Installer->>Installer: validate and install native-runtimes tree
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
ndizazzo
force-pushed
the
codex/build-unification-foundation
branch
from
July 29, 2026 00:54
5a7c663 to
fe6ad29
Compare
ndizazzo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-runtime-install/src/lib.rs (1)
258-277: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCopy explicit
--bundle-dirruntime installs into the cache.
NativeRuntimeSource::BundlereturnsAlreadyInstalledand storesruntime.pathas the supplied bundle directory, which is fine for executable-adjacent discovery paths but not for user-supplied--bundle-dirinstalls. If a user installs from/tmp/extracted-runtime, the next resolution can fail after that user-owned path is removed. UseAlreadyInstalledonly for bundles discovered next to the product binary; for explicit CLI SDK paths, copy/move the runtime into the native runtime cache and reportInstalled, while preserving the in-place behavior only for product-adjacent bundles.🤖 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 `@crates/mesh-llm-runtime-install/src/lib.rs` around lines 258 - 277, Update the NativeRuntimeSource::Bundle handling to distinguish product-adjacent discovered bundles from explicit --bundle-dir SDK paths. Preserve the current in-place AlreadyInstalled result only for product-adjacent bundles; for explicit CLI paths, copy or move the runtime into the native runtime cache, set the cached path on the returned runtime, and report NativeRuntimeInstallStatus::Installed so later resolution does not depend on the original directory.
🤖 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 `@AGENTS.md`:
- Around line 89-94: Update the release and shipping guidance near the
host-binary instructions to stop directing users to copy only
target/release/mesh-llm. Direct them to run just release-bundle and use the
resulting composed bundle containing the backend-neutral host plus exactly one
selected native runtime.
In `@build-unification.md`:
- Line 75: Update the bundle-layout fenced code block in build-unification.md to
declare the text language by changing its opening fence to ```text, while
preserving the tree content and closing fence.
In `@CONTRIBUTING.md`:
- Around line 27-31: Update the prerequisites in CONTRIBUTING.md to require the
package manager used by the `just build` workflow, aligning them with the npm
commands documented in README and AGENTS.md. Remove or replace the conflicting
pnpm requirement so fresh contributors install npm-based dependencies correctly.
In `@docs/design/NATIVE_RUNTIMES.md`:
- Around line 103-105: Update the path-searching behavior statement in
NATIVE_RUNTIMES to say the current working directory is not searched unless
explicitly supplied through the API or CLI directory configuration; preserve the
surrounding runtime-loading behavior.
In `@install.ps1`:
- Around line 261-264: Update the installation upgrade flow around
$hostImportsSource to remove the existing destination host-imports.json before
conditionally copying the bundled file. Preserve the current copy behavior when
the source exists, ensuring upgrades from bundles without the file do not retain
stale host metadata.
- Around line 255-260: Update the installation flow around $installedRuntimes so
$runtimeSource is first copied and validated in a temporary staging directory,
without deleting the existing runtime. Replace the installed runtime only after
staging succeeds, and remove the staging directory or restore the previous
runtime if replacement or the related manifest copy fails, preserving a usable
installation on errors.
- Around line 249-251: Strengthen validation around the runtimeSource and
productManifestSource checks before any existing-installation mutation: confirm
the runtime source is a valid, non-empty directory with the expected composed
layout, parse and validate the product-v2 manifest, and require all digests
needed for installation while keeping checksum sidecars optional. Ensure these
validations complete before the code removes or replaces the current
installation.
In `@install.sh`:
- Around line 615-618: Update validate_bundle so the product-manifest.json and
native-runtimes validation is enforced only for resolved release versions/tags
that use the newer bundle contract. Preserve installation of older pinned
releases by skipping this check for pre-contract versions, using the existing
resolved version/tag information rather than making the installer
contract-v2-only.
In `@Justfile`:
- Around line 152-160: Update the target_args handling in the
package-native-runtime command so the no-target path works under macOS Bash 3.2
with set -u enabled. Avoid expanding an empty target_args array, while
preserving the existing --target argument behavior when target is provided and
still passing --build and --backend consistently.
In `@schemas/product-v2.schema.json`:
- Around line 50-76: Constrain the path properties in the host and runtime
artifact schemas to safe relative bundle paths: reject absolute paths and
parent-directory traversal, and require runtime_artifact.path to remain under
native-runtimes/. Update the path validation near the existing host and runtime
path definitions while preserving their non-empty string requirements.
In `@scripts/compose-product-bundle.py`:
- Around line 38-50: Update the runtime validation in the composition flow
around runtime_data and runtime_id to verify that the selected runtime’s
backend/variant matches the --backend argument using the same mapping as runtime
selection. Reject mismatches before building the returned manifest, and preserve
the requirement that exactly one native runtime is selected.
In `@scripts/package-release.ps1`:
- Around line 327-342: Update the runtime-selection invocation in the Windows
packaging flow to build argument tokens and append --cuda-major only when
$cudaMajor has a value, preventing an empty native argument in PowerShell 5.1.
Capture the selector output by trimming it and retaining only its last line
before assigning $runtimeDir, while preserving the existing failure check.
---
Outside diff comments:
In `@crates/mesh-llm-runtime-install/src/lib.rs`:
- Around line 258-277: Update the NativeRuntimeSource::Bundle handling to
distinguish product-adjacent discovered bundles from explicit --bundle-dir SDK
paths. Preserve the current in-place AlreadyInstalled result only for
product-adjacent bundles; for explicit CLI paths, copy or move the runtime into
the native runtime cache, set the cached path on the returned runtime, and
report NativeRuntimeInstallStatus::Installed so later resolution does not depend
on the original directory.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97154b15-0d24-40ff-9d95-0872d98578ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.md.github/workflows/release.ymlAGENTS.mdCONTRIBUTING.mdJustfileREADME.mdRELEASE.mdbuild-unification.mdci/ci.mdcrates/mesh-llm-cli/src/runtime.rscrates/mesh-llm-commands/src/runtime_native.rscrates/mesh-llm-commands/src/runtime_native/formatters.rscrates/mesh-llm-gpu-bench/Cargo.tomlcrates/mesh-llm-gpu-bench/build.rscrates/mesh-llm-gpu-bench/native/metal/membench_metal.mcrates/mesh-llm-gpu-bench/src/lib.rscrates/mesh-llm-gpu-bench/src/runner.rscrates/mesh-llm-host-runtime/Cargo.tomlcrates/mesh-llm-host-runtime/src/exact_test_wrappers.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-native-runtime/src/resolver.rscrates/mesh-llm-nodejs/src/lib.rscrates/mesh-llm-runtime-install/src/discovery.rscrates/mesh-llm-runtime-install/src/lib.rscrates/mesh-llm-system/Cargo.tomlcrates/mesh-llm-system/src/hardware/mod.rscrates/mesh-llm/Cargo.tomlcrates/mesh-llm/src/lib.rsdocs/README.mddocs/cuda-release-lanes.mddocs/design/NATIVE_RUNTIMES.mddocs/design/TESTING.mddocs/specs/mesh-setup-installer.mdinstall.ps1install.shschemas/product-v2.schema.jsonscripts/build-release.shscripts/build-windows.ps1scripts/compose-product-bundle.pyscripts/package-native-runtime.shscripts/package-release.ps1scripts/package-release.shscripts/select-native-runtime.pyscripts/tests/test_build_release.pyscripts/tests/test_install_ps1.pyscripts/tests/test_install_sh.pyscripts/tests/test_package_release.pyscripts/tests/test_verify_host_dependencies.pyscripts/verify-host-dependencies.py
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-commands/src/runtime_native.rs (1)
109-109: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
discover_local_native_runtimesfailure aborts the whole listing/doctor command.Both call sites now source installed runtimes via
discover_local_native_runtimes, which (percrates/mesh-llm-runtime-install/src/discovery.rs) does reading a native runtime manifest from disk validates the manifest shape and then verifies the manifest's declared contents (including checksums) against the on-disk bundle for every discovered bundle directory, propagating any error with?. Previouslyrun_native_runtime_list/run_native_runtime_doctoronly consultedcache.installed()(always well-formed, sincewrite_to_dirauto-populates checksums). Now they also scan env-var/adjacent/explicit bundle dirs, so a single stale, legacy (pre-checksum), or tampered bundle anywhere in that discovery set will hard-failmesh-llm runtime list/mesh-llm runtime doctorentirely instead of just being skipped, even though other runtimes (including cache-managed ones) are perfectly usable.Consider having
discover_local_native_runtimestolerate per-bundle manifest read/verify failures (e.g., log a warning and skip) rather than letting one bad bundle dir take down the whole discovery pass.Also applies to: 319-319
🤖 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 `@crates/mesh-llm-commands/src/runtime_native.rs` at line 109, Update discover_local_native_runtimes so manifest read, validation, or bundle-content verification failures for an individual discovered directory are logged as warnings and skipped rather than propagated with ?. Preserve successful runtime discovery and allow the overall list/doctor commands in run_native_runtime_list and run_native_runtime_doctor to continue processing other bundle directories.
🧹 Nitpick comments (6)
scripts/tests/test_package_native_runtime.py (1)
14-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
cuda-blackwellbranch.
cuda_toolkit_majordefaults to13forcuda-blackwellandbackend_flavorappends-sm120; neither is exercised.💚 Proposed test
def test_cuda_flavor_defaults_to_cuda_12(self) -> None: self.assertEqual(self.backend_flavor("cuda"), "cuda12") + + def test_blackwell_flavor_defaults_to_cuda_13_sm120(self) -> None: + self.assertEqual(self.backend_flavor("cuda-blackwell"), "cuda13-sm120")🤖 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 `@scripts/tests/test_package_native_runtime.py` around lines 14 - 31, Add test coverage for the cuda-blackwell path in backend_flavor, verifying it defaults to toolkit major 13 and produces the cuda13-sm120 flavor. Keep the existing CUDA default and explicit-toolkit tests unchanged.scripts/tests/test_package_release.py (1)
142-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the recorded backend, not just the exit code.
Both alias tests only check
returncode == 0; they would still pass if the manifest recorded the wrongbackendor runtime id. Add an assertion onproduct-manifest.jsoncontents.💚 Proposed test tightening
self.assertEqual(result.returncode, 0, result.stderr) + manifest = json.loads( + (bundle / "product-manifest.json").read_text(encoding="utf-8") + ) + self.assertEqual(manifest["backend"], "cuda-blackwell") + self.assertEqual( + manifest["runtime"]["path"], "native-runtimes/linux-cuda13-sm120" + )🤖 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 `@scripts/tests/test_package_release.py` around lines 142 - 185, Strengthen both test_product_manifest_accepts_cuda_blackwell_backend_alias and test_product_manifest_accepts_hip_backend_alias by reading product-manifest.json after the command succeeds and asserting its recorded backend and runtime identifier match the expected alias and runtime. Keep the existing return-code assertions.scripts/package-native-runtime.sh (1)
145-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
MESH_LLM_CUDA_TOOLKIT_MAJORis numeric.The value is passed through verbatim into the flavor string and interpolated into the embedded Python as
int("$cuda_major")(line 488), so a malformed override (12.1,cuda12) produces a bad artifact id and a lateValueErrorin manifest generation instead of an early, clear failure.♻️ Proposed refactor
cuda_toolkit_major() { if [[ -n "${MESH_LLM_CUDA_TOOLKIT_MAJOR:-}" ]]; then + if [[ ! "$MESH_LLM_CUDA_TOOLKIT_MAJOR" =~ ^[0-9]+$ ]]; then + echo "MESH_LLM_CUDA_TOOLKIT_MAJOR must be an integer, got '$MESH_LLM_CUDA_TOOLKIT_MAJOR'" >&2 + return 1 + fi printf '%s\n' "$MESH_LLM_CUDA_TOOLKIT_MAJOR" return 0 fi🤖 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 `@scripts/package-native-runtime.sh` around lines 145 - 159, Validate MESH_LLM_CUDA_TOOLKIT_MAJOR in cuda_toolkit_major before printing it or using it in downstream flavor and manifest generation. Accept only a numeric major version, and fail immediately with a clear error for values such as 12.1 or cuda12; preserve the existing MESH_CUDA_VERSION and backend fallback behavior.install.sh (1)
639-645: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the staging
mktempand clean up temp dirs on interrupt.
backup_dircreation is failure-checked butstaging_diron Line 641 is not; more importantly, both scratch dirs live insideINSTALL_DIR, so aSIGINT/SIGTERMduring the move loop leaves.mesh-llm-stage.*/.mesh-llm-backup.*directories permanently in the user's bin directory.♻️ Suggested hardening
mkdir -p "$INSTALL_DIR" local staging_dir local backup_dir - staging_dir="$(mktemp -d "$INSTALL_DIR/.mesh-llm-stage.XXXXXX")" + if ! staging_dir="$(mktemp -d "$INSTALL_DIR/.mesh-llm-stage.XXXXXX")"; then + return 1 + fi if ! backup_dir="$(mktemp -d "$INSTALL_DIR/.mesh-llm-backup.XXXXXX")"; then rm -rf -- "$staging_dir" return 1 fi + trap 'rm -rf -- "$staging_dir" "$backup_dir"' INT TERMReset the trap (
trap - INT TERM) before returning frominstall_bundle.🤖 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 `@install.sh` around lines 639 - 645, Update install_bundle’s temporary-directory setup to failure-check staging_dir creation and remove it before returning if creation fails. Add SIGINT/SIGTERM cleanup for both staging_dir and backup_dir during the move loop, and reset the interrupt/termination trap with `trap - INT TERM` before install_bundle returns so the caller’s trap state is not retained.crates/mesh-llm-host-runtime/src/system/native_runtime.rs (1)
201-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog message is now inaccurate on the bundle path.
When
discovered_bundle_dirsis non-empty the cache lookup is skipped entirely, yet the log still asserts "No compatible installed MeshLLM native runtime found". Distinguish the two reasons so operators can tell a genuine cache miss from bundle-precedence.♻️ Suggested tweak
tracing::info!( cache_root = %cache.root().display(), mesh_version = %options.mesh_version, - "No compatible installed MeshLLM native runtime found; attempting one-shot startup install" + bundle_dirs = ?options.bundle_dirs, + "Resolving MeshLLM native runtime via one-shot startup install (locally packaged runtimes take precedence over the cache)" );🤖 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 `@crates/mesh-llm-host-runtime/src/system/native_runtime.rs` around lines 201 - 224, Update the startup-install log in the flow around discover_native_runtime_bundle_dirs and resolve_installed_native_runtime_plan to distinguish an empty discovered_bundle_dirs cache miss from the non-empty bundle-precedence path. Report that no compatible installed runtime was found only for the empty-bundle case; use a message indicating discovered bundles take precedence when bundle directories are present.crates/mesh-llm-commands/src/runtime_native.rs (1)
136-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test asserting the explicit-install policy override.
run_native_runtime_installnow force-overridesbundle_install_policytoInstallExplicitBundlesIntoCache, diverging from theUseInPlacedefault exercised by the setup-helper tests inruntime_native/setup_helpers.rs. No test here pins this CLI-specific override, so a future refactor could silently drop it.🤖 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 `@crates/mesh-llm-commands/src/runtime_native.rs` around lines 136 - 145, Add a focused test for run_native_runtime_install that verifies the install options passed to the runtime use NativeRuntimeBundleInstallPolicy::InstallExplicitBundlesIntoCache, rather than the UseInPlace default produced by native_runtime_install_options. Keep the assertion scoped to this CLI-specific override.
🤖 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 `@crates/mesh-llm-runtime-install/src/lib.rs`:
- Around line 313-338: Update bundle_path_matches_explicit_root so an explicit
directory that fails canonicalization is skipped rather than propagated as an
error. Preserve successful canonicalization and starts_with matching for valid
entries, allowing the function to continue checking all remaining explicit_dirs
and return false when none match.
In `@install.ps1`:
- Around line 381-386: Rename the local variable assigned from the manifest’s
“host” property in Assert-ProductBundle to avoid PowerShell’s read-only
automatic $host variable, and update all subsequent references in the host.path
and host.sha256 validation flow to use the new name.
- Around line 556-560: Move Remove-StaleBinaries out of the try block that calls
Remove-InstallBackups, so failures during stale-binary cleanup occur after the
installation transaction is committed and do not trigger Restore-InstallBackup.
Keep Restore-InstallBackup handling failures only through the
backup-removal/commit steps.
- Around line 491-497: Update the runtime staging logic around
$Bundle.RuntimeSource and $Paths.RuntimeStaging so the copied runtime is placed
under its matching $runtimeId directory within the incoming staging area. Ensure
the subsequent move preserves the native-runtimes/${runtimeId}/manifest.json
hierarchy instead of flattening manifest.json directly under native-runtimes.
- Around line 282-302: Update Get-DeterministicTreeSha256 to derive each file’s
relative POSIX-normalized path before sorting, and order those paths with
ordinal comparison. Use the sorted normalized paths to retrieve each file and
preserve the existing hashing inputs and digest sequence.
---
Outside diff comments:
In `@crates/mesh-llm-commands/src/runtime_native.rs`:
- Line 109: Update discover_local_native_runtimes so manifest read, validation,
or bundle-content verification failures for an individual discovered directory
are logged as warnings and skipped rather than propagated with ?. Preserve
successful runtime discovery and allow the overall list/doctor commands in
run_native_runtime_list and run_native_runtime_doctor to continue processing
other bundle directories.
---
Nitpick comments:
In `@crates/mesh-llm-commands/src/runtime_native.rs`:
- Around line 136-145: Add a focused test for run_native_runtime_install that
verifies the install options passed to the runtime use
NativeRuntimeBundleInstallPolicy::InstallExplicitBundlesIntoCache, rather than
the UseInPlace default produced by native_runtime_install_options. Keep the
assertion scoped to this CLI-specific override.
In `@crates/mesh-llm-host-runtime/src/system/native_runtime.rs`:
- Around line 201-224: Update the startup-install log in the flow around
discover_native_runtime_bundle_dirs and resolve_installed_native_runtime_plan to
distinguish an empty discovered_bundle_dirs cache miss from the non-empty
bundle-precedence path. Report that no compatible installed runtime was found
only for the empty-bundle case; use a message indicating discovered bundles take
precedence when bundle directories are present.
In `@install.sh`:
- Around line 639-645: Update install_bundle’s temporary-directory setup to
failure-check staging_dir creation and remove it before returning if creation
fails. Add SIGINT/SIGTERM cleanup for both staging_dir and backup_dir during the
move loop, and reset the interrupt/termination trap with `trap - INT TERM`
before install_bundle returns so the caller’s trap state is not retained.
In `@scripts/package-native-runtime.sh`:
- Around line 145-159: Validate MESH_LLM_CUDA_TOOLKIT_MAJOR in
cuda_toolkit_major before printing it or using it in downstream flavor and
manifest generation. Accept only a numeric major version, and fail immediately
with a clear error for values such as 12.1 or cuda12; preserve the existing
MESH_CUDA_VERSION and backend fallback behavior.
In `@scripts/tests/test_package_native_runtime.py`:
- Around line 14-31: Add test coverage for the cuda-blackwell path in
backend_flavor, verifying it defaults to toolkit major 13 and produces the
cuda13-sm120 flavor. Keep the existing CUDA default and explicit-toolkit tests
unchanged.
In `@scripts/tests/test_package_release.py`:
- Around line 142-185: Strengthen both
test_product_manifest_accepts_cuda_blackwell_backend_alias and
test_product_manifest_accepts_hip_backend_alias by reading product-manifest.json
after the command succeeds and asserting its recorded backend and runtime
identifier match the expected alias and runtime. Keep the existing return-code
assertions.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2081815a-621a-4b61-8529-fa009aec829a
📒 Files selected for processing (33)
AGENTS.mdCONTRIBUTING.mdJustfilecrates/mesh-llm-commands/src/runtime_native.rscrates/mesh-llm-commands/src/runtime_native/setup_helpers.rscrates/mesh-llm-ffi/src/lib.rscrates/mesh-llm-hardware-profile/src/lib.rscrates/mesh-llm-host-runtime/src/sdk/embedded_startup.rscrates/mesh-llm-host-runtime/src/system/native_runtime.rscrates/mesh-llm-native-runtime/src/cache.rscrates/mesh-llm-native-runtime/src/manifest.rscrates/mesh-llm-native-runtime/src/resolver.rscrates/mesh-llm-nodejs/src/lib.rscrates/mesh-llm-runtime-install/src/discovery.rscrates/mesh-llm-runtime-install/src/lib.rsdocs/design/NATIVE_RUNTIMES.mdinstall.ps1install.shschemas/product-v2.schema.jsonscripts/build-windows.ps1scripts/compose-product-bundle.pyscripts/package-native-runtime.shscripts/package-release.ps1scripts/tests/test_build_windows.pyscripts/tests/test_install_ps1.pyscripts/tests/test_install_sh.pyscripts/tests/test_justfile_release_runtime.pyscripts/tests/test_package_native_runtime.pyscripts/tests/test_package_release.pyscripts/tests/test_package_release_ps1.pyscripts/tests/test_verify_host_dependencies.pyscripts/verify-host-dependencies.pywebsite/src/docs/pages/CLI.md
🚧 Files skipped from review as they are similar to previous changes (11)
- CONTRIBUTING.md
- crates/mesh-llm-nodejs/src/lib.rs
- scripts/build-windows.ps1
- scripts/tests/test_verify_host_dependencies.py
- scripts/package-release.ps1
- scripts/compose-product-bundle.py
- crates/mesh-llm-runtime-install/src/discovery.rs
- crates/mesh-llm-native-runtime/src/resolver.rs
- scripts/verify-host-dependencies.py
- AGENTS.md
- docs/design/NATIVE_RUNTIMES.md
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
install.sh (1)
676-730: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep stale-binary cleanup reversible until commit.
remove_stale_binariesdirectly deletes existing files, while the rollback only restores backed-up overwritten targets. An interrupt or cleanup failure after staging moves can therefore roll back the new install but permanently remove part of the prior one. Move stale entries into the same backup directory and delete that directory only after cleanup succeeds.🤖 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 `@install.sh` around lines 676 - 730, The install_bundle flow currently makes remove_stale_binaries deletions irreversible before the installation is committed. Update remove_stale_binaries and its call in install_bundle to move stale entries into the existing backup_dir, perform cleanup, and only remove backup_dir after cleanup succeeds; ensure rollback restores both overwritten and stale entries on interruption or failure.docs/design/TESTING.md (1)
1161-1161: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate section number
16.Line 525 already uses
### 16. Mesh ID generation (originator). Renumber this new section to keep the checklist references unambiguous.🤖 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/design/TESTING.md` at line 1161, Renumber the new “Release host/runtime/product boundary” section heading so it no longer duplicates section 16, while preserving the existing “Mesh ID generation (originator)” numbering and keeping subsequent checklist references unambiguous.
🧹 Nitpick comments (5)
scripts/tests/test_install_sh.py (1)
274-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two tests are silently coupled to
COMPOSED_PRODUCT_MIN_VERSION.
0.74.0must stay below the floor and0.75.0at/above it; when the floor moves,test_install_bundle_accepts_complete_legacy_archivestarts failing for a non-obvious reason. Consider deriving both versions from the constant sourced out ofinstall.sh, or at least add a comment stating the coupling.🤖 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 `@scripts/tests/test_install_sh.py` around lines 274 - 340, The tests test_install_bundle_accepts_complete_legacy_archive and test_install_bundle_rejects_post_contract_legacy_shape hard-code versions around COMPOSED_PRODUCT_MIN_VERSION. Derive the below-floor and at/above-floor version values from the install.sh constant, or document the intentional coupling clearly so the tests remain aligned when the threshold changes.scripts/ci-smoke-test.sh (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFive CI scripts copy the same packaged-runtime resolution block. The root cause is that there's no shared helper for "resolve
native-runtimesbeside$MESH_LLM, fail if absent, exportMESH_LLM_NATIVE_RUNTIME_BUNDLE_DIR". Extracting it into a sourced helper (e.g.scripts/lib/native-runtime-bundle.sh) keeps the discovery contract consumed bydiscover_native_runtime_bundle_dirsin one place as it evolves.
scripts/ci-smoke-test.sh#L71-L76: replace the block with a call to the shared helper.scripts/ci-compat-smoke.sh#L57-L62: replace the block with a call to the shared helper.scripts/ci-two-node-split-smoke.sh#L52-L57: replace the block with a call to the shared helper.scripts/ci-two-node-client-serving-smoke.sh#L46-L51: replace the block with a call to the shared helper.scripts/ci-client-auto-test.sh#L38-L43: replace the block with a call to the shared helper.🤖 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 `@scripts/ci-smoke-test.sh` around lines 71 - 76, Extract the shared native-runtime resolution and export logic into a sourced helper such as scripts/lib/native-runtime-bundle.sh, preserving the MESH_LLM-relative fallback and missing-directory failure behavior. Replace the duplicated blocks in scripts/ci-smoke-test.sh#L71-L76, scripts/ci-compat-smoke.sh#L57-L62, scripts/ci-two-node-split-smoke.sh#L52-L57, scripts/ci-two-node-client-serving-smoke.sh#L46-L51, and scripts/ci-client-auto-test.sh#L38-L43 with calls to that helper so discover_native_runtime_bundle_dirs consumes one shared contract..github/workflows/release.yml (1)
1218-1230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale build-only env on a now compose-only job.
build_windows_gpuno longer builds backends (it downloads host + runtime artifacts), soROCM_HIP_SDK_FILENAMEandWINDOWS_CUDA_VERSION(plus the CUDA pin comment) are dead configuration here. Consider dropping them to avoid implying SDK setup still happens in this job.🤖 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 @.github/workflows/release.yml around lines 1218 - 1230, Remove the unused ROCM_HIP_SDK_FILENAME and WINDOWS_CUDA_VERSION environment variables, along with the related CUDA pin comment, from the build_windows_gpu job. Leave the job’s artifact composition configuration and other environment settings unchanged.tools/xtask/src/workflow_checks.rs (1)
580-609: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the contract assertions to the Windows job sections.
The first loop correctly checks
ci_windows_cpu/ci_windows_gpu, but this loop checks the entireci_workflow, so any occurrence elsewhere inci.ymlsatisfies the Windows host-build / runtime-packaging / readiness-smoke invariants. Using the already-extracted sections makes the check enforce what the message claims.♻️ Proposed change
- for (workflow, context) in [ - (ci_workflow, "main CI"), - (pr_windows_targets, "PR Builds Windows targets"), - ] { + for (workflow, context) in [ + (ci_windows_cpu, "main CI Windows CPU"), + (ci_windows_gpu, "main CI Windows GPU"), + (pr_windows_targets, "PR Builds Windows targets"), + ] {Note the CPU section currently packages the runtime without a
\-continuation, so the"scripts/package-native-runtime.sh \\"needle would need loosening to"scripts/package-native-runtime.sh".🤖 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 `@tools/xtask/src/workflow_checks.rs` around lines 580 - 609, Update the second workflow-check loop to validate the extracted Windows job sections, ci_windows_cpu and ci_windows_gpu, instead of the entire ci_workflow. Preserve the existing assertions and context labels, but loosen the package-runtime needle from the backslash-continued form to scripts/package-native-runtime.sh so the CPU section is matched correctly.Justfile (1)
143-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate
build-runtimeto the existing primitive
build-linuxalready handles the backend-native runtime build logic with a bash shebang, whilebuild-runtimerepeats the same backend defaulting and uses[[ ]], a bashism that breaks if the recipe shell is not bash.♻️ Delegate to the existing primitive
[linux] -build-runtime backend="" cuda_arch="" rocm_arch="": - `@backend`="{{ backend }}"; \ - [[ -n "$$backend" ]] || backend=cpu; \ - LLAMA_STAGE_CUDA_ARCHITECTURES="{{ cuda_arch }}" LLAMA_STAGE_AMDGPU_TARGETS="{{ rocm_arch }}" scripts/package-native-runtime.sh --build --backend "$$backend" +build-runtime backend="" cuda_arch="" rocm_arch="": (build-linux backend cuda_arch rocm_arch)🤖 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 `@Justfile` around lines 143 - 148, Update the [linux] build-runtime recipe to delegate directly to the existing build-linux primitive instead of repeating backend defaulting and package-script invocation. Preserve forwarding of backend, cuda_arch, and rocm_arch arguments through build-linux, relying on its bash-backed implementation for shell-specific logic.
🤖 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 @.agents/skills/manage-ci/SKILL.md:
- Around line 240-244: Clarify the contract in the “Main CI executable lanes”
guidance: state that backend lanes may build the neutral host in a distinct step
from the runtime, but must consume the verified producer host unchanged for the
OS/architecture rather than producing independent backend-specific hosts.
Preserve the adjacent packaged runtime requirement and exact-byte host reuse.
In @.github/workflows/release.yml:
- Around line 230-236: Replace the mapfile usages in the compose_cpu_products
macOS release step with a Bash 3.2-compatible portable read loop, while
preserving the archive and runtime-directory collection, count validations, and
subsequent extraction and verification flow.
In `@crates/mesh-llm-system/src/benchmark.rs`:
- Around line 170-182: Update the benchmark command setup around the Windows
PATH handling to configure Unix dynamic-library lookup for the spawned tool:
prepend the runtime’s sibling lib directory to LD_LIBRARY_PATH on Linux and
DYLD_LIBRARY_PATH on macOS, preserving each existing environment value.
Alternatively, update the mesh-llm-gpu-bench build configuration to embed the
appropriate $ORIGIN/../lib or `@loader_path/`../lib rpath, ensuring the backend
libraries load when launched through this function.
In `@docs/design/NATIVE_RUNTIMES.md`:
- Around line 30-36: Update the build command description in NATIVE_RUNTIMES.md
to avoid presenting just build-runtime as universally available; either mention
the platform-specific macOS build-mac alternative alongside it or remove the
explicit just build-runtime reference, while preserving the distinction between
local product composition and static native ABI compilation.
In `@scripts/build-linux.sh`:
- Around line 28-33: Update the positional fallback in the build script’s
argument parsing case to reject values beginning with “-” before assigning
CUDA_ARCH. Preserve the existing compatibility behavior for non-option
positional architecture values, while routing unknown flags such as --skipui to
the usage/error path.
In `@scripts/ci-client-readiness-smoke.sh`:
- Around line 44-56: The cleanup function’s return status is discarded when
invoked by the EXIT trap, allowing client failures after SIGINT to be masked.
Update cleanup and the EXIT-trap invocation to preserve cleanup_status as the
authoritative result, explicitly exiting with that status after cleanup while
retaining the intentional set +e/wait handling.
- Around line 96-101: Guard the shutdown signal in the client readiness success
path so a missing or already-exited process does not trigger set -e failure.
Update the kill command following the “client readiness observed” message, while
preserving the existing polling loop and successful smoke-run behavior.
In `@scripts/verify-native-runtime-package.sh`:
- Around line 143-146: Update the runtime manifest parsing around the files and
tools assignments to default only when the fields are absent, rather than using
falsy-value coercion. Preserve the existing isinstance checks so values such as
[], "", and 0 fail the checksum-map validation while missing fields still use an
empty map.
---
Outside diff comments:
In `@docs/design/TESTING.md`:
- Line 1161: Renumber the new “Release host/runtime/product boundary” section
heading so it no longer duplicates section 16, while preserving the existing
“Mesh ID generation (originator)” numbering and keeping subsequent checklist
references unambiguous.
In `@install.sh`:
- Around line 676-730: The install_bundle flow currently makes
remove_stale_binaries deletions irreversible before the installation is
committed. Update remove_stale_binaries and its call in install_bundle to move
stale entries into the existing backup_dir, perform cleanup, and only remove
backup_dir after cleanup succeeds; ensure rollback restores both overwritten and
stale entries on interruption or failure.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 1218-1230: Remove the unused ROCM_HIP_SDK_FILENAME and
WINDOWS_CUDA_VERSION environment variables, along with the related CUDA pin
comment, from the build_windows_gpu job. Leave the job’s artifact composition
configuration and other environment settings unchanged.
In `@Justfile`:
- Around line 143-148: Update the [linux] build-runtime recipe to delegate
directly to the existing build-linux primitive instead of repeating backend
defaulting and package-script invocation. Preserve forwarding of backend,
cuda_arch, and rocm_arch arguments through build-linux, relying on its
bash-backed implementation for shell-specific logic.
In `@scripts/ci-smoke-test.sh`:
- Around line 71-76: Extract the shared native-runtime resolution and export
logic into a sourced helper such as scripts/lib/native-runtime-bundle.sh,
preserving the MESH_LLM-relative fallback and missing-directory failure
behavior. Replace the duplicated blocks in scripts/ci-smoke-test.sh#L71-L76,
scripts/ci-compat-smoke.sh#L57-L62, scripts/ci-two-node-split-smoke.sh#L52-L57,
scripts/ci-two-node-client-serving-smoke.sh#L46-L51, and
scripts/ci-client-auto-test.sh#L38-L43 with calls to that helper so
discover_native_runtime_bundle_dirs consumes one shared contract.
In `@scripts/tests/test_install_sh.py`:
- Around line 274-340: The tests
test_install_bundle_accepts_complete_legacy_archive and
test_install_bundle_rejects_post_contract_legacy_shape hard-code versions around
COMPOSED_PRODUCT_MIN_VERSION. Derive the below-floor and at/above-floor version
values from the install.sh constant, or document the intentional coupling
clearly so the tests remain aligned when the threshold changes.
In `@tools/xtask/src/workflow_checks.rs`:
- Around line 580-609: Update the second workflow-check loop to validate the
extracted Windows job sections, ci_windows_cpu and ci_windows_gpu, instead of
the entire ci_workflow. Preserve the existing assertions and context labels, but
loosen the package-runtime needle from the backslash-continued form to
scripts/package-native-runtime.sh so the CPU section is matched correctly.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4ca805d-090c-4be3-a6f6-46bfbf372046
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.agents/skills/benchmark-tune/SKILL.md.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.md.github/actions/restore-smoke-inputs/action.yml.github/workflows/ci.yml.github/workflows/pr_builds.yml.github/workflows/release.ymlAGENTS.mdCONTRIBUTING.mdJustfileRELEASE.mdbuild-unification.mdci/ci.mdcrates/mesh-llm-gpu-bench/Cargo.tomlcrates/mesh-llm-gpu-bench/build.rscrates/mesh-llm-gpu-bench/native/metal/membench_main.mcrates/mesh-llm-gpu-bench/src/capture.rscrates/mesh-llm-gpu-bench/src/cuda.rscrates/mesh-llm-gpu-bench/src/hip.rscrates/mesh-llm-gpu-bench/src/intel.rscrates/mesh-llm-gpu-bench/src/lib.rscrates/mesh-llm-gpu-bench/src/metal.rscrates/mesh-llm-gpu-bench/src/runner.rscrates/mesh-llm-host-runtime/Cargo.tomlcrates/mesh-llm-native-runtime/src/cache.rscrates/mesh-llm-native-runtime/src/lib.rscrates/mesh-llm-native-runtime/src/manifest.rscrates/mesh-llm-system/Cargo.tomlcrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm-system/src/benchmark/tests.rscrates/mesh-llm/Cargo.tomldocs/CODESIGNING.mddocs/design/NATIVE_RUNTIMES.mddocs/design/TESTING.mdinstall.ps1install.shschemas/product-v2.schema.jsonscripts/build-development-product.shscripts/build-host.shscripts/build-linux.shscripts/build-mac.shscripts/build-release.shscripts/build-windows.ps1scripts/ci-client-auto-test.shscripts/ci-client-readiness-smoke.shscripts/ci-compat-smoke.shscripts/ci-smoke-test.shscripts/ci-two-node-client-serving-smoke.shscripts/ci-two-node-split-smoke.shscripts/package-native-runtime.shscripts/package-release.ps1scripts/package-release.shscripts/publish-crates.shscripts/tests/test_build_development_product.pyscripts/tests/test_build_windows.pyscripts/tests/test_install_ps1.pyscripts/tests/test_install_sh.pyscripts/tests/test_package_release.pyscripts/tests/test_windows_native_runtime_deps.pyscripts/verify-native-runtime-package.shscripts/windows-native-runtime-deps.pytools/xtask/src/workflow_checks.rs
💤 Files with no reviewable changes (8)
- crates/mesh-llm-gpu-bench/src/intel.rs
- crates/mesh-llm-gpu-bench/src/hip.rs
- crates/mesh-llm-gpu-bench/src/cuda.rs
- crates/mesh-llm-gpu-bench/src/metal.rs
- crates/mesh-llm-gpu-bench/src/capture.rs
- crates/mesh-llm-gpu-bench/src/runner.rs
- crates/mesh-llm-gpu-bench/build.rs
- crates/mesh-llm-gpu-bench/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (11)
- schemas/product-v2.schema.json
- .agents/skills/manage-ci/references/current-inventory.md
- CONTRIBUTING.md
- AGENTS.md
- scripts/tests/test_package_release.py
- scripts/tests/test_install_ps1.py
- install.ps1
- build-unification.md
- crates/mesh-llm-native-runtime/src/manifest.rs
- scripts/package-release.sh
- RELEASE.md
This was referenced Aug 8, 2026
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TLDR: This is needed to correct the docker builds - during some testing, I found that there were CUDA errors inside the containers because we had a couple different build processes going on. This unifies them all to be based on the dynamic runtime going forward.
product-v2bundleWhy
The prior release paths could rebuild or package backend-specific hosts and relied on cache-centric runtime discovery. This change makes the host/runtime boundary explicit and gives downstream packaging one immutable product contract.
Validation
cargo fmt --all --checkmesh-llm-runtime-install: 21 tests passedmesh-llm-host-runtime --lib: 1,838 passed, 8 ignoredmesh-llm-system: 179 tests passed--version,--help, and host-only client smoke passedFollow-up scope
The release workflow is compatible with the new contract but still duplicates the neutral host build across backend product jobs. A later workflow-only refactor can split that into a single host producer matrix plus product consumers without changing the artifact contract.
Summary by CodeRabbit
New Features
product-manifest.json.runtime listterminology and readiness behavior.Bug Fixes
Documentation