chore(code-quality): generalized code-quality refactoring - #1098
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughChangesControl-plane lifecycle commands
Shared prefix affinity
Model target lookup indexing
Capability inference helpers
Model artifact selection
ANSI output test normalization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
crates/mesh-llm-routing/src/prefix_affinity.rs (2)
91-118: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider cloning only the target, not the whole entry.
self.entries.get(&key).cloned()clonesAffinityEntry<T>(target +Instant) on every lookup, and the entry is then re-fetched mutably to refreshlast_used. Cloning the target alone after the candidate check keeps the same semantics with one fewer copy.🤖 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-routing/src/prefix_affinity.rs` around lines 91 - 118, Update lookup to borrow the stored AffinityEntry while checking candidates, then clone only its target for the returned value; avoid cloning the entire entry via entries.get(&key).cloned(). Preserve the existing stale-entry removal, touch_key, timestamp refresh, and hit/miss statistics behavior.
75-87: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffLRU bookkeeping is O(n) per operation with a
Stringclone each touch.
touch_key/remove_keylinearly scan aVecDequethat can hold up toPREFIX_AFFINITY_MAX_ENTRIES(4096)AffinityKeys, each comparison including aStringcompare, andVecDeque::removeshifts elements. This runs on everylookup/learn, i.e. per routed request. Behavior matches the previous per-crate implementations, so this is not a regression, but now that the logic is centralized it is a good place to switch to a monotonic sequence counter (HashMap<AffinityKey, u64>order stamps +BTreeMap<u64, AffinityKey>) or anIndexMap/intrusive LRU to get O(log n)/O(1) touches.🤖 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-routing/src/prefix_affinity.rs` around lines 75 - 87, Replace the VecDeque-based LRU bookkeeping in touch_key and remove_key with an indexed ordering structure that avoids linear scans, String comparisons, and element shifting on each operation. Use monotonic order stamps with a HashMap<AffinityKey, u64> and BTreeMap<u64, AffinityKey>, or an equivalent O(log n)/O(1) LRU implementation, while preserving eviction order and existing entry-removal behavior.crates/mesh-client/src/network/affinity.rs (1)
126-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMapping helper is duplicated in
crates/mesh-llm-host-runtime/src/network/affinity.rs(lines 241-261).The two
affinity_stats_snapshotfunctions are identical apart from the extratarget_reputationfield. If the snapshot struct grows again, both copies must be updated. Optional: add aFrom<PrefixAffinityStats>-style constructor next toPrefixAffinityStatsand let each crate extend it, or keep as-is since the twoAffinityStatsSnapshottypes are crate-local.🤖 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-client/src/network/affinity.rs` around lines 126 - 145, The AffinityStatsSnapshot field mapping is duplicated between the two crate-local affinity_stats_snapshot helpers. Consolidate the shared PrefixAffinityStats-to-snapshot mapping through a reusable From-style constructor or equivalent helper near PrefixAffinityStats, then have each crate-specific snapshot builder reuse it while preserving the runtime crate’s target_reputation field.crates/mesh-llm-types/src/models/capabilities.rs (3)
13-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
const fnfor the pure mapping helpers.
status/label/is_supportedare total, side-effect-free mappings over aCopyenum; making themconst fnmatches the existingconst fn as_strconvention used elsewhere in the workspace (e.g.crates/mesh-llm-host-runtime/src/api/model_targets.rs:85) and allows const-context use.♻️ Proposed tweak
- fn is_supported(self) -> bool { + const fn is_supported(self) -> bool { - self == Self::Supported + matches!(self, Self::Supported) } - fn status(self) -> &'static str { + const fn status(self) -> &'static str { - fn label(self) -> Option<&'static str> { + const fn label(self) -> Option<&'static str> {Note
==isn't usable inconst fnfor derivedPartialEq, hence thematches!swap.🤖 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-types/src/models/capabilities.rs` around lines 13 - 34, Make CapabilityLevel::is_supported, CapabilityLevel::status, and CapabilityLevel::label const fn so they can be used in const contexts. Replace the derived-PartialEq comparison in is_supported with a const-compatible matches! check, leaving the existing mappings and return values unchanged.
236-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVision special-case could fold into
name_signal_level.The audio/reasoning/tool-use branches share
name_signal_level, while vision duplicates the same shape inline only to gate the "likely" tier. Passing the gate into the helper removes the asymmetry and keeps the precedence rule in one place.♻️ Proposed refactor
fn merge_name_signal(caps: &mut ModelCapabilities, value: &str, allow_likely_vision: bool) { - let vision = if strong_vision_name_signal(value) { - CapabilityLevel::Supported - } else if allow_likely_vision && likely_vision_name_signal(value) { - CapabilityLevel::Likely - } else { - CapabilityLevel::None - }; - caps.upgrade_vision(vision); + caps.upgrade_vision(name_signal_level( + value, + strong_vision_name_signal, + likely_vision_name_signal, + allow_likely_vision, + )); caps.upgrade_audio(name_signal_level( value, strong_audio_name_signal, likely_audio_name_signal, + true, )); @@ fn name_signal_level( value: &str, strong: fn(&str) -> bool, likely: fn(&str) -> bool, + allow_likely: bool, ) -> CapabilityLevel { if strong(value) { CapabilityLevel::Supported - } else if likely(value) { + } else if allow_likely && likely(value) { CapabilityLevel::Likely } else { CapabilityLevel::None } }Behavior is unchanged;
config_model_type_preserves_likely_and_supported_levelsstill pins the vision-likely suppression for config-derived signals.🤖 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-types/src/models/capabilities.rs` around lines 236 - 295, Refactor name_signal_level to accept a boolean gate for its likely tier, then use it in merge_name_signal for vision, audio, reasoning, and tool-use signals. Preserve vision’s existing allow_likely_vision behavior while keeping supported signals and config-derived likely suppression unchanged, including config_model_type_preserves_likely_and_supported_levels.
500-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name overstates what is asserted.
strong_name_evidence_dominates_likely_and_existing_valuesasserts that vision staysLikelyand audio staysSupported, i.e. it characterizes monotonic upgrade (never downgrade), not dominance. Something likename_signals_only_upgrade_existing_levelsreads truer.🤖 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-types/src/models/capabilities.rs` around lines 500 - 520, The test function name strong_name_evidence_dominates_likely_and_existing_values mischaracterizes the assertions; rename it to reflect that name signals only upgrade existing capability levels, such as name_signals_only_upgrade_existing_levels. Keep the test setup and assertions unchanged.
🤖 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-client/src/network/affinity.rs`:
- Around line 126-145: The AffinityStatsSnapshot field mapping is duplicated
between the two crate-local affinity_stats_snapshot helpers. Consolidate the
shared PrefixAffinityStats-to-snapshot mapping through a reusable From-style
constructor or equivalent helper near PrefixAffinityStats, then have each
crate-specific snapshot builder reuse it while preserving the runtime crate’s
target_reputation field.
In `@crates/mesh-llm-routing/src/prefix_affinity.rs`:
- Around line 91-118: Update lookup to borrow the stored AffinityEntry while
checking candidates, then clone only its target for the returned value; avoid
cloning the entire entry via entries.get(&key).cloned(). Preserve the existing
stale-entry removal, touch_key, timestamp refresh, and hit/miss statistics
behavior.
- Around line 75-87: Replace the VecDeque-based LRU bookkeeping in touch_key and
remove_key with an indexed ordering structure that avoids linear scans, String
comparisons, and element shifting on each operation. Use monotonic order stamps
with a HashMap<AffinityKey, u64> and BTreeMap<u64, AffinityKey>, or an
equivalent O(log n)/O(1) LRU implementation, while preserving eviction order and
existing entry-removal behavior.
In `@crates/mesh-llm-types/src/models/capabilities.rs`:
- Around line 13-34: Make CapabilityLevel::is_supported,
CapabilityLevel::status, and CapabilityLevel::label const fn so they can be used
in const contexts. Replace the derived-PartialEq comparison in is_supported with
a const-compatible matches! check, leaving the existing mappings and return
values unchanged.
- Around line 236-295: Refactor name_signal_level to accept a boolean gate for
its likely tier, then use it in merge_name_signal for vision, audio, reasoning,
and tool-use signals. Preserve vision’s existing allow_likely_vision behavior
while keeping supported signals and config-derived likely suppression unchanged,
including config_model_type_preserves_likely_and_supported_levels.
- Around line 500-520: The test function name
strong_name_evidence_dominates_likely_and_existing_values mischaracterizes the
assertions; rename it to reflect that name signals only upgrade existing
capability levels, such as name_signals_only_upgrade_existing_levels. Keep the
test setup and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f2a8c44-2e2b-41a1-a946-eebc9b20b4c0
📒 Files selected for processing (11)
crates/mesh-client/src/client/control_plane.rscrates/mesh-client/src/network/affinity.rscrates/mesh-client/tests/control_plane_lifecycle_client.rscrates/mesh-llm-host-runtime/src/api/mod.rscrates/mesh-llm-host-runtime/src/api/model_targets.rscrates/mesh-llm-host-runtime/src/api/split_readiness.rscrates/mesh-llm-host-runtime/src/network/affinity.rscrates/mesh-llm-routing/src/lib.rscrates/mesh-llm-routing/src/prefix_affinity.rscrates/mesh-llm-types/src/models/capabilities.rscrates/model-artifact/src/lib.rs
4215870 to
3770a3e
Compare
…-retention * origin/main: Refresh llama.cpp upstream patch queue (#1105) Revert "Refresh llama.cpp upstream patch queue (#1099)" Refresh llama.cpp upstream patch queue (#1099) chore(code-quality): generalized code-quality refactoring (#1098) Document canonical Homebrew tap (#1102) Rename the Node SDK npm package scope (#1101)
1 participant
Summary
Reduces duplicated routing, model-selection, capability-detection, and lifecycle code while preserving existing behavior and compatibility.
This work follows an audit of all 56 workspace crates and retains only reductions with measurable production-code or runtime benefits.
Changes
Impact
ifand 6matchnodes.ModelTargetPayloadclones per target.Total repository LoC increases because of the added characterization and integration tests.
Validation
cargo fmt --all --checkand changed-file diagnostics passed.just buildpassed.control_unsupportedbehavior passed.just test-allrun passed.Subsequent exact-final-state suite reruns encountered unrelated existing system and UI flakes. Each failing test passed in isolated repeated runs; none touches files changed by this PR.
Compatibility
This change is behavior-preserving:
Summary by CodeRabbit
New Features
Bug Fixes
Tests