Skip to content

fix(native): honor LCM_DEFAULT_URL in the rust LCM transport - #3740

Merged
aclauer merged 4 commits into
mainfrom
claude/fix/rust_lcm_default_url
Sep 1, 2026
Merged

fix(native): honor LCM_DEFAULT_URL in the rust LCM transport#3740
aclauer merged 4 commits into
mainfrom
claude/fix/rust_lcm_default_url

Conversation

@jeff-hykin

@jeff-hykin jeff-hykin commented Aug 27, 2026

Copy link
Copy Markdown
Member

liblcm reads LCM_DEFAULT_URL, so every python module lands on the bus it
names, but the rust transport hardcoded 239.255.76.67:7667. The two halves
of a pipeline then sit on different buses and nothing is delivered, with no
error on either side.

This is also why no python-to-native LCM integration test can pass: dimos'
conftest pins each pytest session to its own udpm port for isolation, which
the native side never joined.
@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change lets the native Rust LCM transport read LCM_DEFAULT_URL so it can join the multicast bus configured for Python modules. Focused execution found that malformed udpm:// endpoints still apply a valid ttl query parameter while falling back to the default group and port, creating a mixed configuration that contradicts the fallback warning. Align the parser behavior and warning before merging.

Confidence Score: 4/5

The URL fallback path needs correction or clearer semantics before merging because malformed endpoint values can still change the effective multicast TTL.

The malformed-group and malformed-port paths were both executed with a valid TTL and consistently produced the default endpoint with an overridden TTL.

Files Needing Attention: native/rust/dimos-module/src/lcm.rs, particularly the invalid group/port handling and subsequent TTL query parsing.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for the posted P2 finding and built a focused Rust test for an invalid endpoint with a valid TTL, which showed the full-fallback expectation failed and the partial-fallback observation test passed.
  • T-Rex validated the contract behavior after changes, confirming that invalid endpoint variants now produce group=239.255.76.67, port=7667, ttl=42, and verified the TTL parsing logic in native/rust/dimos-module/src/lcm.rs at lines 64-75, with the warning in 64-67 followed by continued parsing and TTL assignment.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P2 Invalid udpm endpoint warning claims a full fallback while TTL is retained

    • Bug
      • For udpm://not-an-ip:7667?ttl=42 and udpm://239.255.76.67:not-a-port?ttl=42, options_from_url returns the default multicast group 239.255.76.67 and port 7667, but returns TTL 42 rather than default TTL 1.
    • Cause
      • After group/port parsing fails, the match arm logs a warning but does not return the default options; execution continues into the query loop, which parses and assigns ttl.
    • Fix
      • Either return options immediately after the invalid group/port warning to make behavior match the message, or revise the warning to explicitly state that endpoint defaults are retained while valid query options such as TTL are still applied. Add coverage for invalid group and invalid port with a valid TTL.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix(native): honor LCM_DEFAULT_URL in th..." | Re-trigger Greptile

Comment thread native/rust/dimos-module/src/lcm.rs Outdated
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

@@           Coverage Diff           @@
##             main    #3740   +/-   ##
=======================================
  Coverage   77.58%   77.58%           
=======================================
  Files        1315     1315           
  Lines      124697   124697           
  Branches    10867    10867           
=======================================
+ Hits        96748    96750    +2     
+ Misses      24816    24814    -2     
  Partials     3133     3133           
Flag Coverage Δ
OS-ubuntu-24.04-arm 74.23% <ø> (-0.01%) ⬇️
OS-ubuntu-latest 74.78% <ø> (+<0.01%) ⬆️
Py-3.10 74.77% <ø> (ø)
Py-3.11 74.77% <ø> (-0.01%) ⬇️
Py-3.12 74.77% <ø> (-0.01%) ⬇️
Py-3.13 74.78% <ø> (+<0.01%) ⬆️
Py-3.14 74.78% <ø> (-0.01%) ⬇️
Py-3.14t 74.77% <ø> (-0.01%) ⬇️
SelfHosted-Large 30.22% <ø> (+<0.01%) ⬆️
SelfHosted-Linux 34.74% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread native/rust/dimos-module/src/lcm.rs Outdated
Hand-splitting on "://", "?", ":" and "&" reimplements a parser that is
already a dependency and gets the edge cases wrong.
aclauer
aclauer previously approved these changes Aug 28, 2026
@aclauer
aclauer enabled auto-merge August 31, 2026 23:16
@aclauer
aclauer added this pull request to the merge queue Aug 31, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 31, 2026
@aclauer
aclauer added this pull request to the merge queue Aug 31, 2026
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 1, 2026
Merged via the queue into main with commit 75375cd Sep 1, 2026
48 of 51 checks passed
@aclauer
aclauer deleted the claude/fix/rust_lcm_default_url branch September 1, 2026 00:43
poorwym pushed a commit that referenced this pull request Sep 4, 2026
Co-authored-by: Andrew Lauer <andrewcl@email.unc.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants