Require explicit native runtime version selection - #873
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces config-file-driven ChangesConfig-driven native runtime version selection
macOS dylib rpath rewriting
Sequence Diagram(s)sequenceDiagram
participant CLI as run_cli_entrypoint
participant HostRuntime as initialize_host_runtime_with_config
participant Config as plugin::load_config
participant Selector as NativeRuntimeStartupSelection
participant Loader as try_load_installed_native_runtime
participant Resolver as resolve_installed_native_runtime_plan
CLI->>HostRuntime: config_path
HostRuntime->>Config: load_config(config_path)
Config-->>HostRuntime: RuntimeConfig {native_runtime}
HostRuntime->>Selector: explicit(mesh_version, skippy_abi) or current()
HostRuntime->>Loader: startup_selection
Loader->>Resolver: target_mesh_version, target_skippy_abi
Resolver-->>Loader: Option<LoadedNativeRuntime>
Loader-->>HostRuntime: Result<Option<LoadedNativeRuntime>>
sequenceDiagram
participant CLI as dispatch_runtime_command
participant Selector as native_runtime_config_selector
participant Commands as run_native_runtime_list/install
participant Formatter as runtime_native_formatter
participant Output as stdout/stderr
CLI->>Selector: load config_path
Selector-->>CLI: Option<NativeRuntimeConfigSelector>
CLI->>Commands: NativeRuntimeConfigSelection {mesh_version, skippy_abi, selection}
Commands->>Formatter: render_available/render_installed/render_doctor
Formatter-->>Output: human or JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
i386
changed the title
ndizazzo
marked this pull request as ready for review
June 18, 2026 21:00
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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-host-runtime/src/system/native_runtime.rs`:
- Around line 214-228: The issue is that the synthesized manifest passed to
select_native_runtime_for_skippy_abi will have its artifacts normalized to fill
in missing mesh_version fields from the parent manifest, allowing stale cached
runtimes to be selected. Replace the call to
select_native_runtime_for_skippy_abi with an installed-artifact-specific
selection function that evaluates cached manifests without normalizing missing
mesh_version values. Additionally, add a regression test that verifies an
installed runtime whose manifest omits mesh_version is not incorrectly rewritten
or selected.
In `@crates/mesh-llm-native-runtime/src/resolver.rs`:
- Around line 161-166: After normalizing artifacts with
artifact_with_manifest_mesh_version in the resolver, add validation to ensure
that source_for_artifact returns bundles with matching mesh_version and
skippy_abi before using them. Compare the full artifact identity of the bundle
source against the normalized artifact's mesh_version and skippy_abi fields
before proceeding with seen.insert and artifacts.push operations. Apply this
validation check at both locations where artifact_with_manifest_mesh_version is
called (around lines 161-166 and 194-199) to prevent stale bundles with matching
runtime ids from bypassing version-selection guarantees.
In `@crates/mesh-llm/src/commands/runtime.rs`:
- Around line 21-23: The native_runtime_config_selector is being loaded
unconditionally before calling run_native_runtime_list, but the selector is only
used when listing available runtimes with the --available flag. For installed or
default listings, the function only reads from cache and doesn't use the
selector, so a bad config shouldn't cause failures. Move the
native_runtime_config_selector call inside a conditional block that only
executes when the --available variant is being handled, ensuring the selector is
only created and validated when it's actually needed by the command.
In `@scripts/package-native-runtime.sh`:
- Around line 258-259: The install_name_tool commands for rewriting the library
identity and adding rpaths are critical safety operations that should not
silently fail. Remove the `|| true` suppression from the install_name_tool -id
command to ensure identity rewrites fail properly if they encounter errors. For
the install_name_tool -add_rpath command, instead of using blanket `|| true`
suppression, selectively handle only the duplicate rpath error case while
allowing actual failures to propagate and cause the script to exit with an
error.
In `@scripts/verify-native-runtime-package.sh`:
- Around line 146-148: The find command in the dylib detection logic uses -type
f which only matches regular files and ignores symlinks. Since the artifact
directory can contain .dylib symlinks, this causes the check to incorrectly
return 0 when only symlinks exist, skipping necessary macOS path validation.
Modify the find command to detect both regular files and symlinks by either
replacing -type f with a condition that matches both types (using -type f -o
-type l) or by removing the type restriction entirely, ensuring the detection
correctly identifies both .dylib files and .dylib symlinks in the artifact_dir.
- Around line 175-177: The current check for "`@loader_path`" uses a simple string
containment search on the entire otool output, which incorrectly passes if
"`@loader_path`" appears anywhere in the output rather than specifically within an
LC_RPATH load command. Modify the logic at line 175-177 to parse the otool
output and explicitly look for LC_RPATH sections, then verify that
"`@loader_path`" appears within those specific LC_RPATH command blocks rather than
just somewhere in the output. This ensures only valid packages with properly
configured LC_RPATH entries are accepted.
🪄 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: 5dec15d1-afa1-4283-853f-4bfcaeb70c18
📒 Files selected for processing (14)
crates/mesh-llm-commands/src/runtime_native.rscrates/mesh-llm-config/src/lib.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-host-runtime/src/plugin/config.rscrates/mesh-llm-host-runtime/src/system/native_runtime.rscrates/mesh-llm-native-runtime/src/resolver.rscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/commands/runtime.rscrates/mesh-llm/src/lib.rsscripts/package-native-runtime.shscripts/verify-native-runtime-package.sh
ndizazzo
requested changes
Jun 18, 2026
Collaborator
There was a problem hiding this comment.
I've been having some struggles with the ergonomics of the CLI lately - so my review is going to be a bit more centred around the experience:
runtime list --availablewont give any rejection reasons if there's nothing comparable... largely because resolver.rs bails out before returning any evaluated candidates against a matched runtime.- native runtime config schema understates the restart boundary. (my agent found this one):
The new settings are registered via runtime_setting at
[built_in_schema.rs (line 143)]
(mesh-llm-config/src/model/built_in_schema.rs:143), and that helper
marks runtime settings as API-visible / dynamic-validation-only while
inheriting ModelReload restart scope from basic_setting at
[built_in_schema.rs (line 895)]
(crates/mesh-llm-config/src/model/built_in_schema.rs:895).
But the selector is consumed before dynamic native libraries are loaded
during host initialization in [lib.rs (line 63)]
(crates/mesh-llm-host-runtime/src/lib.rs:63).
This should be a process-restart setting, probably with a
native-runtime-specific schema helper and a clearer description.
Otherwise the UI/API can imply that a model reload is sufficient.
Otherwise, seems like a good change. I tend to want poweruser-type features and this has the right config additions I would wanna see. A rough improvement there would be helping the discoverability of this (doc might be fine, or add to the sample templates).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/design/NATIVE_RUNTIMES.md (1)
329-341: ⚡ Quick winClarify the
selectionfield and its relationship tomesh_version.The configuration example (lines 331–335) shows both
mesh_versionandselectionfields, but the prose explanation (lines 337–341) only describesmesh_versionandskippy_abi. Users need to understand:
- What the
selectionfield represents and how to use it (e.g., theexact:<artifact-id>syntax shown in the example).- Whether both fields can be specified together, and if so, what takes precedence or if they have specific validation rules.
- The relationship between
selectionand the auto-detection behavior when neither field is provided.Add a sentence or two to the prose explaining the
selectionfield's purpose and usage pattern.🤖 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/NATIVE_RUNTIMES.md` around lines 329 - 341, The prose explanation following the configuration example does not adequately describe the `selection` field, which is shown in the example but not explained in the text. Add clarification to the section after the TOML code block to explain what the `selection` field represents, describe the `exact:<artifact-id>` syntax pattern shown in the example, clarify whether `mesh_version` and `selection` can both be specified together and what their relationship is (precedence, validation rules), and explain how specifying `selection` relates to the auto-detection behavior that occurs when neither field is provided. Keep the explanation concise but complete enough for users to understand the purpose and usage of the `selection` field.
🤖 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 `@docs/design/NATIVE_RUNTIMES.md`:
- Around line 329-341: The prose explanation following the configuration example
does not adequately describe the `selection` field, which is shown in the
example but not explained in the text. Add clarification to the section after
the TOML code block to explain what the `selection` field represents, describe
the `exact:<artifact-id>` syntax pattern shown in the example, clarify whether
`mesh_version` and `selection` can both be specified together and what their
relationship is (precedence, validation rules), and explain how specifying
`selection` relates to the auto-detection behavior that occurs when neither
field is provided. Keep the explanation concise but complete enough for users to
understand the purpose and usage of the `selection` field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1469e557-2bdf-4d30-b7c3-287e7371879b
📒 Files selected for processing (18)
crates/mesh-llm-commands/src/runtime_native.rscrates/mesh-llm-config/src/lib.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-ffi/src/lib.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-host-runtime/src/system/native_runtime.rscrates/mesh-llm-native-runtime/src/lib.rscrates/mesh-llm-native-runtime/src/resolver.rscrates/mesh-llm-nodejs/src/lib.rscrates/mesh-llm-runtime-install/src/lib.rscrates/mesh-llm/src/commands/doctor.rscrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/commands/runtime.rsdocs/design/NATIVE_RUNTIMES.mdscripts/package-native-runtime.shscripts/verify-native-runtime-package.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/mesh-llm-host-runtime/src/lib.rs
- scripts/package-native-runtime.sh
- crates/mesh-llm-config/src/lib.rs
- scripts/verify-native-runtime-package.sh
- crates/mesh-llm/src/commands/mod.rs
- crates/mesh-llm-native-runtime/src/resolver.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/mesh-llm-commands/src/runtime_native/formatters.rs (1)
333-335: 💤 Low valueMinor inconsistency in rejection message wording.
The
TargetTripleMismatchmessage says "host is {actual}" while other mismatch messages (lines 328, 331) use "artifact is for {actual}". The semantic is thatexpectedis what the host needs andactualis what the artifact provides, so this message has the labels reversed or uses inconsistent phrasing.Suggested fix for consistency
CandidateRejection::TargetTripleMismatch { expected, actual } => { - format!("target triple mismatch: expected {expected}, host is {actual}") + format!("target triple mismatch: expected {expected}, artifact is for {actual}") }🤖 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/formatters.rs` around lines 333 - 335, The TargetTripleMismatch error message in the match expression uses inconsistent phrasing compared to other rejection messages. Change the message from "target triple mismatch: expected {expected}, host is {actual}" to use "artifact is for {actual}" instead of "host is {actual}" to match the phrasing pattern used in the other CandidateRejection variants (appearing around lines 328 and 331), ensuring consistency in how the actual value is described across all mismatch rejection messages.crates/mesh-llm-commands/src/runtime_native.rs (1)
39-54: ⚡ Quick winRedundant
native_runtime_cachecall.
native_runtime_cache(cache_dir)?is called at line 39 unconditionally, and then again at line 54 inside theif availableblock. Whenavailableis true, the cache from line 39 is unused; whenavailableis false, only the cache from line 39 is used.Consider moving the cache creation to where it's needed to avoid the redundant call in the
availablepath.Suggested fix
let mesh_version = configured.mesh_version_or_current(); let selection = RuntimeSelection::parse(configured.selection)?; - let cache = native_runtime_cache(cache_dir)?; let formatter = runtime_native_formatter(json_output); if available { print_configured_selector(configured, json_output); if !json_output && manifest_path.is_none() && bundle_dirs.is_empty() { eprintln!("🔎 Loading native runtime release manifest"); } // ... manifest loading ... let profile = host_runtime_profile(); let cache = native_runtime_cache(cache_dir)?; // ... resolver and rows ... return formatter.render_available(&rows); } + let cache = native_runtime_cache(cache_dir)?; let installed = cache.installed()?; formatter.render_installed(&installed, cache.root())🤖 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 39 - 54, The function native_runtime_cache(cache_dir)? is called redundantly - once at the beginning before the if available block and again inside that block. Remove the duplicate call to native_runtime_cache inside the if available block (currently at line 54) and reuse the cache variable that was already created and assigned at line 39 before the conditional. This eliminates the unnecessary duplicate function call in the available code path.
🤖 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 `@crates/mesh-llm-commands/src/runtime_native.rs`:
- Around line 39-54: The function native_runtime_cache(cache_dir)? is called
redundantly - once at the beginning before the if available block and again
inside that block. Remove the duplicate call to native_runtime_cache inside the
if available block (currently at line 54) and reuse the cache variable that was
already created and assigned at line 39 before the conditional. This eliminates
the unnecessary duplicate function call in the available code path.
In `@crates/mesh-llm-commands/src/runtime_native/formatters.rs`:
- Around line 333-335: The TargetTripleMismatch error message in the match
expression uses inconsistent phrasing compared to other rejection messages.
Change the message from "target triple mismatch: expected {expected}, host is
{actual}" to use "artifact is for {actual}" instead of "host is {actual}" to
match the phrasing pattern used in the other CandidateRejection variants
(appearing around lines 328 and 331), ensuring consistency in how the actual
value is described across all mismatch rejection messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9151eb2d-163a-42d9-80be-6b677eb77f19
📒 Files selected for processing (2)
crates/mesh-llm-commands/src/runtime_native.rscrates/mesh-llm-commands/src/runtime_native/formatters.rs
This was referenced Jun 28, 2026
This was referenced Jul 21, 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
2 participants
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.
MeshLLM now pairs native runtimes with an explicit MeshLLM version and Skippy ABI instead of accepting any cached runtime with a matching ABI.
What changed
mesh_versiondoes not match the selected MeshLLM runtime version.mesh-llm runtime installandmesh-llm runtime list --availableuse the same explicit selector from config.@rpath/...names and adds@loader_path; package verification now catches absolute packaged-dylib dependencies and missing@loader_path.Why
Upgrading from a MeshLLM version without separate native runtimes could leave an old cached runtime, such as
0.68.0, selected by a newer binary. On macOS, that stale runtime could then fail duringdlopenbecause packaged dylibs still referenced build-tree paths for sibling llama libraries.Runtime install scenarios
mesh_versionmismatch and pruned.mesh_versionmesh_version+skippy_abiselectioncuda12orexact:....runtime install cuda12with config selectionruntime prune --active-onlykeeps the pinned version instead of pruning it away.runtime prune --mesh-version Xskippy_abiorselectionwithoutmesh_versionruntime list --installedwith bad configruntime list --availablewith no compatible runtimemesh_versionmesh-llm doctorwith pinValidation
cargo check -p mesh-llmcargo test -p mesh-llm-native-runtime --libcargo test -p mesh-llm-config --libcargo test -p mesh-llm-host-runtime --features dynamic-native-runtime --lib system::native_runtimebash -n scripts/package-native-runtime.shbash -n scripts/verify-native-runtime-package.shcargo fmt --all -- --checkcargo clippy -p mesh-llm --all-targets -- -D warningscargo clippy -p mesh-llm --features dynamic-native-runtime --all-targets -- -D warningsSummary by CodeRabbit
[runtime.native_runtime]settings to pin nativemesh_version,skippy_abi, andselectionfor native runtime resolution.skippy_abi_versionwhen not provided.@loader_pathin verification.