fix(mesh): expose upsert_served_model_descriptor to production callers - #1885
Merged
Merged
Conversation
`runtime/model_presentation.rs` calls `Node::upsert_served_model_descriptor` from production code since #1879, and #1708 landed six minutes later with the method still under `#[cfg(test)]`, because every caller it knew about was a test or `runtime/local.rs`. Neither branch saw the other, so `main` at 85b142d no longer builds the crate outside of `cfg(test)`: error[E0599]: no method named `upsert_served_model_descriptor` found for reference `&mesh::node::Node` in the current scope --> crates/mesh-llm-host-runtime/src/runtime/model_presentation.rs:44:18 The `Main · Quality` run on that head fails in all three Clippy batches with the same error. Dropping the attribute is the whole fix: the method has a production caller now, so it is no longer dead code outside tests. Reported in #1884.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe ChangesModel descriptor runtime availability
Priority: ⬆️ High Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix · Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to The runtime startup path can again update served model descriptors in production. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
i386
pushed a commit
that referenced
this pull request
Sep 15, 2026
…es (#1888) Six host-runtime tests build a `MeshConfig` by interpolating a temp path straight into a TOML basic string. On Windows that path is `C:\Users\...`, and TOML reads `\U` as the start of a unicode escape: ``` Error { message: "too few unicode value digits, expected unicode hexadecimal value" } ``` Same defect #1834 fixed in `mesh-llm-commands`, in the same shape. ## The six sites | test | site | |---|---| | `resolver::exact_head_tests::canonical_selector_wins_over_another_models_derived_profile` | `resolver/exact_head_tests.rs:50` | | `resolver::exact_head_tests::served_alias_and_resolved_path_do_not_select_model_specific_config` | same fixture | | `resolver::native_mtp_tests::speculative_strategy_auto_uses_hardware_model_path_for_direct_gguf_detection` | `resolver/native_mtp_tests.rs:389` | | `resolver::native_mtp_tests::speculative_strategy_native_mtp_accepts_external_mtp_sidecar` | `resolver/native_mtp_tests.rs:1030` | | `local_split::test_support::runtime_resolver_uses_config_identity_and_honors_device_override` | `local_split/test_support.rs:693` and `:704`, one template | | `local_split::tests::content_attestation::strict_multimodal_stage_builder_keeps_local_paths_off_downstream_loads` | `local_split/tests/content_attestation.rs:85` | The `local_split` template carries two `[[models]]` entries that share a single `{model_path}` argument, so both placeholders lose their quotes together. My first pass fixed only the second one and the test kept failing with the rendered literal string wrapped in a second pair of quotes, which is how the seventh site turned up. ## What this changes Each site renders the path through `toml::Value::String(...).to_string()`, which quotes and escapes for TOML, and the fixture drops its own quotes around the placeholder. Nothing new is introduced: `resolver/test_support.rs` already has `toml_path` doing exactly that, used eleven times by the speculative tests, and `local_split/test_support.rs` already writes the same expression three times a few lines below one of the sites that bypassed it. The one addition is an explicit import in `exact_head_tests.rs`, since `test_support` is a private module with no glob re-export. On Linux and macOS the rendered value is byte-identical to what the fixtures produced before, since a path without backslashes or quotes renders as the same basic string. ## Measured effect Windows 11. On `main` at 950e905 all six fail with the unicode-escape error above. On this branch, the six plus the seven other tests of `runtime::local_split::test_support`, run on the tree that `main` at 8ab3586 now has (85b142d plus #1885, which the run carried as a cherry-pick because `main` did not build in between): ``` test result: ok. 13 passed; 0 failed; 0 ignored ``` ## Gates - `cargo test -p mesh-llm-host-runtime` (the six tests and `runtime::local_split::test_support`): 13 passed, 0 failed. - `cargo fmt --all --check`: clean. - `cargo clippy -p mesh-llm-host-runtime --all-targets`: 8 locations, complete sorted set identical to `main` at 60b7f5f. - `cargo run -p xtask -- repo-consistency no-console-print`: passed. - `scripts/check-env-mutation-contract.py` and `python -m unittest scripts.tests.test_env_mutation_contract`: passed, 234 sites. - `scripts/check-conventional-commit.py --range upstream/main..HEAD`: passed. ## Not verified Nothing run on Linux or macOS. The rendered TOML is the same there, but I have not executed the tests on either. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved test configuration handling for model, projector, hardware model, and sidecar paths. - Added safer path serialization to preserve paths accurately across different filesystem formats. - Maintained coverage for model resolution, native MTP detection, external sidecar resolution, and multimodal configuration validation. - Updated test fixtures to use canonical path values, improving reliability across environments. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Fixes #1884.
mainat 85b142d does not buildmesh-llm-host-runtimeoutside ofcfg(test):runtime/model_presentation.rs:44(from #1879, 20:51Z) callsNode::upsert_served_model_descriptor, andmesh/node.rs:1355(from #1708, 20:56Z) still has that method under#[cfg(test)]. Each PR was green on its own base; the error only exists on the merged result, andMain · Qualityon that head fails in all three Clippy batches with it.What this changes
One line: the
#[cfg(test)]attribute onupsert_served_model_descriptorgoes away. The method has a production caller now, so it is no longer dead code outside tests, and nothing else moves.Measured
Windows 11,
cargo check -p mesh-llm-host-runtime:Not verified
I have not rerun the full test suite on this branch; the change removes a compile-time gate and adds no code. Nothing run on Linux or macOS.
Summary by CodeRabbit