From de664a85d7f10138e619bb9103bfd3cd6478aa37 Mon Sep 17 00:00:00 2001 From: "spy-architect (osvetnik)" Date: Mon, 6 Jul 2026 18:53:52 +0300 Subject: [PATCH 01/33] =?UTF-8?q?spec:=20dimos=20spy=20task=20package=20?= =?UTF-8?q?=E2=80=94=20universal=20transport=20spy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design ratified in ticket f6c74d39 (subscribe_all non-conflating decision confirmed by Ivan). Task package for a worker to implement: - TASK.md: goal, decisions, implementation map, acceptance - docs/usage/transports/spy.md: design doc (why, architecture, decisions) - spec.py: subscribe_all contract tightened (every message, no conflation); SubscribeLatestMixin.subscribe_latest() stub = the explicit conflation opt-in - protocol/pubsub/spy.py: TopicStats / SpySource / LCMSpySource / ZenohSpySource / TransportSpy stubs (typed, NotImplementedError bodies) - utils/cli/spy/run_spy.py + `dimos spy` CLI registration (TUI stub) - test_spy.py: 17 contract tests, 15 failing until implemented; the zenoh subscribe_all conflation bug fails sharply (2 of 50 messages delivered) mypy clean (796 files); existing pubsub suites green. --- TASK.md | 125 ++++++++ dimos/protocol/pubsub/spec.py | 42 ++- dimos/protocol/pubsub/spy.py | 221 ++++++++++++++ dimos/protocol/pubsub/test_spy.py | 461 ++++++++++++++++++++++++++++++ dimos/robot/cli/dimos.py | 9 + dimos/utils/cli/spy/run_spy.py | 35 +++ docs/docs.json | 3 +- docs/usage/transports/spy.md | 104 +++++++ 8 files changed, 995 insertions(+), 5 deletions(-) create mode 100644 TASK.md create mode 100644 dimos/protocol/pubsub/spy.py create mode 100644 dimos/protocol/pubsub/test_spy.py create mode 100644 dimos/utils/cli/spy/run_spy.py create mode 100644 docs/usage/transports/spy.md diff --git a/TASK.md b/TASK.md new file mode 100644 index 0000000000..08489913fa --- /dev/null +++ b/TASK.md @@ -0,0 +1,125 @@ +# TASK: `dimos spy` — universal transport spy + +## Goal (the outcome) + +`dimos spy` shows every topic flowing on every DimOS pubsub transport — LCM and Zenoh +simultaneously in v1 — with per-topic message rate, bandwidth, average size, total +traffic, and liveness (age since last message), in a Textual TUI matching `dimos lcmspy`'s +look. **The spy never decodes a message payload** (hard constraint from Ivan): its +per-message work is `(topic string, payload length, timestamp)` and nothing else. + +Along the way, fix the pubsub spec so `subscribe_all` means the same thing on every +transport (every message, no conflation) and move conflation to an explicit opt-in. + +## Context & where it fits + +Design doc (read first): `docs/usage/transports/spy.md`. Decisions were ratified by +Ivan/brain in ticket f6c74d39; the doc records them. Summary: zenoh's `subscribe_all` +conflates (latest-wins, built for the rerun bridge, its only consumer) and only watches +`dimos/**`; LCM's delivers everything. The spy builds on `subscribe_all`, so the spec +asymmetry gets fixed as part of this task. + +The branch already contains the full skeleton — this file, typed stubs (compile + pass +mypy, bodies raise `NotImplementedError`), and contract tests in +`dimos/protocol/pubsub/test_spy.py` that fail until implemented. **The tests are the +acceptance criteria.** Don't weaken them; if one seems wrong, flag it instead. + +## What to implement + +### 1. Spec fix: non-conflating `subscribe_all` + opt-in `subscribe_latest` + +- `dimos/protocol/pubsub/spec.py` — implement + `SubscribeLatestMixin.subscribe_latest()`: the latest-wins-per-topic buffer + single + drain thread + wake event currently living in `ZenohPubSubBase.subscribe_all` + (`impl/zenohpubsub.py:199`) — lift that machinery here essentially verbatim (it is + transport-agnostic: key the latest-dict by `str(topic)`). Contract is in the stub + docstring: callback on the drain thread, newest-per-topic eventually delivered, + returned callable unsubscribes and stops+joins the drain. +- `impl/zenohpubsub.py` — `subscribe_all` becomes a plain non-conflating + `self.subscribe(Topic("**"), callback)` (scope widened from `dimos/**` to `**` per + ratified decision; see design doc). Keep the stop()-vs-subscribe locking behavior that + is already there. Delete the now-lifted drain machinery, but keep zenoh's + drain-stop bookkeeping working for `subscribe_latest` subscriptions — + `stop()` must still terminate any live drain threads (move `_drain_stops` + handling as needed; simplest: `subscribe_latest` registers its stopper the same + way `subscribe_all` used to). +- `dimos/visualization/rerun/bridge.py:461` — the bridge needs conflation + (rerun lags otherwise, see the old comment): switch `pubsub.subscribe_all(...)` to + `pubsub.subscribe_latest(...)`. +- LCM's `subscribe_all` already satisfies the contract — leave it. + +### 2. Spy core: `dimos/protocol/pubsub/spy.py` + +All stubs documented in-file. Implementation notes: + +- `TopicStats`: port the math from `dimos/utils/cli/lcmspy/lcmspy.py` `Topic` (deque of + `(timestamp, nbytes)`, windowed freq/bandwidth/avg-size, lifetime totals) but with + explicit timestamps (see stub docstrings) and typed. Lock around deque mutation vs + reads (record() on transport threads, queries from the UI thread). +- `LCMSpySource` / `ZenohSpySource`: own a raw-bytes bus instance + (`LCMPubSubBase(**kwargs)` / `ZenohPubSubBase(**kwargs)`), `start()/stop()` delegate, + `tap(cb)` = `bus.subscribe_all(lambda msg, topic: cb(str(topic), len(msg)))`. + That lambda is the entire hot path — no other work is permitted there. +- `TransportSpy`: on `start()`, start each source and tap it with a callback that stamps + `time.time()` once and updates the per-`SpyKey` `TopicStats` + `totals`. `snapshot()` + returns a shallow copy of the dict (TopicStats objects shared, dict copied). +- `subscribe_decoded`: leave raising `NotImplementedError` — it is the spec'd hook for + the follow-up task (per-topic opt-in decode, separate subscription via the typed + encoder classes; never part of the tap path). +- `default_sources()`: `[LCMSpySource(), ZenohSpySource()]` — both, always; the spy + ignores `DIMOS_TRANSPORT` (it observes, it doesn't participate). + +### 3. TUI + CLI: `dimos/utils/cli/spy/run_spy.py` + +Model on `dimos/utils/cli/lcmspy/run_lcmspy.py` (DataTable, 0.5 s refresh interval, +theme colors/gradients, `q`/`ctrl+c` quit, `web` argv mode via textual-serve). Layout +and flags are in the stub docstring. Sort rows by total traffic; render Type via +`split_type_suffix`; grey/dim rows whose age exceeds a few seconds (liveness). +Call `lcmservice.autoconf(check_only=True)` before entering raw TUI mode when the LCM +source is active (run_lcmspy.py:84 shows why). + +`dimos spy` is already registered in `dimos/robot/cli/dimos.py`. Make `dimos lcmspy` +a deprecated alias: print a one-line deprecation note to stderr and run the spy with +the lcm source only (keep the command working; delete nothing in +`dimos/utils/cli/lcmspy/` in this task). + +### 4. Hot-path caveat to fix while you're there + +`Topic.from_channel_str` (`impl/lcmpubsub.py:113`, called per message on LCM pattern +subscriptions) calls `resolve_msg_type`, which attempts imports and is **uncached** +(`dimos/msgs/helpers.py:26`) — that's real per-message overhead on the spy's LCM path. +Zenoh already caches the equivalent via `@lru_cache` on `_key_expr_to_topic` +(`impl/zenohpubsub.py:93`). Add `functools.lru_cache` to `resolve_msg_type` (or cache +channel→Topic in the LCM handler, whichever is smaller). + +## Constraints / non-goals + +- **No payload decoding on the spy hot path, ever.** The contract tests monkeypatch + `Vector3.lcm_decode` to explode and publish undecodable garbage to enforce it. +- No conflation anywhere in the spy path — every message counts once. +- Non-goals for v1: SHM/ROS/DDS/Redis sources; implementing `subscribe_decoded`; + restructuring `dimos/utils/cli/lcmspy/` (the alias just routes around it); + zero-copy zenoh sizing (`len(sample.payload)` without `to_bytes()`) — noted as + future work in the design doc. +- Follow repo conventions: `uv` for deps, mypy + pytest green before commit + (`bin/pytest-fast`), no docstring bloat. + +## Pointers (read in this order) + +1. `docs/usage/transports/spy.md` — the design doc (why + decisions). +2. `dimos/protocol/pubsub/spec.py` — subscribe-all abstraction + new mixin stub. +3. `dimos/protocol/pubsub/impl/zenohpubsub.py:199` — the conflation machinery to lift. +4. `dimos/utils/cli/lcmspy/lcmspy.py` — stats math to port; `run_lcmspy.py` — TUI to model. +5. `dimos/protocol/pubsub/spy.py` — the stubs you're filling in. +6. `dimos/protocol/pubsub/test_spy.py` — the acceptance tests. +7. `dimos/visualization/rerun/bridge.py:461` — the one-line bridge migration. + +## Acceptance + +- `uv run pytest dimos/protocol/pubsub/test_spy.py` — all pass. +- Existing suites stay green, notably `test_spec.py`, `test_zenohpubsub.py`, + `test_lcmpubsub.py`, and rerun bridge tests. +- `mypy` clean. +- Manual smoke: `dimos --transport=zenoh run unitree-go2 --replay ...` (or any + publisher) in one terminal, `dimos spy` in another → topics from both zenoh and LCM + visible with sane rates; `dimos lcmspy` still works and warns it's deprecated. diff --git a/dimos/protocol/pubsub/spec.py b/dimos/protocol/pubsub/spec.py index fe979fce82..96a3bf6f4a 100644 --- a/dimos/protocol/pubsub/spec.py +++ b/dimos/protocol/pubsub/spec.py @@ -115,7 +115,35 @@ def subscribe( # # - DiscoveryPubSub: Native support for discovering new topics as they appear. # Provides a default subscribe_all() by subscribing to each discovered topic. -class AllPubSub(PubSub[TopicT, MsgT], ABC): +class SubscribeLatestMixin(Generic[TopicT, MsgT], ABC): + """Conflated subscribe-all, built on top of subscribe_all(). + + Mixed into AllPubSub and DiscoveryPubSub so both expose subscribe_latest(). + """ + + @abstractmethod + def subscribe_all(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: ... + + def subscribe_latest(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: + """Subscribe to all topics, conflated: newest message per topic wins. + + For consumers that only need the current value per topic and must not + lag behind a fast producer (e.g. the rerun bridge). When the callback + is slower than the message flow, intermediate messages on a topic are + dropped and only the latest is delivered. + + Contract: + - The callback runs on a dedicated drain thread, never on the + transport's delivery thread. + - Per topic, the newest pending message is always eventually delivered + (no starvation); intermediate ones may be skipped. + - The returned callable unsubscribes the underlying subscribe_all and + stops+joins the drain thread. + """ + raise NotImplementedError + + +class AllPubSub(SubscribeLatestMixin[TopicT, MsgT], PubSub[TopicT, MsgT], ABC): """Mixin for PubSub that supports subscribing to all topics. Subclass from this if you support native subscribe-all (e.g. MQTT #, Redis *). @@ -124,7 +152,13 @@ class AllPubSub(PubSub[TopicT, MsgT], ABC): @abstractmethod def subscribe_all(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: - """Subscribe to all topics.""" + """Subscribe to all topics. + + Contract: delivers EVERY message on every topic this transport can + observe — implementations must not conflate, sample, or drop beyond + the transport's own delivery semantics. Consumers that only want the + newest message per topic use subscribe_latest() instead. + """ ... def subscribe_new_topics(self, callback: Callable[[TopicT], Any]) -> Callable[[], None]: @@ -144,7 +178,7 @@ def on_msg(msg: MsgT, topic: TopicT) -> None: # This is for ros for now -class DiscoveryPubSub(PubSub[TopicT, MsgT], ABC): +class DiscoveryPubSub(SubscribeLatestMixin[TopicT, MsgT], PubSub[TopicT, MsgT], ABC): """Mixin for PubSub that supports discovery of topics. Subclass from this if you support topic discovery (e.g. MQTT, Redis, NATS, RabbitMQ). @@ -187,5 +221,5 @@ class SubscribeAllCapable(Protocol[MsgT_co, TopicT_co]): """ def subscribe_all(self, callback: Callable[[Any, Any], Any]) -> Callable[[], None]: - """Subscribe to all messages on all topics.""" + """Subscribe to all messages on all topics (every message, no conflation).""" ... diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py new file mode 100644 index 0000000000..e19eb3e36d --- /dev/null +++ b/dimos/protocol/pubsub/spy.py @@ -0,0 +1,221 @@ +# Copyright 2026 Dimensional Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Transport-agnostic pubsub spy: topic discovery, rates, sizes, liveness. + +Design doc: docs/usage/transports/spy.md. Task spec: TASK.md (branch agent/spy-architect). + +HARD CONSTRAINT: the spy never decodes message payloads. Sources tap the +raw-bytes pubsub layer (LCMPubSubBase, ZenohPubSubBase — beneath the encoder +mixins), so the hot path per message is (topic string, payload length, +timestamp) and nothing else. Message *types* are still visible because they +are embedded in the topic string ("/cmd_vel#geometry_msgs.Twist"). + +Decoding is a separate, per-topic, opt-in concern: SpySource.subscribe_decoded +is the spec'd hook, not implemented in v1. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable + +if TYPE_CHECKING: + from collections.abc import Callable, Sequence + + # The entire hot-path event: (topic string incl. '#type' suffix, wire payload length). + TapCallback = Callable[[str, int], None] + + +@dataclass(frozen=True, slots=True) +class SpyKey: + """Identity of one spied topic: which transport saw it + its raw topic string.""" + + transport: str # SpySource.name, e.g. "lcm", "zenoh" + topic: str # raw transport topic, e.g. "/cmd_vel#geometry_msgs.Twist" + + +def split_type_suffix(topic: str) -> tuple[str, str | None]: + """Split a spied topic string into (base_topic, msg_type_name or None). + + Both LCM and zenoh sources deliver topics in the uniform str(Topic) form + "base#pkg.Msg" (zenoh keys are converted back by _key_expr_to_topic). + Render-time helper — never called on the hot path. + + >>> split_type_suffix("/cmd_vel#geometry_msgs.Twist") + ('/cmd_vel', 'geometry_msgs.Twist') + >>> split_type_suffix("/plain") + ('/plain', None) + """ + raise NotImplementedError + + +class TopicStats: + """Sliding-window traffic statistics for one spied topic. + + Records only (timestamp, nbytes) pairs — no payloads are retained. + Timestamps are passed in explicitly (callers use time.time(); tests inject + values), so all stats are deterministic functions of recorded data. + + Thread-safety: record() may be called from transport threads while readers + query from a UI thread. + """ + + total_bytes: int + total_msgs: int + last_seen: float | None # timestamp of newest recorded message, None if none yet + + def __init__(self, history_window: float = 60.0) -> None: + """history_window: seconds of per-message history kept for windowed stats. + + total_bytes/total_msgs/last_seen survive eviction; only windowed stats + (freq/bytes_per_sec/avg_size) forget evicted messages. + """ + raise NotImplementedError + + def record(self, nbytes: int, timestamp: float) -> None: + """Hot path: O(1) amortized append + eviction of entries older than history_window.""" + raise NotImplementedError + + def freq(self, window: float, now: float) -> float: + """Messages per second over [now - window, now]. 0.0 if none.""" + raise NotImplementedError + + def bytes_per_sec(self, window: float, now: float) -> float: + """Payload bytes per second over [now - window, now]. 0.0 if none.""" + raise NotImplementedError + + def avg_size(self, window: float, now: float) -> float: + """Mean payload size in bytes over [now - window, now]. 0.0 if none.""" + raise NotImplementedError + + +@runtime_checkable +class SpySource(Protocol): + """One transport's raw firehose feeding the spy. + + Invariants for implementations: + - tap() delivers EVERY observable message exactly once per callback + (non-conflating; built on the raw-bytes bus's subscribe_all). + - The tap callback receives (topic_str, nbytes) where topic_str is the + uniform str(Topic) form and nbytes is the wire payload length. + - Never decodes payloads, never retains them past the callback. + - start() is required before tap() delivers; stop() releases the bus. + """ + + name: str + + def start(self) -> None: ... + + def stop(self) -> None: ... + + def tap(self, callback: Callable[[str, int], None]) -> Callable[[], None]: ... + + def subscribe_decoded( + self, topic: str, callback: Callable[[Any], None] + ) -> Callable[[], None]: ... + + +class LCMSpySource: + """Spy source over LCM, via LCMPubSubBase.subscribe_all (raw regex '.*'). + + Delivers raw channel strings incl. the '#pkg.Msg' suffix. Payloads are the + LCM-encoded bytes; nbytes = len(payload). + """ + + name = "lcm" + + def __init__(self, **lcm_kwargs: Any) -> None: + """lcm_kwargs forwarded to LCMPubSubBase (e.g. lcm_url).""" + raise NotImplementedError + + def start(self) -> None: + raise NotImplementedError + + def stop(self) -> None: + raise NotImplementedError + + def tap(self, callback: Callable[[str, int], None]) -> Callable[[], None]: + raise NotImplementedError + + def subscribe_decoded(self, topic: str, callback: Callable[[Any], None]) -> Callable[[], None]: + """Opt-in per-topic decoded tap — OFF the spy hot path. Not implemented in v1.""" + raise NotImplementedError + + +class ZenohSpySource: + """Spy source over zenoh, via ZenohPubSubBase.subscribe_all (all keys, '**'). + + Depends on the subscribe_all non-conflation fix (see TASK.md): every sample + must reach the tap. nbytes = payload length; topics arrive as str(Topic) + with the type suffix reconstructed from the key expression. + """ + + name = "zenoh" + + def __init__(self, **zenoh_kwargs: Any) -> None: + """zenoh_kwargs forwarded to ZenohPubSubBase (e.g. mode/connect/listen, session_pool).""" + raise NotImplementedError + + def start(self) -> None: + raise NotImplementedError + + def stop(self) -> None: + raise NotImplementedError + + def tap(self, callback: Callable[[str, int], None]) -> Callable[[], None]: + raise NotImplementedError + + def subscribe_decoded(self, topic: str, callback: Callable[[Any], None]) -> Callable[[], None]: + """Opt-in per-topic decoded tap — OFF the spy hot path. Not implemented in v1.""" + raise NotImplementedError + + +class TransportSpy: + """Aggregates SpySources into per-(transport, topic) stats plus global totals. + + Owns the sources' lifecycle: start() starts every source and taps it; + stop() untaps and stops them. Stats rows appear lazily as topics are first + seen (a topic with no traffic since start is invisible — the spy observes, + it does not enumerate). + + Thread-safety: tap callbacks arrive on transport threads; snapshot() may be + called from any thread and returns a consistent view for rendering. + """ + + totals: TopicStats # all messages across all sources + + def __init__( + self, sources: Sequence[SpySource] | None = None, history_window: float = 60.0 + ) -> None: + """sources=None means default_sources().""" + raise NotImplementedError + + def start(self) -> None: + raise NotImplementedError + + def stop(self) -> None: + raise NotImplementedError + + def snapshot(self) -> dict[SpyKey, TopicStats]: + """Current per-topic stats, safe to iterate while messages keep arriving.""" + raise NotImplementedError + + +def default_sources() -> list[SpySource]: + """The spy observes ALL transports simultaneously, regardless of DIMOS_TRANSPORT. + + v1: [LCMSpySource(), ZenohSpySource()]. SHM/ROS/DDS/Redis are future sources. + """ + raise NotImplementedError diff --git a/dimos/protocol/pubsub/test_spy.py b/dimos/protocol/pubsub/test_spy.py new file mode 100644 index 0000000000..a0f37e9128 --- /dev/null +++ b/dimos/protocol/pubsub/test_spy.py @@ -0,0 +1,461 @@ +# Copyright 2026 Dimensional Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Contract tests for `dimos spy` (TASK.md on branch agent/spy-architect). + +These encode the acceptance criteria and FAIL until the task is implemented: +- TopicStats: deterministic windowed stats from injected timestamps. +- subscribe_all: delivers EVERY message on LCM and zenoh (non-conflating). +- subscribe_latest: conflates to the newest message per topic. +- SpySources: count every message without ever decoding a payload. +- TransportSpy: merges sources into (transport, topic)-keyed stats + totals. +- subscribe_decoded: spec'd hook, must stay NotImplementedError in v1. +""" + +from collections.abc import Callable +from contextlib import contextmanager +import threading +import time +from typing import Any + +import pytest + +from dimos.msgs.geometry_msgs.Vector3 import Vector3 +from dimos.protocol.pubsub.impl.lcmpubsub import LCMPubSubBase, Topic +from dimos.protocol.pubsub.impl.zenohpubsub import ZenohPubSubBase +from dimos.protocol.pubsub.spec import AllPubSub +from dimos.protocol.pubsub.spy import ( + LCMSpySource, + SpyKey, + TopicStats, + TransportSpy, + ZenohSpySource, + split_type_suffix, +) +from dimos.protocol.service.zenohservice import ZenohSessionPool + +VEC = Vector3(1.0, 2.0, 3.0) +VEC_BYTES = VEC.lcm_encode() + + +# --------------------------------------------------------------------------- +# TopicStats: pure, deterministic (injected timestamps, no sleeps) +# --------------------------------------------------------------------------- + + +def test_topic_stats_counts_and_totals(): + s = TopicStats(history_window=60.0) + t0 = 1000.0 + for i in range(10): + s.record(100, t0 + i * 0.1) + assert s.total_msgs == 10 + assert s.total_bytes == 1000 + assert s.last_seen == pytest.approx(t0 + 0.9) + + +def test_topic_stats_windowed_rates(): + s = TopicStats(history_window=60.0) + t0 = 1000.0 + for i in range(10): # 1 Hz, 50 bytes each + s.record(50, t0 + i) + now = t0 + 9.0 + assert s.freq(5.0, now) == pytest.approx(1.0, rel=0.25) + assert s.bytes_per_sec(5.0, now) == pytest.approx(50.0, rel=0.25) + assert s.avg_size(5.0, now) == pytest.approx(50.0) + + +def test_topic_stats_empty(): + s = TopicStats() + assert s.total_msgs == 0 + assert s.total_bytes == 0 + assert s.last_seen is None + assert s.freq(5.0, now=123.0) == 0.0 + assert s.bytes_per_sec(5.0, now=123.0) == 0.0 + assert s.avg_size(5.0, now=123.0) == 0.0 + + +def test_topic_stats_history_eviction_keeps_totals(): + s = TopicStats(history_window=60.0) + s.record(100, 0.0) + s.record(100, 1000.0) # first record is now far outside the history window + assert s.freq(60.0, now=1000.0) == pytest.approx(1 / 60.0) + assert s.total_msgs == 2 # totals survive eviction + assert s.total_bytes == 200 + + +# --------------------------------------------------------------------------- +# split_type_suffix: render-time topic parsing +# --------------------------------------------------------------------------- + + +def test_split_type_suffix(): + assert split_type_suffix("/cmd_vel#geometry_msgs.Twist") == ("/cmd_vel", "geometry_msgs.Twist") + assert split_type_suffix("dimos/cmd_vel#geometry_msgs.Twist") == ( + "dimos/cmd_vel", + "geometry_msgs.Twist", + ) + assert split_type_suffix("/plain") == ("/plain", None) + + +# --------------------------------------------------------------------------- +# subscribe_all contract: EVERY message, no conflation (spec-level fix) +# --------------------------------------------------------------------------- + + +@contextmanager +def lcm_base_ctx(): + bus = LCMPubSubBase() + bus.start() + try: + yield bus, Topic("/spy_contract", Vector3) + finally: + bus.stop() + + +@contextmanager +def zenoh_base_ctx(): + pool = ZenohSessionPool() + bus = ZenohPubSubBase(session_pool=pool) + bus.start() + try: + yield bus, Topic("dimos/spy_contract", Vector3) + finally: + bus.stop() + pool.close_all() + + +@pytest.mark.parametrize("bus_ctx", [lcm_base_ctx, zenoh_base_ctx], ids=["lcm", "zenoh"]) +def test_subscribe_all_delivers_every_message(bus_ctx): + """A gated (slow) consumer must still receive every message eventually. + + This is the sharp edge of the non-conflation contract: zenoh's original + subscribe_all conflated to latest-per-topic and fails this with ~2 of 50. + """ + n = 50 + with bus_ctx() as (bus, topic): + gate = threading.Event() + got = [] + done = threading.Event() + + def cb(msg, t): + gate.wait(15.0) # simulate a consumer slower than the burst + got.append((str(t), len(msg))) + if len(got) >= n: + done.set() + + unsub = bus.subscribe_all(cb) + time.sleep(0.5) # let the wildcard subscription establish + + for _ in range(n): + bus.publish(topic, VEC_BYTES) + gate.set() + + done.wait(10.0) + ours = [g for g in got if g[0] == str(topic)] + assert len(ours) == n + assert all(size == len(VEC_BYTES) for _, size in ours) + unsub() + + +# --------------------------------------------------------------------------- +# subscribe_latest contract: conflation is the explicit opt-in +# --------------------------------------------------------------------------- + + +class FakeAllPubSub(AllPubSub[str, bytes]): + """In-memory AllPubSub: publish() synchronously fans out to subscribers.""" + + def __init__(self): + self._subs: list[Callable[[bytes, str], None]] = [] + + def publish(self, topic: str, message: bytes) -> None: + for cb in list(self._subs): + cb(message, topic) + + def subscribe(self, topic, callback): + return self.subscribe_all(callback) + + def subscribe_all(self, callback): + self._subs.append(callback) + + def unsub(): + if callback in self._subs: + self._subs.remove(callback) + + return unsub + + +def test_subscribe_latest_conflates_to_newest(): + bus = FakeAllPubSub() + delivered = [] + first = threading.Event() + gate = threading.Event() + quiet = threading.Event() + + def cb(msg, topic): + delivered.append((topic, msg)) + first.set() + gate.wait(10.0) # block the drain thread while the burst happens + if msg == b"99": + quiet.set() + + unsub = bus.subscribe_latest(cb) + bus.publish("t", b"0") + assert first.wait(5.0) + for i in range(1, 100): + bus.publish("t", str(i).encode()) + gate.set() + + assert quiet.wait(5.0) + # Newest message must arrive; the 98 intermediate ones conflate away. + assert delivered[-1] == ("t", b"99") + assert len(delivered) <= 5 + unsub() + + +def test_subscribe_latest_keeps_latest_per_topic(): + bus = FakeAllPubSub() + delivered = [] + first = threading.Event() + gate = threading.Event() + done = threading.Event() + + def cb(msg, topic): + delivered.append((topic, msg)) + first.set() + gate.wait(10.0) + if {t for t, _ in delivered} >= {"a", "b"} and len(delivered) >= 3: + done.set() + + unsub = bus.subscribe_latest(cb) + bus.publish("a", b"a0") + assert first.wait(5.0) + for i in range(1, 50): + bus.publish("a", f"a{i}".encode()) + bus.publish("b", f"b{i}".encode()) + gate.set() + + assert done.wait(5.0) + latest = dict(delivered) # last write per topic wins + assert latest["a"] == b"a49" + assert latest["b"] == b"b49" + unsub() + + +def test_subscribe_latest_unsubscribe_stops_delivery(): + bus = FakeAllPubSub() + delivered = [] + got_one = threading.Event() + + def cb(msg, topic): + delivered.append(msg) + got_one.set() + + unsub = bus.subscribe_latest(cb) + bus.publish("t", b"1") + assert got_one.wait(5.0) + unsub() + count = len(delivered) + bus.publish("t", b"2") + time.sleep(0.2) + assert len(delivered) == count + + +# --------------------------------------------------------------------------- +# SpySources: every message counted, payloads never decoded +# --------------------------------------------------------------------------- + + +class _TapCollector: + def __init__(self, topic_str: str, n: int): + self.topic_str = topic_str + self.n = n + self.events: list[tuple[str, int]] = [] + self.done = threading.Event() + + def __call__(self, topic: str, nbytes: int) -> None: + self.events.append((topic, nbytes)) + if len(self.ours()) >= self.n: + self.done.set() + + def ours(self) -> list[tuple[str, int]]: + return [e for e in self.events if e[0] == self.topic_str] + + +def _assert_no_decode(monkeypatch): + """Make any Vector3 decode explode — the spy must never trigger one.""" + + def boom(*a, **k): + raise AssertionError("spy decoded a payload on the hot path") + + monkeypatch.setattr(Vector3, "lcm_decode", staticmethod(boom)) + + +def test_lcm_source_counts_all_without_decoding(monkeypatch): + _assert_no_decode(monkeypatch) + topic = Topic("/spy_e2e", Vector3) + collector = _TapCollector(str(topic), 20) + + src = LCMSpySource() + src.start() + untap = src.tap(collector) + pub = LCMPubSubBase() + pub.start() + try: + time.sleep(0.3) + for _ in range(20): + pub.publish(topic, VEC_BYTES) + time.sleep(0.01) + assert collector.done.wait(10.0) + assert [n for _, n in collector.ours()] == [len(VEC_BYTES)] * 20 + finally: + untap() + src.stop() + pub.stop() + + +def test_lcm_source_counts_undecodable_garbage(monkeypatch): + """Payloads that would crash a decoder must still be counted (proves raw tap).""" + _assert_no_decode(monkeypatch) + topic = Topic("/spy_garbage", Vector3) + collector = _TapCollector(str(topic), 5) + + src = LCMSpySource() + src.start() + untap = src.tap(collector) + pub = LCMPubSubBase() + pub.start() + try: + time.sleep(0.3) + for _ in range(5): + pub.publish(topic, b"\x00garbage-not-lcm") + time.sleep(0.01) + assert collector.done.wait(10.0) + assert [n for _, n in collector.ours()] == [len(b"\x00garbage-not-lcm")] * 5 + finally: + untap() + src.stop() + pub.stop() + + +def test_zenoh_source_counts_all_without_decoding(monkeypatch): + _assert_no_decode(monkeypatch) + topic = Topic("dimos/spy_e2e", Vector3) + collector = _TapCollector(str(topic), 20) + + pool = ZenohSessionPool() + src = ZenohSpySource(session_pool=pool) + src.start() + untap = src.tap(collector) + pub = ZenohPubSubBase(session_pool=pool) + pub.start() + try: + time.sleep(0.5) + for _ in range(20): + pub.publish(topic, VEC_BYTES) + time.sleep(0.01) + assert collector.done.wait(10.0) + assert [n for _, n in collector.ours()] == [len(VEC_BYTES)] * 20 + finally: + untap() + src.stop() + pub.stop() + pool.close_all() + + +# --------------------------------------------------------------------------- +# TransportSpy: merging + totals + lifecycle (fake sources, deterministic) +# --------------------------------------------------------------------------- + + +class FakeSource: + def __init__(self, name: str): + self.name = name + self.started = False + self.taps: list[Callable[[str, int], None]] = [] + + def start(self) -> None: + self.started = True + + def stop(self) -> None: + self.started = False + + def tap(self, callback): + self.taps.append(callback) + + def untap(): + if callback in self.taps: + self.taps.remove(callback) + + return untap + + def subscribe_decoded(self, topic: str, callback: Callable[[Any], None]): + raise NotImplementedError + + def emit(self, topic: str, nbytes: int) -> None: + for cb in list(self.taps): + cb(topic, nbytes) + + +def test_transport_spy_merges_sources_and_totals(): + a, b = FakeSource("lcm"), FakeSource("zenoh") + spy = TransportSpy(sources=[a, b]) + spy.start() + assert a.started and b.started + + a.emit("/t#geometry_msgs.Twist", 10) + a.emit("/t#geometry_msgs.Twist", 10) + b.emit("dimos/t#geometry_msgs.Twist", 20) + a.emit("/u", 5) + + snap = spy.snapshot() + assert snap[SpyKey("lcm", "/t#geometry_msgs.Twist")].total_msgs == 2 + assert snap[SpyKey("lcm", "/t#geometry_msgs.Twist")].total_bytes == 20 + assert snap[SpyKey("zenoh", "dimos/t#geometry_msgs.Twist")].total_msgs == 1 + assert snap[SpyKey("lcm", "/u")].total_bytes == 5 + assert spy.totals.total_msgs == 4 + assert spy.totals.total_bytes == 45 + + spy.stop() + assert not a.started and not b.started + assert not a.taps and not b.taps # untapped on stop + + +def test_transport_spy_snapshot_is_stable_copy(): + a = FakeSource("lcm") + spy = TransportSpy(sources=[a]) + spy.start() + a.emit("/t", 1) + snap = spy.snapshot() + a.emit("/new_topic_after_snapshot", 1) + assert SpyKey("lcm", "/new_topic_after_snapshot") not in snap # snapshot doesn't mutate + assert SpyKey("lcm", "/new_topic_after_snapshot") in spy.snapshot() + spy.stop() + + +def test_fake_source_satisfies_protocol(): + from dimos.protocol.pubsub.spy import SpySource + + assert isinstance(FakeSource("x"), SpySource) + + +# --------------------------------------------------------------------------- +# Lazy decode hook: spec'd now, implemented in a follow-up (stays off hot path) +# --------------------------------------------------------------------------- + + +def test_subscribe_decoded_is_not_implemented_in_v1(): + src = LCMSpySource() + with pytest.raises(NotImplementedError): + src.subscribe_decoded("/x#geometry_msgs.Vector3", lambda m: None) diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index d7615a2795..ea7526a04e 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -620,6 +620,15 @@ def list_blueprints() -> None: typer.echo(blueprint_name) +@main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) +def spy(ctx: typer.Context) -> None: + """Universal transport spy: topics, rates, sizes across all pubsub transports.""" + from dimos.utils.cli.spy.run_spy import main as spy_main + + sys.argv = ["spy", *ctx.args] + spy_main() + + @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def lcmspy(ctx: typer.Context) -> None: """LCM spy tool for monitoring LCM messages.""" diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py new file mode 100644 index 0000000000..5bc9335844 --- /dev/null +++ b/dimos/utils/cli/spy/run_spy.py @@ -0,0 +1,35 @@ +# Copyright 2026 Dimensional Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""`dimos spy` TUI: live table of all topics across all pubsub transports. + +Textual app modeled on run_lcmspy.py (DataTable, 0.5s refresh, theme colors, +'q' to quit, optional `spy web` mode via textual-serve). One row per +(transport, topic) from TransportSpy.snapshot(): + + Transport | Topic | Type | Freq (Hz) | Bandwidth | Total | Age + +- Topic/Type come from split_type_suffix(); rows sort by total traffic. +- Age is seconds since TopicStats.last_seen (liveness: dims/greys out stale rows). +- `--transport lcm --transport zenoh` (repeatable) filters sources; default all. +- LCM system config warning: call lcmservice.autoconf(check_only=True) before + entering raw TUI mode, like run_lcmspy does. +""" + +from __future__ import annotations + + +def main() -> None: + """Entry point for `dimos spy` (argv: optional 'web', --transport filters).""" + raise NotImplementedError diff --git a/docs/docs.json b/docs/docs.json index 7cdbc96204..f1cb8c2b86 100644 --- a/docs/docs.json +++ b/docs/docs.json @@ -73,7 +73,8 @@ "group": "Transports", "pages": [ "usage/transports/index", - "usage/transports/dds" + "usage/transports/dds", + "usage/transports/spy" ] } ] diff --git a/docs/usage/transports/spy.md b/docs/usage/transports/spy.md new file mode 100644 index 0000000000..d80a05b005 --- /dev/null +++ b/docs/usage/transports/spy.md @@ -0,0 +1,104 @@ +--- +title: "dimos spy" +--- + +`dimos spy` is the universal transport spy: one live view of every topic moving on every +DimOS pubsub transport — names, message rates, bandwidth, sizes, and liveness — regardless +of whether the system runs on LCM, Zenoh, or both. + +> Status: design ratified, implementation in progress. This page is the design doc; +> it becomes the user doc as the tool lands. + +## Motivation + +`dimos lcmspy` only sees LCM. Since the Zenoh transport merged (#2362), a robot can run +with `--transport=zenoh` (default on macOS) and the spy goes blind. Debugging "is data +flowing?" should not require knowing which backend is active — the spy should observe +everything at once. + +## The hard constraint: no decoding on the hot path + +Decoding messages is expensive (LCM decode, image payloads, pointclouds). The spy's job +is *flow* visibility — topic names, Hz, bytes/s, last-seen — none of which needs message +contents. Therefore: + +**The spy never decodes a payload.** Its per-message hot path is +`(topic string, payload length, timestamp)` — an O(1) stats update. + +This falls out of the pubsub layering: encoding/decoding lives in `PubSubEncoderMixin` +subclasses (`LCM`, `Zenoh`), while the base classes (`LCMPubSubBase`, `ZenohPubSubBase`) +move raw bytes. The spy taps the base layer. Message *types* are still displayed for +free because DimOS embeds them in topic strings (`/cmd_vel#geometry_msgs.Twist` on LCM, +`dimos/cmd_vel/geometry_msgs.Twist` as a Zenoh key — both normalize to the `#` form via +`str(Topic)`). + +Per-topic decoded inspection (echo/preview) is a separate, opt-in concern with its own +subscription (`SpySource.subscribe_decoded`), spec'd now and implemented in a follow-up. + +## Architecture + +``` +LCMPubSubBase.subscribe_all(".*") ZenohPubSubBase.subscribe_all("**") + │ raw bytes │ raw bytes + LCMSpySource ("lcm") ZenohSpySource ("zenoh") + │ (topic_str, nbytes) │ (topic_str, nbytes) + └──────────────┬───────────────────────┘ + TransportSpy + dict[SpyKey(transport, topic) → TopicStats] + totals + │ snapshot() + dimos spy TUI + Transport | Topic | Type | Freq | Bandwidth | Total | Age +``` + +- **`SpySource`** (Protocol): one transport's raw firehose. `tap(cb)` delivers + `(topic, nbytes)` for *every* observable message. v1 ships `lcm` and `zenoh` sources; + SHM/ROS/DDS/Redis plug in later by implementing the protocol. +- **`TopicStats`**: sliding-window `(timestamp, nbytes)` history per topic — freq, + bytes/s, avg size over a window, plus lifetime totals and `last_seen` for liveness. + Explicit timestamps keep it deterministic under test. +- **`TransportSpy`**: owns sources, merges taps, serves thread-safe snapshots to the UI. + +Code: `dimos/protocol/pubsub/spy.py` (core), `dimos/utils/cli/spy/run_spy.py` (TUI). + +## The subscribe_all contract fix + +The spy builds on the pubsub layer's subscribe-all abstraction +(`dimos/protocol/pubsub/spec.py`), which exposed a semantic asymmetry: + +- LCM's `subscribe_all` delivered every message (regex `.*`). +- Zenoh's `subscribe_all` **conflated** — latest-wins per topic with a drain thread, + scoped to `dimos/**` — because it was built for its only consumer, the rerun bridge. + +A spy counting rates through a conflating tap silently under-reports. Ratified decision +(Ivan, ticket f6c74d39): **`subscribe_all` is defined non-conflating on every +transport.** Conflation becomes the explicit opt-in `subscribe_latest()` (same +latest-wins + drain-thread semantics, hosted in the spec as a default), and the rerun +bridge migrates to it. Zenoh's `subscribe_all` also widens from `dimos/**` to `**` — +"all topics" means everything the transport can see; the rerun bridge is unaffected +because unknown/untyped keys already fail type resolution and are skipped by its decoder. + +## Decisions (ticket f6c74d39) + +| # | Decision | +|---|----------| +| 1 | `subscribe_all` = every message, non-conflating, on all transports; conflation moves to opt-in `subscribe_latest()`; rerun bridge migrates. Confirmed by Ivan. | +| 2 | v1 sources: LCM + Zenoh, behind the pluggable `SpySource` protocol. SHM/ROS/DDS/Redis later (SHM needs a discovery mechanism first). | +| 3 | `dimos spy` shows **all transports simultaneously**, rows tagged by transport; `dimos lcmspy` becomes a deprecated alias. | +| 4 | Lazy per-topic decode is spec'd (`subscribe_decoded`) but not implemented in v1 — decode stays off the hot path. | +| 5 | This design doc lives in dimos `docs/`. | + +## CLI + +```bash +dimos spy # everything, all transports +dimos spy --transport zenoh # filter to one transport (repeatable flag) +dimos spy web # serve the TUI in a browser (textual-serve) +dimos lcmspy # deprecated alias for: dimos spy --transport lcm +``` + +## Future work + +- SHM source (needs channel discovery), ROS/DDS/Redis sources. +- Per-topic decoded echo/preview from the TUI row (`subscribe_decoded` follow-up). +- Zero-copy zenoh sizing (`len(sample.payload)` without `to_bytes()`), if profiling + shows the copy matters for pointcloud-heavy systems. From 7c7720c10e818dfdc5b30610c1491ae8a5105102 Mon Sep 17 00:00:00 2001 From: "brain (osvetnik)" Date: Mon, 6 Jul 2026 19:15:23 +0300 Subject: [PATCH 02/33] =?UTF-8?q?feat:=20dimos=20spy=20=E2=80=94=20univers?= =?UTF-8?q?al=20transport=20spy=20over=20LCM=20+=20Zenoh?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement the spy core (TopicStats, LCM/Zenoh SpySources, TransportSpy) that taps the raw-bytes pubsub layer — per-message hot path is (topic, len, ts), never decoding a payload — plus the Textual TUI (dimos spy) and CLI wiring. Spec fix: subscribe_all is now non-conflating on every transport; conflation moves to the opt-in SubscribeLatestMixin.subscribe_latest() (lifted from zenoh, transport-agnostic). Zenoh subscribe_all widens dimos/** -> ** and keeps its drain-stop bookkeeping via _register/_unregister_drain_stop hooks. Rerun bridge migrates to subscribe_latest. dimos lcmspy becomes a deprecated alias for dimos spy --transport lcm. Also strip 14 decorative section-banner comments from the delivered test_spy.py to satisfy codebase_checks/test_no_sections (approved by brain; no logic change). --- dimos/protocol/pubsub/impl/zenohpubsub.py | 66 ++----- dimos/protocol/pubsub/spec.py | 70 ++++++- dimos/protocol/pubsub/spy.py | 92 ++++++++-- dimos/protocol/pubsub/test_spy.py | 14 -- dimos/robot/cli/dimos.py | 9 +- dimos/utils/cli/spy/run_spy.py | 172 +++++++++++++++++- dimos/visualization/rerun/bridge.py | 2 +- .../rerun/test_viewer_integration.py | 3 + 8 files changed, 340 insertions(+), 88 deletions(-) diff --git a/dimos/protocol/pubsub/impl/zenohpubsub.py b/dimos/protocol/pubsub/impl/zenohpubsub.py index afaa413c43..466eff6e1a 100644 --- a/dimos/protocol/pubsub/impl/zenohpubsub.py +++ b/dimos/protocol/pubsub/impl/zenohpubsub.py @@ -197,61 +197,31 @@ def unsubscribe() -> None: return unsubscribe def subscribe_all(self, callback: Callable[[bytes, Topic], Any]) -> Callable[[], None]: - """Subscribe to all dimos topics, delivering only the latest per topic. + """Subscribe to every key, delivering every message (non-conflating). - Unlike `subscribe`, this is best effort. If it's done otherwise, rerun lags behind. + "All topics" means everything this session can observe ('**'). Consumers + that only want the newest message per topic use subscribe_latest(). """ - latest: dict[str, tuple[bytes, Topic]] = {} - lock = threading.Lock() - wake = threading.Event() - stop = threading.Event() - - def collect(msg: bytes, topic: Topic) -> None: - # Fast path on the Zenoh delivery thread: keep only the newest per topic. - with lock: - latest[str(topic)] = (msg, topic) - wake.set() - - def drain() -> None: - while not stop.is_set(): - wake.wait() - wake.clear() - with lock: - batch = list(latest.values()) - latest.clear() - for msg, topic in batch: - try: - callback(msg, topic) - except Exception: - logger.error("Error in subscribe_all callback", exc_info=True) - - thread = threading.Thread(target=drain, name="zenoh-subscribe-all", daemon=True) - - def stop_drain() -> None: - stop.set() - wake.set() # unblock the drain so it observes the stop flag - thread.join(timeout=2.0) - - # Register the stop callback and launch the drain thread under the lock so - # stop() can't run between them and miss the thread. If stop() already ran, - # bail without starting anything. + return self.subscribe(Topic("**"), callback) + + def _register_drain_stop( + self, stop_drain: Callable[[], None], thread: threading.Thread + ) -> bool: + # Register + start under the subscriber lock so stop() can't run between + # them and miss the thread. If stop() already ran, bail without starting. with self._subscriber_lock: if self._stopped: - return lambda: None + return False self._drain_stops.append(stop_drain) thread.start() + return True - inner_unsub = self.subscribe(Topic("dimos/**"), collect) - - def unsubscribe() -> None: - with self._subscriber_lock: - if stop_drain not in self._drain_stops: - return # Already removed by stop() or a concurrent unsubscribe - self._drain_stops.remove(stop_drain) - inner_unsub() - stop_drain() - - return unsubscribe + def _unregister_drain_stop(self, stop_drain: Callable[[], None]) -> bool: + with self._subscriber_lock: + if stop_drain not in self._drain_stops: + return False # Already removed by stop() or a concurrent unsubscribe + self._drain_stops.remove(stop_drain) + return True def stop(self) -> None: with self._subscriber_lock: diff --git a/dimos/protocol/pubsub/spec.py b/dimos/protocol/pubsub/spec.py index 96a3bf6f4a..1652621417 100644 --- a/dimos/protocol/pubsub/spec.py +++ b/dimos/protocol/pubsub/spec.py @@ -17,8 +17,13 @@ from collections.abc import AsyncIterator, Callable from contextlib import asynccontextmanager from dataclasses import dataclass +import threading from typing import Any, Generic, Protocol, TypeVar, runtime_checkable +from dimos.utils.logging_config import setup_logger + +logger = setup_logger() + MsgT = TypeVar("MsgT") TopicT = TypeVar("TopicT") MsgT_co = TypeVar("MsgT_co", covariant=True) @@ -124,6 +129,22 @@ class SubscribeLatestMixin(Generic[TopicT, MsgT], ABC): @abstractmethod def subscribe_all(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: ... + def _register_drain_stop( + self, stop_drain: Callable[[], None], thread: threading.Thread + ) -> bool: + """Start ``thread`` and record ``stop_drain`` for transport-level shutdown. + + Default: just start the thread. Transports whose stop() must join live + drains (e.g. zenoh) override this to register under their own lock and + return False if already stopped (subscribe_latest then bails). + """ + thread.start() + return True + + def _unregister_drain_stop(self, stop_drain: Callable[[], None]) -> bool: + """Forget a drain stopper. Returns False if already removed.""" + return True + def subscribe_latest(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: """Subscribe to all topics, conflated: newest message per topic wins. @@ -140,7 +161,50 @@ def subscribe_latest(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[ - The returned callable unsubscribes the underlying subscribe_all and stops+joins the drain thread. """ - raise NotImplementedError + latest: dict[str, tuple[MsgT, TopicT]] = {} + lock = threading.Lock() + wake = threading.Event() + stop = threading.Event() + + def collect(msg: MsgT, topic: TopicT) -> None: + # Fast path on the transport's delivery thread: keep only newest per topic. + with lock: + latest[str(topic)] = (msg, topic) + wake.set() + + def drain() -> None: + while not stop.is_set(): + wake.wait() + wake.clear() + with lock: + batch = list(latest.values()) + latest.clear() + for msg, topic in batch: + try: + callback(msg, topic) + except Exception: + logger.error("Error in subscribe_latest callback", exc_info=True) + + thread = threading.Thread(target=drain, name="subscribe-latest-drain", daemon=True) + + def stop_drain() -> None: + stop.set() + wake.set() # unblock the drain so it observes the stop flag + thread.join(timeout=2.0) + + # Register + start the drain atomically w.r.t. transport shutdown, then + # subscribe. If the transport already stopped, bail without subscribing. + if not self._register_drain_stop(stop_drain, thread): + return lambda: None + inner_unsub = self.subscribe_all(collect) + + def unsubscribe() -> None: + if not self._unregister_drain_stop(stop_drain): + return # Already removed by stop() or a concurrent unsubscribe + inner_unsub() + stop_drain() + + return unsubscribe class AllPubSub(SubscribeLatestMixin[TopicT, MsgT], PubSub[TopicT, MsgT], ABC): @@ -223,3 +287,7 @@ class SubscribeAllCapable(Protocol[MsgT_co, TopicT_co]): def subscribe_all(self, callback: Callable[[Any, Any], Any]) -> Callable[[], None]: """Subscribe to all messages on all topics (every message, no conflation).""" ... + + def subscribe_latest(self, callback: Callable[[Any, Any], Any]) -> Callable[[], None]: + """Subscribe to all topics, conflated to the newest message per topic.""" + ... diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index e19eb3e36d..1e6086630f 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -28,7 +28,10 @@ from __future__ import annotations +from collections import deque from dataclasses import dataclass +import threading +import time from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable if TYPE_CHECKING: @@ -58,7 +61,8 @@ def split_type_suffix(topic: str) -> tuple[str, str | None]: >>> split_type_suffix("/plain") ('/plain', None) """ - raise NotImplementedError + base, sep, suffix = topic.partition("#") + return (base, suffix) if sep else (topic, None) class TopicStats: @@ -82,23 +86,43 @@ def __init__(self, history_window: float = 60.0) -> None: total_bytes/total_msgs/last_seen survive eviction; only windowed stats (freq/bytes_per_sec/avg_size) forget evicted messages. """ - raise NotImplementedError + self.history_window = history_window + self._history: deque[tuple[float, int]] = deque() # (timestamp, nbytes) + self._lock = threading.Lock() + self.total_bytes = 0 + self.total_msgs = 0 + self.last_seen = None def record(self, nbytes: int, timestamp: float) -> None: """Hot path: O(1) amortized append + eviction of entries older than history_window.""" - raise NotImplementedError + with self._lock: + self._history.append((timestamp, nbytes)) + self.total_bytes += nbytes + self.total_msgs += 1 + self.last_seen = timestamp + cutoff = timestamp - self.history_window + while self._history and self._history[0][0] < cutoff: + self._history.popleft() + + def _in_window(self, window: float, now: float) -> list[tuple[float, int]]: + cutoff = now - window + with self._lock: + return [(ts, n) for ts, n in self._history if ts >= cutoff] def freq(self, window: float, now: float) -> float: """Messages per second over [now - window, now]. 0.0 if none.""" - raise NotImplementedError + msgs = self._in_window(window, now) + return len(msgs) / window if msgs else 0.0 def bytes_per_sec(self, window: float, now: float) -> float: """Payload bytes per second over [now - window, now]. 0.0 if none.""" - raise NotImplementedError + msgs = self._in_window(window, now) + return sum(n for _, n in msgs) / window if msgs else 0.0 def avg_size(self, window: float, now: float) -> float: """Mean payload size in bytes over [now - window, now]. 0.0 if none.""" - raise NotImplementedError + msgs = self._in_window(window, now) + return sum(n for _, n in msgs) / len(msgs) if msgs else 0.0 @runtime_checkable @@ -138,16 +162,18 @@ class LCMSpySource: def __init__(self, **lcm_kwargs: Any) -> None: """lcm_kwargs forwarded to LCMPubSubBase (e.g. lcm_url).""" - raise NotImplementedError + from dimos.protocol.pubsub.impl.lcmpubsub import LCMPubSubBase + + self._bus = LCMPubSubBase(**lcm_kwargs) def start(self) -> None: - raise NotImplementedError + self._bus.start() def stop(self) -> None: - raise NotImplementedError + self._bus.stop() def tap(self, callback: Callable[[str, int], None]) -> Callable[[], None]: - raise NotImplementedError + return self._bus.subscribe_all(lambda msg, topic: callback(str(topic), len(msg))) def subscribe_decoded(self, topic: str, callback: Callable[[Any], None]) -> Callable[[], None]: """Opt-in per-topic decoded tap — OFF the spy hot path. Not implemented in v1.""" @@ -166,16 +192,18 @@ class ZenohSpySource: def __init__(self, **zenoh_kwargs: Any) -> None: """zenoh_kwargs forwarded to ZenohPubSubBase (e.g. mode/connect/listen, session_pool).""" - raise NotImplementedError + from dimos.protocol.pubsub.impl.zenohpubsub import ZenohPubSubBase + + self._bus = ZenohPubSubBase(**zenoh_kwargs) def start(self) -> None: - raise NotImplementedError + self._bus.start() def stop(self) -> None: - raise NotImplementedError + self._bus.stop() def tap(self, callback: Callable[[str, int], None]) -> Callable[[], None]: - raise NotImplementedError + return self._bus.subscribe_all(lambda msg, topic: callback(str(topic), len(msg))) def subscribe_decoded(self, topic: str, callback: Callable[[Any], None]) -> Callable[[], None]: """Opt-in per-topic decoded tap — OFF the spy hot path. Not implemented in v1.""" @@ -200,17 +228,43 @@ def __init__( self, sources: Sequence[SpySource] | None = None, history_window: float = 60.0 ) -> None: """sources=None means default_sources().""" - raise NotImplementedError + self._sources = list(sources) if sources is not None else default_sources() + self._history_window = history_window + self._stats: dict[SpyKey, TopicStats] = {} + self._lock = threading.Lock() + self._untaps: list[Callable[[], None]] = [] + self.totals = TopicStats(history_window=history_window) + + def _tap_callback(self, transport: str) -> Callable[[str, int], None]: + def on_message(topic: str, nbytes: int) -> None: + now = time.time() + key = SpyKey(transport, topic) + with self._lock: + stats = self._stats.get(key) + if stats is None: + stats = TopicStats(history_window=self._history_window) + self._stats[key] = stats + stats.record(nbytes, now) + self.totals.record(nbytes, now) + + return on_message def start(self) -> None: - raise NotImplementedError + for source in self._sources: + source.start() + self._untaps.append(source.tap(self._tap_callback(source.name))) def stop(self) -> None: - raise NotImplementedError + for untap in self._untaps: + untap() + self._untaps.clear() + for source in self._sources: + source.stop() def snapshot(self) -> dict[SpyKey, TopicStats]: """Current per-topic stats, safe to iterate while messages keep arriving.""" - raise NotImplementedError + with self._lock: + return dict(self._stats) def default_sources() -> list[SpySource]: @@ -218,4 +272,4 @@ def default_sources() -> list[SpySource]: v1: [LCMSpySource(), ZenohSpySource()]. SHM/ROS/DDS/Redis are future sources. """ - raise NotImplementedError + return [LCMSpySource(), ZenohSpySource()] diff --git a/dimos/protocol/pubsub/test_spy.py b/dimos/protocol/pubsub/test_spy.py index a0f37e9128..a61f5198aa 100644 --- a/dimos/protocol/pubsub/test_spy.py +++ b/dimos/protocol/pubsub/test_spy.py @@ -49,9 +49,7 @@ VEC_BYTES = VEC.lcm_encode() -# --------------------------------------------------------------------------- # TopicStats: pure, deterministic (injected timestamps, no sleeps) -# --------------------------------------------------------------------------- def test_topic_stats_counts_and_totals(): @@ -94,9 +92,7 @@ def test_topic_stats_history_eviction_keeps_totals(): assert s.total_bytes == 200 -# --------------------------------------------------------------------------- # split_type_suffix: render-time topic parsing -# --------------------------------------------------------------------------- def test_split_type_suffix(): @@ -108,9 +104,7 @@ def test_split_type_suffix(): assert split_type_suffix("/plain") == ("/plain", None) -# --------------------------------------------------------------------------- # subscribe_all contract: EVERY message, no conflation (spec-level fix) -# --------------------------------------------------------------------------- @contextmanager @@ -168,9 +162,7 @@ def cb(msg, t): unsub() -# --------------------------------------------------------------------------- # subscribe_latest contract: conflation is the explicit opt-in -# --------------------------------------------------------------------------- class FakeAllPubSub(AllPubSub[str, bytes]): @@ -272,9 +264,7 @@ def cb(msg, topic): assert len(delivered) == count -# --------------------------------------------------------------------------- # SpySources: every message counted, payloads never decoded -# --------------------------------------------------------------------------- class _TapCollector: @@ -374,9 +364,7 @@ def test_zenoh_source_counts_all_without_decoding(monkeypatch): pool.close_all() -# --------------------------------------------------------------------------- # TransportSpy: merging + totals + lifecycle (fake sources, deterministic) -# --------------------------------------------------------------------------- class FakeSource: @@ -450,9 +438,7 @@ def test_fake_source_satisfies_protocol(): assert isinstance(FakeSource("x"), SpySource) -# --------------------------------------------------------------------------- # Lazy decode hook: spec'd now, implemented in a follow-up (stays off hot path) -# --------------------------------------------------------------------------- def test_subscribe_decoded_is_not_implemented_in_v1(): diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index ea7526a04e..63a0682d05 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -631,11 +631,12 @@ def spy(ctx: typer.Context) -> None: @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def lcmspy(ctx: typer.Context) -> None: - """LCM spy tool for monitoring LCM messages.""" - from dimos.utils.cli.lcmspy.run_lcmspy import main as lcmspy_main + """Deprecated alias for `dimos spy --transport lcm`.""" + print("dimos lcmspy is deprecated; use `dimos spy --transport lcm`.", file=sys.stderr) + from dimos.utils.cli.spy.run_spy import main as spy_main - sys.argv = ["lcmspy", *ctx.args] - lcmspy_main() + sys.argv = ["spy", "--transport", "lcm", *ctx.args] + spy_main() @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 5bc9335844..c33f4c5b5e 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -29,7 +29,177 @@ from __future__ import annotations +import time + +from rich.text import Text +from textual.app import App, ComposeResult +from textual.color import Color +from textual.widgets import DataTable + +from dimos.protocol.pubsub.spy import ( + SpyKey, + TopicStats, + TransportSpy, + default_sources, + split_type_suffix, +) +from dimos.utils.cli import theme +from dimos.utils.human import human_bytes + +# Rows older than this (seconds since last message) render dimmed as "stale". +STALE_AGE = 3.0 +# Window for freq / bandwidth readouts. +STAT_WINDOW = 5.0 + + +def gradient(max_value: float, value: float) -> str: + """Gradient from cyan (low) to yellow (high) using DimOS theme colors.""" + ratio = min(value / max_value, 1.0) if max_value else 0.0 + cyan = Color.parse(theme.CYAN) + yellow = Color.parse(theme.YELLOW) + return cyan.blend(yellow, ratio).hex + + +def topic_text(base: str) -> Text: + """Format a base topic name with DimOS theme colors.""" + if base[:4] == "/rpc": + return Text(base[:4], style=theme.BLUE) + Text(base[4:], style=theme.BRIGHT_WHITE) + return Text(base, style=theme.BRIGHT_WHITE) + + +def _parse_transports(argv: list[str]) -> list[str] | None: + """Parse repeated --transport flags; None means all default sources.""" + transports: list[str] = [] + i = 0 + while i < len(argv): + arg = argv[i] + if arg == "--transport": + i += 1 + if i < len(argv): + transports.append(argv[i]) + elif arg.startswith("--transport="): + transports.append(arg.split("=", 1)[1]) + i += 1 + return transports or None + + +class SpyApp(App): # type: ignore[type-arg] + """A real-time dashboard for all-transport pubsub traffic using Textual.""" + + CSS_PATH = "../dimos.tcss" + + CSS = f""" + Screen {{ + layout: vertical; + background: {theme.BACKGROUND}; + }} + DataTable {{ + height: 2fr; + width: 1fr; + border: solid {theme.BORDER}; + background: {theme.BG}; + scrollbar-size: 0 0; + }} + DataTable > .datatable--header {{ + color: {theme.ACCENT}; + background: transparent; + }} + """ + + refresh_interval: float = 0.5 # seconds + + BINDINGS = [ + ("q", "quit"), + ("ctrl+c", "quit"), + ] + + def __init__(self, transports: list[str] | None = None, **kwargs) -> None: # type: ignore[no-untyped-def] + super().__init__(**kwargs) + sources = default_sources() + if transports is not None: + sources = [s for s in sources if s.name in transports] + # Warn about missing system config before entering TUI raw mode (LCM only). + if any(s.name == "lcm" for s in sources): + from dimos.protocol.service.lcmservice import autoconf + + autoconf(check_only=True) + + self.spy = TransportSpy(sources=sources) + self.spy.start() + self.table: DataTable | None = None # type: ignore[type-arg] + + def compose(self) -> ComposeResult: + self.table = DataTable(zebra_stripes=False, cursor_type=None) # type: ignore[arg-type] + self.table.add_column("Transport") + self.table.add_column("Topic") + self.table.add_column("Type") + self.table.add_column("Freq (Hz)") + self.table.add_column("Bandwidth") + self.table.add_column("Total") + self.table.add_column("Age") + yield self.table + + def on_mount(self) -> None: + self.set_interval(self.refresh_interval, self.refresh_table) + + async def on_unmount(self) -> None: + self.spy.stop() + + def refresh_table(self) -> None: + now = time.time() + snap = self.spy.snapshot() + rows: list[tuple[SpyKey, TopicStats]] = sorted( + snap.items(), key=lambda kv: kv[1].total_bytes, reverse=True + ) + self.table.clear(columns=False) # type: ignore[union-attr] + + for key, stats in rows: + base, msg_type = split_type_suffix(key.topic) + freq = stats.freq(STAT_WINDOW, now) + bps = stats.bytes_per_sec(STAT_WINDOW, now) + age = now - stats.last_seen if stats.last_seen is not None else None + stale = age is not None and age > STALE_AGE + + if stale: + # Liveness: dim the whole row for topics gone quiet. + self.table.add_row( # type: ignore[union-attr] + Text(key.transport, style=theme.DIM), + Text(base, style=theme.DIM), + Text(msg_type or "", style=theme.DIM), + Text(f"{freq:.1f}", style=theme.DIM), + Text(f"{human_bytes(bps)}/s", style=theme.DIM), + Text(human_bytes(stats.total_bytes), style=theme.DIM), + Text(f"{age:.0f}s", style=theme.DIM), + ) + continue + + age_str = f"{age:.1f}s" if age is not None else "-" + self.table.add_row( # type: ignore[union-attr] + Text(key.transport, style=theme.BLUE), + topic_text(base), + Text(msg_type or "", style=theme.BLUE), + Text(f"{freq:.1f}", style=gradient(10, freq)), + Text(f"{human_bytes(bps)}/s", style=gradient(1024 * 3, bps)), + Text(human_bytes(stats.total_bytes)), + Text(age_str, style=theme.ACCENT), + ) + def main() -> None: """Entry point for `dimos spy` (argv: optional 'web', --transport filters).""" - raise NotImplementedError + import sys + + argv = sys.argv[1:] + if argv and argv[0] == "web": + import os + + from textual_serve.server import Server # type: ignore[import-not-found] + + server = Server(f"python {os.path.abspath(__file__)}") + server.serve() + else: + SpyApp(transports=_parse_transports(argv)).run() + + +if __name__ == "__main__": + main() diff --git a/dimos/visualization/rerun/bridge.py b/dimos/visualization/rerun/bridge.py index 960d5e7826..52821b99f3 100644 --- a/dimos/visualization/rerun/bridge.py +++ b/dimos/visualization/rerun/bridge.py @@ -458,7 +458,7 @@ def start(self) -> None: logger.info(f"bridge listening on {pubsub.__class__.__name__}") if hasattr(pubsub, "start"): pubsub.start() - unsub = pubsub.subscribe_all(self._on_message) + unsub = pubsub.subscribe_latest(self._on_message) self.register_disposable(Disposable(unsub)) # Add pubsub stop as disposable diff --git a/dimos/visualization/rerun/test_viewer_integration.py b/dimos/visualization/rerun/test_viewer_integration.py index d6a0ba1e95..a4d5489e35 100644 --- a/dimos/visualization/rerun/test_viewer_integration.py +++ b/dimos/visualization/rerun/test_viewer_integration.py @@ -126,6 +126,9 @@ class ExplicitPubSubOverride: def subscribe_all(self, callback): return lambda: None + def subscribe_latest(self, callback): + return lambda: None + class TestBridgePubsubResolution: def test_legacy_lcm_pubsubs_defers_to_transport_default(self): From fae1dc74ee73160f6c257cfa4fcfa82149537a48 Mon Sep 17 00:00:00 2001 From: "brain (osvetnik)" Date: Mon, 6 Jul 2026 19:24:25 +0300 Subject: [PATCH 03/33] chore: drop internal task package file before publishing --- TASK.md | 125 -------------------------------------------------------- 1 file changed, 125 deletions(-) delete mode 100644 TASK.md diff --git a/TASK.md b/TASK.md deleted file mode 100644 index 08489913fa..0000000000 --- a/TASK.md +++ /dev/null @@ -1,125 +0,0 @@ -# TASK: `dimos spy` — universal transport spy - -## Goal (the outcome) - -`dimos spy` shows every topic flowing on every DimOS pubsub transport — LCM and Zenoh -simultaneously in v1 — with per-topic message rate, bandwidth, average size, total -traffic, and liveness (age since last message), in a Textual TUI matching `dimos lcmspy`'s -look. **The spy never decodes a message payload** (hard constraint from Ivan): its -per-message work is `(topic string, payload length, timestamp)` and nothing else. - -Along the way, fix the pubsub spec so `subscribe_all` means the same thing on every -transport (every message, no conflation) and move conflation to an explicit opt-in. - -## Context & where it fits - -Design doc (read first): `docs/usage/transports/spy.md`. Decisions were ratified by -Ivan/brain in ticket f6c74d39; the doc records them. Summary: zenoh's `subscribe_all` -conflates (latest-wins, built for the rerun bridge, its only consumer) and only watches -`dimos/**`; LCM's delivers everything. The spy builds on `subscribe_all`, so the spec -asymmetry gets fixed as part of this task. - -The branch already contains the full skeleton — this file, typed stubs (compile + pass -mypy, bodies raise `NotImplementedError`), and contract tests in -`dimos/protocol/pubsub/test_spy.py` that fail until implemented. **The tests are the -acceptance criteria.** Don't weaken them; if one seems wrong, flag it instead. - -## What to implement - -### 1. Spec fix: non-conflating `subscribe_all` + opt-in `subscribe_latest` - -- `dimos/protocol/pubsub/spec.py` — implement - `SubscribeLatestMixin.subscribe_latest()`: the latest-wins-per-topic buffer + single - drain thread + wake event currently living in `ZenohPubSubBase.subscribe_all` - (`impl/zenohpubsub.py:199`) — lift that machinery here essentially verbatim (it is - transport-agnostic: key the latest-dict by `str(topic)`). Contract is in the stub - docstring: callback on the drain thread, newest-per-topic eventually delivered, - returned callable unsubscribes and stops+joins the drain. -- `impl/zenohpubsub.py` — `subscribe_all` becomes a plain non-conflating - `self.subscribe(Topic("**"), callback)` (scope widened from `dimos/**` to `**` per - ratified decision; see design doc). Keep the stop()-vs-subscribe locking behavior that - is already there. Delete the now-lifted drain machinery, but keep zenoh's - drain-stop bookkeeping working for `subscribe_latest` subscriptions — - `stop()` must still terminate any live drain threads (move `_drain_stops` - handling as needed; simplest: `subscribe_latest` registers its stopper the same - way `subscribe_all` used to). -- `dimos/visualization/rerun/bridge.py:461` — the bridge needs conflation - (rerun lags otherwise, see the old comment): switch `pubsub.subscribe_all(...)` to - `pubsub.subscribe_latest(...)`. -- LCM's `subscribe_all` already satisfies the contract — leave it. - -### 2. Spy core: `dimos/protocol/pubsub/spy.py` - -All stubs documented in-file. Implementation notes: - -- `TopicStats`: port the math from `dimos/utils/cli/lcmspy/lcmspy.py` `Topic` (deque of - `(timestamp, nbytes)`, windowed freq/bandwidth/avg-size, lifetime totals) but with - explicit timestamps (see stub docstrings) and typed. Lock around deque mutation vs - reads (record() on transport threads, queries from the UI thread). -- `LCMSpySource` / `ZenohSpySource`: own a raw-bytes bus instance - (`LCMPubSubBase(**kwargs)` / `ZenohPubSubBase(**kwargs)`), `start()/stop()` delegate, - `tap(cb)` = `bus.subscribe_all(lambda msg, topic: cb(str(topic), len(msg)))`. - That lambda is the entire hot path — no other work is permitted there. -- `TransportSpy`: on `start()`, start each source and tap it with a callback that stamps - `time.time()` once and updates the per-`SpyKey` `TopicStats` + `totals`. `snapshot()` - returns a shallow copy of the dict (TopicStats objects shared, dict copied). -- `subscribe_decoded`: leave raising `NotImplementedError` — it is the spec'd hook for - the follow-up task (per-topic opt-in decode, separate subscription via the typed - encoder classes; never part of the tap path). -- `default_sources()`: `[LCMSpySource(), ZenohSpySource()]` — both, always; the spy - ignores `DIMOS_TRANSPORT` (it observes, it doesn't participate). - -### 3. TUI + CLI: `dimos/utils/cli/spy/run_spy.py` - -Model on `dimos/utils/cli/lcmspy/run_lcmspy.py` (DataTable, 0.5 s refresh interval, -theme colors/gradients, `q`/`ctrl+c` quit, `web` argv mode via textual-serve). Layout -and flags are in the stub docstring. Sort rows by total traffic; render Type via -`split_type_suffix`; grey/dim rows whose age exceeds a few seconds (liveness). -Call `lcmservice.autoconf(check_only=True)` before entering raw TUI mode when the LCM -source is active (run_lcmspy.py:84 shows why). - -`dimos spy` is already registered in `dimos/robot/cli/dimos.py`. Make `dimos lcmspy` -a deprecated alias: print a one-line deprecation note to stderr and run the spy with -the lcm source only (keep the command working; delete nothing in -`dimos/utils/cli/lcmspy/` in this task). - -### 4. Hot-path caveat to fix while you're there - -`Topic.from_channel_str` (`impl/lcmpubsub.py:113`, called per message on LCM pattern -subscriptions) calls `resolve_msg_type`, which attempts imports and is **uncached** -(`dimos/msgs/helpers.py:26`) — that's real per-message overhead on the spy's LCM path. -Zenoh already caches the equivalent via `@lru_cache` on `_key_expr_to_topic` -(`impl/zenohpubsub.py:93`). Add `functools.lru_cache` to `resolve_msg_type` (or cache -channel→Topic in the LCM handler, whichever is smaller). - -## Constraints / non-goals - -- **No payload decoding on the spy hot path, ever.** The contract tests monkeypatch - `Vector3.lcm_decode` to explode and publish undecodable garbage to enforce it. -- No conflation anywhere in the spy path — every message counts once. -- Non-goals for v1: SHM/ROS/DDS/Redis sources; implementing `subscribe_decoded`; - restructuring `dimos/utils/cli/lcmspy/` (the alias just routes around it); - zero-copy zenoh sizing (`len(sample.payload)` without `to_bytes()`) — noted as - future work in the design doc. -- Follow repo conventions: `uv` for deps, mypy + pytest green before commit - (`bin/pytest-fast`), no docstring bloat. - -## Pointers (read in this order) - -1. `docs/usage/transports/spy.md` — the design doc (why + decisions). -2. `dimos/protocol/pubsub/spec.py` — subscribe-all abstraction + new mixin stub. -3. `dimos/protocol/pubsub/impl/zenohpubsub.py:199` — the conflation machinery to lift. -4. `dimos/utils/cli/lcmspy/lcmspy.py` — stats math to port; `run_lcmspy.py` — TUI to model. -5. `dimos/protocol/pubsub/spy.py` — the stubs you're filling in. -6. `dimos/protocol/pubsub/test_spy.py` — the acceptance tests. -7. `dimos/visualization/rerun/bridge.py:461` — the one-line bridge migration. - -## Acceptance - -- `uv run pytest dimos/protocol/pubsub/test_spy.py` — all pass. -- Existing suites stay green, notably `test_spec.py`, `test_zenohpubsub.py`, - `test_lcmpubsub.py`, and rerun bridge tests. -- `mypy` clean. -- Manual smoke: `dimos --transport=zenoh run unitree-go2 --replay ...` (or any - publisher) in one terminal, `dimos spy` in another → topics from both zenoh and LCM - visible with sane rates; `dimos lcmspy` still works and warns it's deprecated. From 1ed1963913f3a0d62232f0c56ec5dfc0a388cc90 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 19:34:49 +0300 Subject: [PATCH 04/33] refactor: harden spy lifecycle + transport selection - TransportSpy.start() rolls back already-started sources on failure - subscribe_latest stops+joins the drain thread if subscribe_all raises - SOURCE_FACTORIES registry: construct only the requested transports (a filtered-out transport is never imported or instantiated) - run_spy/SpyApp validate unknown --transport names up front - add contract tests for the failure-rollback paths and run_spy selection --- dimos/protocol/pubsub/spec.py | 8 +++++- dimos/protocol/pubsub/spy.py | 34 +++++++++++++++++------ dimos/protocol/pubsub/test_spy.py | 43 +++++++++++++++++++++++++++++ dimos/robot/cli/dimos.py | 7 +++++ dimos/utils/cli/spy/run_spy.py | 29 +++++++++++++++---- dimos/utils/cli/spy/test_run_spy.py | 40 +++++++++++++++++++++++++++ 6 files changed, 146 insertions(+), 15 deletions(-) create mode 100644 dimos/utils/cli/spy/test_run_spy.py diff --git a/dimos/protocol/pubsub/spec.py b/dimos/protocol/pubsub/spec.py index 1652621417..d9d3f2e888 100644 --- a/dimos/protocol/pubsub/spec.py +++ b/dimos/protocol/pubsub/spec.py @@ -196,7 +196,13 @@ def stop_drain() -> None: # subscribe. If the transport already stopped, bail without subscribing. if not self._register_drain_stop(stop_drain, thread): return lambda: None - inner_unsub = self.subscribe_all(collect) + try: + inner_unsub = self.subscribe_all(collect) + except BaseException: + # Don't leak the drain thread if the underlying subscribe fails. + if self._unregister_drain_stop(stop_drain): + stop_drain() + raise def unsubscribe() -> None: if not self._unregister_drain_stop(stop_drain): diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index 1e6086630f..2a307c65d2 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -250,9 +250,20 @@ def on_message(topic: str, nbytes: int) -> None: return on_message def start(self) -> None: - for source in self._sources: - source.start() - self._untaps.append(source.tap(self._tap_callback(source.name))) + """Start and tap every source; on failure roll back the ones already started.""" + started: list[SpySource] = [] + try: + for source in self._sources: + source.start() + started.append(source) + self._untaps.append(source.tap(self._tap_callback(source.name))) + except BaseException: + for untap in self._untaps: + untap() + self._untaps.clear() + for source in reversed(started): + source.stop() + raise def stop(self) -> None: for untap in self._untaps: @@ -267,9 +278,16 @@ def snapshot(self) -> dict[SpyKey, TopicStats]: return dict(self._stats) -def default_sources() -> list[SpySource]: - """The spy observes ALL transports simultaneously, regardless of DIMOS_TRANSPORT. +SOURCE_FACTORIES: dict[str, Callable[[], SpySource]] = { + LCMSpySource.name: LCMSpySource, + ZenohSpySource.name: ZenohSpySource, +} +"""Known transports by name; each factory constructs its source only when called. - v1: [LCMSpySource(), ZenohSpySource()]. SHM/ROS/DDS/Redis are future sources. - """ - return [LCMSpySource(), ZenohSpySource()] +v1: lcm + zenoh. SHM/ROS/DDS/Redis are future sources. +""" + + +def default_sources() -> list[SpySource]: + """The spy observes ALL transports simultaneously, regardless of DIMOS_TRANSPORT.""" + return [factory() for factory in SOURCE_FACTORIES.values()] diff --git a/dimos/protocol/pubsub/test_spy.py b/dimos/protocol/pubsub/test_spy.py index a61f5198aa..46c2c73b3c 100644 --- a/dimos/protocol/pubsub/test_spy.py +++ b/dimos/protocol/pubsub/test_spy.py @@ -264,6 +264,23 @@ def cb(msg, topic): assert len(delivered) == count +def test_subscribe_latest_failed_subscribe_stops_drain_thread(): + class FailingBus(FakeAllPubSub): + def subscribe_all(self, callback): + raise RuntimeError("subscriber declaration failed") + + bus = FailingBus() + before = set(threading.enumerate()) + with pytest.raises(RuntimeError, match="subscriber declaration failed"): + bus.subscribe_latest(lambda msg, topic: None) + leaked = [ + t + for t in threading.enumerate() + if t not in before and t.name == "subscribe-latest-drain" and t.is_alive() + ] + assert not leaked # drain thread must be stopped and joined on subscribe failure + + # SpySources: every message counted, payloads never decoded @@ -432,6 +449,32 @@ def test_transport_spy_snapshot_is_stable_copy(): spy.stop() +def test_transport_spy_start_failure_stops_started_sources(): + class FailingStartSource(FakeSource): + def start(self) -> None: + raise RuntimeError("no zenoh here") + + a, b = FakeSource("lcm"), FailingStartSource("zenoh") + spy = TransportSpy(sources=[a, b]) + with pytest.raises(RuntimeError, match="no zenoh here"): + spy.start() + assert not a.started # rolled back, not left running + assert not a.taps + + +def test_transport_spy_tap_failure_stops_started_sources(): + class FailingTapSource(FakeSource): + def tap(self, callback): + raise RuntimeError("tap exploded") + + a, b = FakeSource("lcm"), FailingTapSource("zenoh") + spy = TransportSpy(sources=[a, b]) + with pytest.raises(RuntimeError, match="tap exploded"): + spy.start() + assert not a.started and not b.started + assert not a.taps + + def test_fake_source_satisfies_protocol(): from dimos.protocol.pubsub.spy import SpySource diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index 63a0682d05..5001e0c97f 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -633,6 +633,13 @@ def spy(ctx: typer.Context) -> None: def lcmspy(ctx: typer.Context) -> None: """Deprecated alias for `dimos spy --transport lcm`.""" print("dimos lcmspy is deprecated; use `dimos spy --transport lcm`.", file=sys.stderr) + if any(arg == "--transport" or arg.startswith("--transport=") for arg in ctx.args): + typer.echo( + "Error: dimos lcmspy is LCM-only; use `dimos spy --transport ...` " + "to choose transports.", + err=True, + ) + raise typer.Exit(2) from dimos.utils.cli.spy.run_spy import main as spy_main sys.argv = ["spy", "--transport", "lcm", *ctx.args] diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index c33f4c5b5e..c55b2e8605 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -37,10 +37,10 @@ from textual.widgets import DataTable from dimos.protocol.pubsub.spy import ( + SOURCE_FACTORIES, SpyKey, TopicStats, TransportSpy, - default_sources, split_type_suffix, ) from dimos.utils.cli import theme @@ -68,7 +68,11 @@ def topic_text(base: str) -> Text: def _parse_transports(argv: list[str]) -> list[str] | None: - """Parse repeated --transport flags; None means all default sources.""" + """Parse repeated --transport flags; None means all default sources. + + Exits with an error (rather than launching an empty spy) if any requested + transport is not a known source name. + """ transports: list[str] = [] i = 0 while i < len(argv): @@ -80,6 +84,12 @@ def _parse_transports(argv: list[str]) -> list[str] | None: elif arg.startswith("--transport="): transports.append(arg.split("=", 1)[1]) i += 1 + unknown = [t for t in transports if t not in SOURCE_FACTORIES] + if unknown: + valid = ", ".join(SOURCE_FACTORIES) + raise SystemExit( + f"Error: unknown transport(s) {', '.join(unknown)} — valid choices: {valid}" + ) return transports or None @@ -115,15 +125,22 @@ class SpyApp(App): # type: ignore[type-arg] def __init__(self, transports: list[str] | None = None, **kwargs) -> None: # type: ignore[no-untyped-def] super().__init__(**kwargs) - sources = default_sources() - if transports is not None: - sources = [s for s in sources if s.name in transports] + names = list(SOURCE_FACTORIES) if transports is None else list(transports) + unknown = [n for n in names if n not in SOURCE_FACTORIES] + if unknown: + raise ValueError( + f"unknown transport(s) {', '.join(unknown)} — valid choices: " + f"{', '.join(SOURCE_FACTORIES)}" + ) # Warn about missing system config before entering TUI raw mode (LCM only). - if any(s.name == "lcm" for s in sources): + if "lcm" in names: from dimos.protocol.service.lcmservice import autoconf autoconf(check_only=True) + # Construct only the requested sources: a filtered-out transport must + # never be imported or instantiated. + sources = [SOURCE_FACTORIES[name]() for name in names] self.spy = TransportSpy(sources=sources) self.spy.start() self.table: DataTable | None = None # type: ignore[type-arg] diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py new file mode 100644 index 0000000000..0c286cbd43 --- /dev/null +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -0,0 +1,40 @@ +# Copyright 2026 Dimensional Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""CLI arg handling for `dimos spy`: --transport parsing and validation.""" + +import pytest + +from dimos.utils.cli.spy.run_spy import _parse_transports + + +def test_parse_transports_default_is_none(): + assert _parse_transports([]) is None + assert _parse_transports(["web"]) is None + + +def test_parse_transports_collects_repeated_flags(): + assert _parse_transports(["--transport", "lcm"]) == ["lcm"] + assert _parse_transports(["--transport=zenoh"]) == ["zenoh"] + assert _parse_transports(["--transport", "lcm", "--transport", "zenoh"]) == ["lcm", "zenoh"] + + +def test_parse_transports_rejects_unknown_transport(): + with pytest.raises(SystemExit, match="zneoh"): + _parse_transports(["--transport", "zneoh"]) + + +def test_parse_transports_error_lists_valid_choices(): + with pytest.raises(SystemExit, match="lcm"): + _parse_transports(["--transport=nope"]) From e55a1a2d7b0f629b445a70e2a21c4d059b3b0dee Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 19:39:25 +0300 Subject: [PATCH 05/33] refactor: make lcmspy a thin alias for spy --transport lcm - dimos lcmspy subcommand: drop deprecation/guard, just run spy --transport lcm - lcmspy console-script repointed to run_spy:lcm_main shim (LCM-only spy) - delete the superseded dimos/utils/cli/lcmspy/ module (TUI + stats + tests) --- dimos/robot/cli/dimos.py | 10 +- dimos/utils/cli/lcmspy/lcmspy.py | 196 ----------------------- dimos/utils/cli/lcmspy/run_lcmspy.py | 138 ---------------- dimos/utils/cli/lcmspy/test_lcmspy.py | 218 -------------------------- dimos/utils/cli/spy/run_spy.py | 16 +- pyproject.toml | 2 +- 6 files changed, 14 insertions(+), 566 deletions(-) delete mode 100755 dimos/utils/cli/lcmspy/lcmspy.py delete mode 100644 dimos/utils/cli/lcmspy/run_lcmspy.py delete mode 100644 dimos/utils/cli/lcmspy/test_lcmspy.py diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index 5001e0c97f..be2dfadaed 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -631,15 +631,7 @@ def spy(ctx: typer.Context) -> None: @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def lcmspy(ctx: typer.Context) -> None: - """Deprecated alias for `dimos spy --transport lcm`.""" - print("dimos lcmspy is deprecated; use `dimos spy --transport lcm`.", file=sys.stderr) - if any(arg == "--transport" or arg.startswith("--transport=") for arg in ctx.args): - typer.echo( - "Error: dimos lcmspy is LCM-only; use `dimos spy --transport ...` " - "to choose transports.", - err=True, - ) - raise typer.Exit(2) + """Alias for `dimos spy --transport lcm`.""" from dimos.utils.cli.spy.run_spy import main as spy_main sys.argv = ["spy", "--transport", "lcm", *ctx.args] diff --git a/dimos/utils/cli/lcmspy/lcmspy.py b/dimos/utils/cli/lcmspy/lcmspy.py deleted file mode 100755 index b1fdc2db38..0000000000 --- a/dimos/utils/cli/lcmspy/lcmspy.py +++ /dev/null @@ -1,196 +0,0 @@ -# Copyright 2025-2026 Dimensional Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -from collections import deque -import threading -import time -from typing import Any - -from dimos.constants import DEFAULT_THREAD_JOIN_TIMEOUT -from dimos.protocol.service.lcmservice import LCMConfig, LCMService -from dimos.utils.human import human_bytes - - -class Topic: - history_window: float = 60.0 - - def __init__(self, name: str, history_window: float = 60.0) -> None: - self.name = name - # Store (timestamp, data_size) tuples for statistics - self.message_history = deque() # type: ignore[var-annotated] - self._lock = threading.Lock() - self.history_window = history_window - # Total traffic accumulator (doesn't get cleaned up) - self.total_traffic_bytes = 0 - - def msg(self, data: bytes) -> None: - # print(f"> msg {self.__str__()} {len(data)} bytes") - datalen = len(data) - with self._lock: - self.message_history.append((time.time(), datalen)) - self.total_traffic_bytes += datalen - self._cleanup_old_messages() - - def _cleanup_old_messages(self, max_age: float | None = None) -> None: - """Remove messages older than max_age seconds""" - current_time = time.time() - while self.message_history and current_time - self.message_history[0][0] > ( - max_age or self.history_window - ): - self.message_history.popleft() - - def _get_messages_in_window(self, time_window: float): # type: ignore[no-untyped-def] - """Get messages within the specified time window""" - current_time = time.time() - cutoff_time = current_time - time_window - with self._lock: - return [(ts, size) for ts, size in self.message_history if ts >= cutoff_time] - - # avg msg freq in the last n seconds - def freq(self, time_window: float) -> float: - messages = self._get_messages_in_window(time_window) - if not messages: - return 0.0 - return len(messages) / time_window - - # avg bandwidth in kB/s in the last n seconds - def kbps(self, time_window: float) -> float: - messages = self._get_messages_in_window(time_window) - if not messages: - return 0.0 - total_bytes = sum(size for _, size in messages) - total_kbytes = total_bytes / 1000 # Convert bytes to kB - return total_kbytes / time_window # type: ignore[no-any-return] - - def kbps_hr(self, time_window: float) -> str: - """Return human-readable bandwidth with appropriate units""" - bps = self.kbps(time_window) * 1000 - return human_bytes(bps) + "/s" - - # avg msg size in the last n seconds - def size(self, time_window: float) -> float: - messages = self._get_messages_in_window(time_window) - if not messages: - return 0.0 - total_size = sum(size for _, size in messages) - return total_size / len(messages) # type: ignore[no-any-return] - - def total_traffic(self) -> int: - """Return total traffic passed in bytes since the beginning""" - with self._lock: - return self.total_traffic_bytes - - def total_traffic_hr(self) -> str: - """Return human-readable total traffic with appropriate units""" - return human_bytes(self.total_traffic()) - - def __str__(self) -> str: - return f"topic({self.name})" - - -class LCMSpyConfig(LCMConfig): - topic_history_window: float = 60.0 - - -class LCMSpy(LCMService, Topic): - config: LCMSpyConfig - topic = dict[str, Topic] - graph_log_window: float = 1.0 - topic_class: type[Topic] = Topic - - def __init__(self, **kwargs: Any) -> None: - super().__init__(**kwargs) - Topic.__init__(self, name="total", history_window=self.config.topic_history_window) - self.topic = {} # type: ignore[assignment] - self._topic_lock = threading.Lock() - - def start(self) -> None: - super().start() - self.l.subscribe(".*", self.msg) # type: ignore[union-attr] - - def stop(self) -> None: - """Stop the LCM spy and clean up resources""" - super().stop() - - def msg(self, topic, data) -> None: # type: ignore[no-untyped-def, override] - Topic.msg(self, data) - - with self._topic_lock: - if topic not in self.topic: # type: ignore[operator] - print(self.config) - self.topic[topic] = self.topic_class( # type: ignore[assignment, call-arg] - topic, - history_window=self.config.topic_history_window, - ) - self.topic[topic].msg(data) # type: ignore[attr-defined, type-arg] - - -class GraphTopic(Topic): - def __init__(self, *args, **kwargs) -> None: # type: ignore[no-untyped-def] - super().__init__(*args, **kwargs) - self.freq_history = deque(maxlen=20) # type: ignore[var-annotated] - self.bandwidth_history = deque(maxlen=20) # type: ignore[var-annotated] - - def update_graphs(self, step_window: float = 1.0) -> None: - """Update historical data for graphing""" - freq = self.freq(step_window) - kbps = self.kbps(step_window) - self.freq_history.append(freq) - self.bandwidth_history.append(kbps) - - -class GraphLCMSpyConfig(LCMSpyConfig): - graph_log_window: float = 1.0 - - -class GraphLCMSpy(LCMSpy, GraphTopic): - config: GraphLCMSpyConfig - graph_log_thread: threading.Thread | None = None - graph_log_stop_event: threading.Event = threading.Event() - topic_class: type[Topic] = GraphTopic - - def __init__(self, **kwargs: Any) -> None: - super().__init__(**kwargs) - GraphTopic.__init__(self, name="total", history_window=self.config.topic_history_window) - - def start(self) -> None: - super().start() - self.graph_log_thread = threading.Thread(target=self.graph_log, daemon=True) - self.graph_log_thread.start() - - def graph_log(self) -> None: - while not self.graph_log_stop_event.is_set(): - self.update_graphs(self.config.graph_log_window) # type: ignore[attr-defined] # Update global history - with self._topic_lock: - topics = list(self.topic.values()) # type: ignore[call-arg] - for topic in topics: - topic.update_graphs(self.config.graph_log_window) # type: ignore[attr-defined] - time.sleep(self.config.graph_log_window) # type: ignore[attr-defined] - - def stop(self) -> None: - """Stop the graph logging and LCM spy""" - self.graph_log_stop_event.set() - if self.graph_log_thread and self.graph_log_thread.is_alive(): - self.graph_log_thread.join(timeout=DEFAULT_THREAD_JOIN_TIMEOUT) - super().stop() - - -if __name__ == "__main__": - lcm_spy = LCMSpy() - lcm_spy.start() - try: - while True: - time.sleep(1) - except KeyboardInterrupt: - print("LCM Spy stopped.") diff --git a/dimos/utils/cli/lcmspy/run_lcmspy.py b/dimos/utils/cli/lcmspy/run_lcmspy.py deleted file mode 100644 index 438d93fa1f..0000000000 --- a/dimos/utils/cli/lcmspy/run_lcmspy.py +++ /dev/null @@ -1,138 +0,0 @@ -# Copyright 2025-2026 Dimensional Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -from __future__ import annotations - -from rich.text import Text -from textual.app import App, ComposeResult -from textual.color import Color -from textual.widgets import DataTable - -from dimos.utils.cli import theme -from dimos.utils.cli.lcmspy.lcmspy import GraphLCMSpy, GraphTopic as SpyTopic - - -def gradient(max_value: float, value: float) -> str: - """Gradient from cyan (low) to yellow (high) using DimOS theme colors""" - ratio = min(value / max_value, 1.0) - # Parse hex colors from theme - cyan = Color.parse(theme.CYAN) - yellow = Color.parse(theme.YELLOW) - color = cyan.blend(yellow, ratio) - - return color.hex - - -def topic_text(topic_name: str) -> Text: - """Format topic name with DimOS theme colors""" - if "#" in topic_name: - parts = topic_name.split("#", 1) - return Text(parts[0], style=theme.BRIGHT_WHITE) + Text("#" + parts[1], style=theme.BLUE) - - if topic_name[:4] == "/rpc": - return Text(topic_name[:4], style=theme.BLUE) + Text( - topic_name[4:], style=theme.BRIGHT_WHITE - ) - - return Text(topic_name, style=theme.BRIGHT_WHITE) - - -class LCMSpyApp(App): # type: ignore[type-arg] - """A real-time CLI dashboard for LCM traffic statistics using Textual.""" - - CSS_PATH = "../dimos.tcss" - - CSS = f""" - Screen {{ - layout: vertical; - background: {theme.BACKGROUND}; - }} - DataTable {{ - height: 2fr; - width: 1fr; - border: solid {theme.BORDER}; - background: {theme.BG}; - scrollbar-size: 0 0; - }} - DataTable > .datatable--header {{ - color: {theme.ACCENT}; - background: transparent; - }} - """ - - refresh_interval: float = 0.5 # seconds - - BINDINGS = [ - ("q", "quit"), - ("ctrl+c", "quit"), - ] - - def __init__(self, *args, **kwargs) -> None: # type: ignore[no-untyped-def] - super().__init__(*args, **kwargs) - # Warn about missing system config before entering TUI raw mode. - from dimos.protocol.service.lcmservice import autoconf - - autoconf(check_only=True) - - self.spy = GraphLCMSpy(graph_log_window=0.5) - self.spy.start() - self.table: DataTable | None = None # type: ignore[type-arg] - - def compose(self) -> ComposeResult: - self.table = DataTable(zebra_stripes=False, cursor_type=None) # type: ignore[arg-type] - self.table.add_column("Topic") - self.table.add_column("Freq (Hz)") - self.table.add_column("Bandwidth") - self.table.add_column("Total Traffic") - yield self.table - - def on_mount(self) -> None: - self.set_interval(self.refresh_interval, self.refresh_table) - - async def on_unmount(self) -> None: - self.spy.stop() - - def refresh_table(self) -> None: - topics: list[SpyTopic] = list(self.spy.topic.values()) # type: ignore[arg-type, call-arg] - topics.sort(key=lambda t: t.total_traffic(), reverse=True) - self.table.clear(columns=False) # type: ignore[union-attr] - - for t in topics: - freq = t.freq(5.0) - kbps = t.kbps(5.0) - - self.table.add_row( # type: ignore[union-attr] - topic_text(t.name), - Text(f"{freq:.1f}", style=gradient(10, freq)), - Text(t.kbps_hr(5.0), style=gradient(1024 * 3, kbps)), - Text(t.total_traffic_hr()), - ) - - -def main() -> None: - import sys - - if len(sys.argv) > 1 and sys.argv[1] == "web": - import os - - from textual_serve.server import Server # type: ignore[import-not-found] - - server = Server(f"python {os.path.abspath(__file__)}") - server.serve() - else: - LCMSpyApp().run() - - -if __name__ == "__main__": - main() diff --git a/dimos/utils/cli/lcmspy/test_lcmspy.py b/dimos/utils/cli/lcmspy/test_lcmspy.py deleted file mode 100644 index 10799d8a9c..0000000000 --- a/dimos/utils/cli/lcmspy/test_lcmspy.py +++ /dev/null @@ -1,218 +0,0 @@ -# Copyright 2025-2026 Dimensional Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import time - -import pytest - -from dimos.protocol.pubsub.impl.lcmpubsub import PickleLCM, Topic -from dimos.utils.cli.lcmspy.lcmspy import GraphLCMSpy, GraphTopic, LCMSpy, Topic as TopicSpy - - -@pytest.fixture -def pickle_lcm(): - lcm = PickleLCM() - lcm.start() - yield lcm - lcm.stop() - - -@pytest.fixture -def lcmspy_instance(): - spy = LCMSpy() - spy.start() - yield spy - spy.stop() - - -@pytest.fixture -def graph_lcmspy_instance(): - spy = GraphLCMSpy(graph_log_window=0.1) - spy.start() - time.sleep(0.2) # Wait for thread to start - yield spy - spy.stop() - - -def test_spy_basic(pickle_lcm, lcmspy_instance) -> None: - video_topic = Topic(topic="/video") - odom_topic = Topic(topic="/odom") - - for i in range(5): - pickle_lcm.publish(video_topic, f"video frame {i}") - time.sleep(0.1) - if i % 2 == 0: - pickle_lcm.publish(odom_topic, f"odometry data {i / 2}") - - # Wait a bit for messages to be processed - time.sleep(0.5) - - # Test statistics for video topic - video_topic_spy = lcmspy_instance.topic["/video"] - assert video_topic_spy is not None - - # Test frequency (should be around 10 Hz for 5 messages in ~0.5 seconds) - freq = video_topic_spy.freq(1.0) - assert freq > 0 - print(f"Video topic frequency: {freq:.2f} Hz") - - # Test bandwidth - kbps = video_topic_spy.kbps(1.0) - assert kbps > 0 - print(f"Video topic bandwidth: {kbps:.2f} kbps") - - # Test average message size - avg_size = video_topic_spy.size(1.0) - assert avg_size > 0 - print(f"Video topic average message size: {avg_size:.2f} bytes") - - # Test statistics for odom topic - odom_topic_spy = lcmspy_instance.topic["/odom"] - assert odom_topic_spy is not None - - freq = odom_topic_spy.freq(1.0) - assert freq > 0 - print(f"Odom topic frequency: {freq:.2f} Hz") - - kbps = odom_topic_spy.kbps(1.0) - assert kbps > 0 - print(f"Odom topic bandwidth: {kbps:.2f} kbps") - - avg_size = odom_topic_spy.size(1.0) - assert avg_size > 0 - print(f"Odom topic average message size: {avg_size:.2f} bytes") - - print(f"Video topic: {video_topic_spy}") - print(f"Odom topic: {odom_topic_spy}") - - -def test_topic_statistics_direct() -> None: - """Test Topic statistics directly without LCM""" - - topic = TopicSpy("/test") - - # Add some test messages - test_data = [b"small", b"medium sized message", b"very long message for testing purposes"] - - for _i, data in enumerate(test_data): - topic.msg(data) - time.sleep(0.1) # Simulate time passing - - # Test statistics over 1 second window - freq = topic.freq(1.0) - kbps = topic.kbps(1.0) - avg_size = topic.size(1.0) - - assert freq > 0 - assert kbps > 0 - assert avg_size > 0 - - print(f"Direct test - Frequency: {freq:.2f} Hz") - print(f"Direct test - Bandwidth: {kbps:.2f} kbps") - print(f"Direct test - Avg size: {avg_size:.2f} bytes") - - -def test_topic_cleanup() -> None: - """Test that old messages are properly cleaned up""" - - topic = TopicSpy("/test") - - # Add a message - topic.msg(b"test message") - initial_count = len(topic.message_history) - assert initial_count == 1 - - # Simulate time passing by manually adding old timestamps - old_time = time.time() - 70 # 70 seconds ago - topic.message_history.appendleft((old_time, 10)) - - # Trigger cleanup - topic._cleanup_old_messages(max_age=60.0) - - # Should only have the recent message - assert len(topic.message_history) == 1 - assert topic.message_history[0][0] > time.time() - 10 # Recent message - - -def test_graph_topic_basic() -> None: - """Test GraphTopic basic functionality""" - topic = GraphTopic("/test_graph") - - # Add some messages and update graphs - topic.msg(b"test message") - topic.update_graphs(1.0) - - # Should have history data - assert len(topic.freq_history) == 1 - assert len(topic.bandwidth_history) == 1 - assert topic.freq_history[0] > 0 - assert topic.bandwidth_history[0] > 0 - - -def test_graph_lcmspy_basic(graph_lcmspy_instance) -> None: - """Test GraphLCMSpy basic functionality""" - # Simulate a message - graph_lcmspy_instance.msg("/test", b"test data") - time.sleep(0.5) # Wait for graph update — macOS needs longer for thread scheduling - - # Should create GraphTopic with history - topic = graph_lcmspy_instance.topic["/test"] - assert isinstance(topic, GraphTopic) - assert len(topic.freq_history) > 0 - assert len(topic.bandwidth_history) > 0 - - -def test_lcmspy_global_totals(lcmspy_instance) -> None: - """Test that LCMSpy tracks global totals as a Topic itself""" - # Send messages to different topics - lcmspy_instance.msg("/video", b"video frame data") - lcmspy_instance.msg("/odom", b"odometry data") - lcmspy_instance.msg("/imu", b"imu data") - - # Verify each test topic received exactly one message (ignore LCM discovery packets) - for t in ("/video", "/odom", "/imu"): - assert len(lcmspy_instance.topic[t].message_history) == 1 - - # Check global statistics - global_freq = lcmspy_instance.freq(1.0) - global_kbps = lcmspy_instance.kbps(1.0) - global_size = lcmspy_instance.size(1.0) - - assert global_freq > 0 - assert global_kbps > 0 - assert global_size > 0 - - print(f"Global frequency: {global_freq:.2f} Hz") - print(f"Global bandwidth: {lcmspy_instance.kbps_hr(1.0)}") - print(f"Global avg message size: {global_size:.0f} bytes") - - -def test_graph_lcmspy_global_totals(graph_lcmspy_instance) -> None: - """Test that GraphLCMSpy tracks global totals with history""" - # Send messages - graph_lcmspy_instance.msg("/video", b"video frame data") - graph_lcmspy_instance.msg("/odom", b"odometry data") - time.sleep(0.2) # Wait for graph update - - # Update global graphs - graph_lcmspy_instance.update_graphs(1.0) - - # Should have global history - assert len(graph_lcmspy_instance.freq_history) == 1 - assert len(graph_lcmspy_instance.bandwidth_history) == 1 - assert graph_lcmspy_instance.freq_history[0] > 0 - assert graph_lcmspy_instance.bandwidth_history[0] > 0 - - print(f"Global frequency history: {graph_lcmspy_instance.freq_history[0]:.2f} Hz") - print(f"Global bandwidth history: {graph_lcmspy_instance.bandwidth_history[0]:.2f} kB/s") diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index c55b2e8605..f85570a04a 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -14,9 +14,9 @@ """`dimos spy` TUI: live table of all topics across all pubsub transports. -Textual app modeled on run_lcmspy.py (DataTable, 0.5s refresh, theme colors, -'q' to quit, optional `spy web` mode via textual-serve). One row per -(transport, topic) from TransportSpy.snapshot(): +Textual app (DataTable, 0.5s refresh, theme colors, 'q' to quit, optional +`spy web` mode via textual-serve). One row per (transport, topic) from +TransportSpy.snapshot(): Transport | Topic | Type | Freq (Hz) | Bandwidth | Total | Age @@ -24,7 +24,7 @@ - Age is seconds since TopicStats.last_seen (liveness: dims/greys out stale rows). - `--transport lcm --transport zenoh` (repeatable) filters sources; default all. - LCM system config warning: call lcmservice.autoconf(check_only=True) before - entering raw TUI mode, like run_lcmspy does. + entering raw TUI mode. """ from __future__ import annotations @@ -218,5 +218,13 @@ def main() -> None: SpyApp(transports=_parse_transports(argv)).run() +def lcm_main() -> None: + """`lcmspy` console-script shim: the spy over the LCM source only.""" + import sys + + sys.argv = ["lcmspy", "--transport", "lcm"] + main() + + if __name__ == "__main__": main() diff --git a/pyproject.toml b/pyproject.toml index c7a043562d..c79169475b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -150,7 +150,7 @@ dependencies = [ [project.scripts] -lcmspy = "dimos.utils.cli.lcmspy.run_lcmspy:main" +lcmspy = "dimos.utils.cli.spy.run_spy:lcm_main" agentspy = "dimos.utils.cli.agentspy.agentspy:main" humancli = "dimos.utils.cli.human.humanclianim:main" dimos = "dimos.robot.cli.dimos:cli_main" From d29c7400e4f8f6d012c16ef7781c94b2da142f1c Mon Sep 17 00:00:00 2001 From: "brain (osvetnik)" Date: Mon, 6 Jul 2026 19:44:06 +0300 Subject: [PATCH 06/33] fix: degrade default spy on missing backends, forward web-mode filters - default_sources() skips backends that fail to import with a one-line stderr warning and errors only if no transport is available; an explicitly requested --transport keeps the hard error - spy web mode serves the child with sys.executable and forwards the --transport filter args (validated before serving) - restore the lcmspy --transport override guard dropped by the thin-alias refactor - tests: default degradation, explicit hard error, web command construction --- dimos/protocol/pubsub/spy.py | 18 +++++++- dimos/protocol/pubsub/test_spy.py | 23 ++++++++++ dimos/robot/cli/dimos.py | 7 ++++ dimos/utils/cli/spy/run_spy.py | 43 ++++++++++++------- dimos/utils/cli/spy/test_run_spy.py | 65 ++++++++++++++++++++++++++++- 5 files changed, 138 insertions(+), 18 deletions(-) diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index 2a307c65d2..7e7c97bbf6 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -30,6 +30,7 @@ from collections import deque from dataclasses import dataclass +import sys import threading import time from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable @@ -289,5 +290,18 @@ def snapshot(self) -> dict[SpyKey, TopicStats]: def default_sources() -> list[SpySource]: - """The spy observes ALL transports simultaneously, regardless of DIMOS_TRANSPORT.""" - return [factory() for factory in SOURCE_FACTORIES.values()] + """The spy observes ALL transports simultaneously, regardless of DIMOS_TRANSPORT. + + A backend that is not importable is skipped with a warning, so the default + spy degrades to whatever transports are available. Requesting a transport + explicitly (constructing its SOURCE_FACTORIES entry) keeps the hard error. + """ + sources: list[SpySource] = [] + for name, factory in SOURCE_FACTORIES.items(): + try: + sources.append(factory()) + except ImportError as exc: + print(f"spy: skipping unavailable transport {name!r}: {exc}", file=sys.stderr) + if not sources: + raise RuntimeError(f"no spy transports available (tried: {', '.join(SOURCE_FACTORIES)})") + return sources diff --git a/dimos/protocol/pubsub/test_spy.py b/dimos/protocol/pubsub/test_spy.py index 46c2c73b3c..fb3215d769 100644 --- a/dimos/protocol/pubsub/test_spy.py +++ b/dimos/protocol/pubsub/test_spy.py @@ -36,11 +36,13 @@ from dimos.protocol.pubsub.impl.zenohpubsub import ZenohPubSubBase from dimos.protocol.pubsub.spec import AllPubSub from dimos.protocol.pubsub.spy import ( + SOURCE_FACTORIES, LCMSpySource, SpyKey, TopicStats, TransportSpy, ZenohSpySource, + default_sources, split_type_suffix, ) from dimos.protocol.service.zenohservice import ZenohSessionPool @@ -475,6 +477,27 @@ def tap(self, callback): assert not a.taps +def test_default_sources_skips_unavailable_backend(monkeypatch, capsys): + def unavailable(): + raise ImportError("zenoh backend missing") + + monkeypatch.setitem(SOURCE_FACTORIES, "lcm", lambda: FakeSource("lcm")) + monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", unavailable) + sources = default_sources() + assert [s.name for s in sources] == ["lcm"] # degrades instead of crashing + assert "zenoh" in capsys.readouterr().err + + +def test_default_sources_errors_when_no_backend_available(monkeypatch): + def unavailable(): + raise ImportError("backend missing") + + for name in SOURCE_FACTORIES: + monkeypatch.setitem(SOURCE_FACTORIES, name, unavailable) + with pytest.raises(RuntimeError, match="no spy transports available"): + default_sources() + + def test_fake_source_satisfies_protocol(): from dimos.protocol.pubsub.spy import SpySource diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index be2dfadaed..00cdf5b72f 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -632,6 +632,13 @@ def spy(ctx: typer.Context) -> None: @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def lcmspy(ctx: typer.Context) -> None: """Alias for `dimos spy --transport lcm`.""" + if any(arg == "--transport" or arg.startswith("--transport=") for arg in ctx.args): + typer.echo( + "Error: dimos lcmspy is LCM-only; use `dimos spy --transport ...` " + "to choose transports.", + err=True, + ) + raise typer.Exit(2) from dimos.utils.cli.spy.run_spy import main as spy_main sys.argv = ["spy", "--transport", "lcm", *ctx.args] diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index f85570a04a..8e7b5342ad 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -41,6 +41,7 @@ SpyKey, TopicStats, TransportSpy, + default_sources, split_type_suffix, ) from dimos.utils.cli import theme @@ -125,22 +126,27 @@ class SpyApp(App): # type: ignore[type-arg] def __init__(self, transports: list[str] | None = None, **kwargs) -> None: # type: ignore[no-untyped-def] super().__init__(**kwargs) - names = list(SOURCE_FACTORIES) if transports is None else list(transports) - unknown = [n for n in names if n not in SOURCE_FACTORIES] - if unknown: - raise ValueError( - f"unknown transport(s) {', '.join(unknown)} — valid choices: " - f"{', '.join(SOURCE_FACTORIES)}" - ) + if transports is None: + # Default: every available transport; unavailable backends are + # skipped with a warning (see default_sources). + sources = default_sources() + else: + unknown = [n for n in transports if n not in SOURCE_FACTORIES] + if unknown: + raise ValueError( + f"unknown transport(s) {', '.join(unknown)} — valid choices: " + f"{', '.join(SOURCE_FACTORIES)}" + ) + # Construct only the requested sources: a filtered-out transport is + # never imported or instantiated, and an unavailable one that was + # explicitly requested stays a hard error. + sources = [SOURCE_FACTORIES[name]() for name in transports] # Warn about missing system config before entering TUI raw mode (LCM only). - if "lcm" in names: + if any(s.name == "lcm" for s in sources): from dimos.protocol.service.lcmservice import autoconf autoconf(check_only=True) - # Construct only the requested sources: a filtered-out transport must - # never be imported or instantiated. - sources = [SOURCE_FACTORIES[name]() for name in names] self.spy = TransportSpy(sources=sources) self.spy.start() self.table: DataTable | None = None # type: ignore[type-arg] @@ -202,17 +208,26 @@ def refresh_table(self) -> None: ) +def _web_command(argv: list[str]) -> str: + """Command line for the textual-serve child: rerun this script with the filter args.""" + import os + import shlex + import sys + + return shlex.join([sys.executable, os.path.abspath(__file__), *argv]) + + def main() -> None: """Entry point for `dimos spy` (argv: optional 'web', --transport filters).""" import sys argv = sys.argv[1:] if argv and argv[0] == "web": - import os - from textual_serve.server import Server # type: ignore[import-not-found] - server = Server(f"python {os.path.abspath(__file__)}") + rest = argv[1:] + _parse_transports(rest) # reject unknown transports before serving + server = Server(_web_command(rest)) server.serve() else: SpyApp(transports=_parse_transports(argv)).run() diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 0c286cbd43..17a73d04c8 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -12,11 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""CLI arg handling for `dimos spy`: --transport parsing and validation.""" +"""CLI arg handling for `dimos spy`: --transport parsing, validation, web mode.""" + +from typing import Any import pytest -from dimos.utils.cli.spy.run_spy import _parse_transports +from dimos.protocol.pubsub.spy import SOURCE_FACTORIES +from dimos.utils.cli.spy.run_spy import SpyApp, _parse_transports, _web_command def test_parse_transports_default_is_none(): @@ -38,3 +41,61 @@ def test_parse_transports_rejects_unknown_transport(): def test_parse_transports_error_lists_valid_choices(): with pytest.raises(SystemExit, match="lcm"): _parse_transports(["--transport=nope"]) + + +class _StubSource: + name = "zenoh" + + def __init__(self) -> None: + self.started = False + + def start(self) -> None: + self.started = True + + def stop(self) -> None: + self.started = False + + def tap(self, callback: Any) -> Any: + return lambda: None + + def subscribe_decoded(self, topic: str, callback: Any) -> Any: + raise NotImplementedError + + +def test_spy_app_default_skips_unavailable_backend(monkeypatch, capsys): + def unavailable(): + raise ImportError("lcm backend missing") + + created: list[_StubSource] = [] + + def stub_factory() -> _StubSource: + source = _StubSource() + created.append(source) + return source + + monkeypatch.setitem(SOURCE_FACTORIES, "lcm", unavailable) + monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", stub_factory) + app = SpyApp() # no --transport: degrades to the available backend + try: + assert [s.started for s in created] == [True] + finally: + app.spy.stop() + assert "lcm" in capsys.readouterr().err + + +def test_spy_app_explicit_unavailable_transport_is_hard_error(monkeypatch): + def unavailable(): + raise ImportError("zenoh backend missing") + + monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", unavailable) + with pytest.raises(ImportError, match="zenoh backend missing"): + SpyApp(transports=["zenoh"]) + + +def test_web_command_forwards_transport_filters(): + cmd = _web_command(["--transport", "zenoh"]) + assert cmd.endswith("run_spy.py --transport zenoh") + + +def test_web_command_without_filters_serves_bare_script(): + assert _web_command([]).endswith("run_spy.py") From 22f5442383f7e368733ec4cd368edc1504d6a094 Mon Sep 17 00:00:00 2001 From: "brain (osvetnik)" Date: Mon, 6 Jul 2026 19:53:11 +0300 Subject: [PATCH 07/33] fix: route lcmspy web through spy web mode dimos lcmspy web previously became spy --transport lcm web, which missed run_spy.main()'s web branch (web must be argv[0]) and silently launched the local TUI. Keep web first and forward the LCM filter behind it; the --transport override guard stays. Alias routing is now covered by CliRunner tests (tui, web, override rejection). --- dimos/robot/cli/dimos.py | 10 ++++++++-- dimos/robot/cli/test_dimos.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index 00cdf5b72f..7f98584b93 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -632,7 +632,8 @@ def spy(ctx: typer.Context) -> None: @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def lcmspy(ctx: typer.Context) -> None: """Alias for `dimos spy --transport lcm`.""" - if any(arg == "--transport" or arg.startswith("--transport=") for arg in ctx.args): + args = list(ctx.args) + if any(arg == "--transport" or arg.startswith("--transport=") for arg in args): typer.echo( "Error: dimos lcmspy is LCM-only; use `dimos spy --transport ...` " "to choose transports.", @@ -641,7 +642,12 @@ def lcmspy(ctx: typer.Context) -> None: raise typer.Exit(2) from dimos.utils.cli.spy.run_spy import main as spy_main - sys.argv = ["spy", "--transport", "lcm", *ctx.args] + # `web` must stay first for run_spy.main() to enter web mode; the LCM + # filter then applies inside the served child. + if args and args[0] == "web": + sys.argv = ["spy", "web", "--transport", "lcm", *args[1:]] + else: + sys.argv = ["spy", "--transport", "lcm", *args] spy_main() diff --git a/dimos/robot/cli/test_dimos.py b/dimos/robot/cli/test_dimos.py index 5f9e8330a2..6b5dd9b18c 100644 --- a/dimos/robot/cli/test_dimos.py +++ b/dimos/robot/cli/test_dimos.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import sys from typing import Literal import pytest @@ -160,3 +161,32 @@ class TestModule(Module): " * testmodule.spam: str (default: eggs)", "", ] + + +@pytest.fixture +def spy_main_argv(monkeypatch): + """Stub run_spy.main and capture the sys.argv the lcmspy alias hands it.""" + import dimos.utils.cli.spy.run_spy as run_spy + + captured: list[list[str]] = [] + monkeypatch.setattr(sys, "argv", ["dimos"]) + monkeypatch.setattr(run_spy, "main", lambda: captured.append(list(sys.argv))) + return captured + + +def test_lcmspy_alias_prepends_lcm_transport(spy_main_argv): + result = CliRunner().invoke(main, ["lcmspy"]) + assert result.exit_code == 0, result.output + assert spy_main_argv == [["spy", "--transport", "lcm"]] + + +def test_lcmspy_alias_keeps_web_mode_first(spy_main_argv): + result = CliRunner().invoke(main, ["lcmspy", "web"]) + assert result.exit_code == 0, result.output + assert spy_main_argv == [["spy", "web", "--transport", "lcm"]] + + +def test_lcmspy_alias_rejects_transport_override(spy_main_argv): + result = CliRunner().invoke(main, ["lcmspy", "--transport", "zenoh"]) + assert result.exit_code == 2 + assert spy_main_argv == [] # never reaches the spy From bf1dac1b8d87ce686e1739c1ef243f3d058b1f28 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 19:59:18 +0300 Subject: [PATCH 08/33] revert: keep zenoh subscribe_all conflating; spy stays protocol-agnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverses the ratified subscribe_all non-conflation spec change (Ivan's call): - zenoh subscribe_all: restore original latest-per-topic drain over dimos/** - spec: drop SubscribeLatestMixin/subscribe_latest entirely - rerun bridge: back to subscribe_all (its original conflating consumer) - spy is transport-agnostic — it just calls bus.subscribe_all(), accepting whatever delivery semantics each transport provides (LCM: every message; zenoh: latest-per-topic). No raw/session-level special-casing. - drop the now-invalid contract tests: subscribe_all-delivers-every[zenoh] and subscribe_latest[*]; docstrings updated to match. Keeps the three Greptile fixes already on feat/ivan/spy (default_sources degradation, web-mode filter forwarding, lcmspy-web routing). --- dimos/protocol/pubsub/impl/zenohpubsub.py | 66 +++++--- dimos/protocol/pubsub/spec.py | 116 +------------- dimos/protocol/pubsub/spy.py | 13 +- dimos/protocol/pubsub/test_spy.py | 141 +----------------- dimos/visualization/rerun/bridge.py | 2 +- .../rerun/test_viewer_integration.py | 3 - 6 files changed, 64 insertions(+), 277 deletions(-) diff --git a/dimos/protocol/pubsub/impl/zenohpubsub.py b/dimos/protocol/pubsub/impl/zenohpubsub.py index 466eff6e1a..afaa413c43 100644 --- a/dimos/protocol/pubsub/impl/zenohpubsub.py +++ b/dimos/protocol/pubsub/impl/zenohpubsub.py @@ -197,31 +197,61 @@ def unsubscribe() -> None: return unsubscribe def subscribe_all(self, callback: Callable[[bytes, Topic], Any]) -> Callable[[], None]: - """Subscribe to every key, delivering every message (non-conflating). + """Subscribe to all dimos topics, delivering only the latest per topic. - "All topics" means everything this session can observe ('**'). Consumers - that only want the newest message per topic use subscribe_latest(). + Unlike `subscribe`, this is best effort. If it's done otherwise, rerun lags behind. """ - return self.subscribe(Topic("**"), callback) - - def _register_drain_stop( - self, stop_drain: Callable[[], None], thread: threading.Thread - ) -> bool: - # Register + start under the subscriber lock so stop() can't run between - # them and miss the thread. If stop() already ran, bail without starting. + latest: dict[str, tuple[bytes, Topic]] = {} + lock = threading.Lock() + wake = threading.Event() + stop = threading.Event() + + def collect(msg: bytes, topic: Topic) -> None: + # Fast path on the Zenoh delivery thread: keep only the newest per topic. + with lock: + latest[str(topic)] = (msg, topic) + wake.set() + + def drain() -> None: + while not stop.is_set(): + wake.wait() + wake.clear() + with lock: + batch = list(latest.values()) + latest.clear() + for msg, topic in batch: + try: + callback(msg, topic) + except Exception: + logger.error("Error in subscribe_all callback", exc_info=True) + + thread = threading.Thread(target=drain, name="zenoh-subscribe-all", daemon=True) + + def stop_drain() -> None: + stop.set() + wake.set() # unblock the drain so it observes the stop flag + thread.join(timeout=2.0) + + # Register the stop callback and launch the drain thread under the lock so + # stop() can't run between them and miss the thread. If stop() already ran, + # bail without starting anything. with self._subscriber_lock: if self._stopped: - return False + return lambda: None self._drain_stops.append(stop_drain) thread.start() - return True - def _unregister_drain_stop(self, stop_drain: Callable[[], None]) -> bool: - with self._subscriber_lock: - if stop_drain not in self._drain_stops: - return False # Already removed by stop() or a concurrent unsubscribe - self._drain_stops.remove(stop_drain) - return True + inner_unsub = self.subscribe(Topic("dimos/**"), collect) + + def unsubscribe() -> None: + with self._subscriber_lock: + if stop_drain not in self._drain_stops: + return # Already removed by stop() or a concurrent unsubscribe + self._drain_stops.remove(stop_drain) + inner_unsub() + stop_drain() + + return unsubscribe def stop(self) -> None: with self._subscriber_lock: diff --git a/dimos/protocol/pubsub/spec.py b/dimos/protocol/pubsub/spec.py index d9d3f2e888..fe979fce82 100644 --- a/dimos/protocol/pubsub/spec.py +++ b/dimos/protocol/pubsub/spec.py @@ -17,13 +17,8 @@ from collections.abc import AsyncIterator, Callable from contextlib import asynccontextmanager from dataclasses import dataclass -import threading from typing import Any, Generic, Protocol, TypeVar, runtime_checkable -from dimos.utils.logging_config import setup_logger - -logger = setup_logger() - MsgT = TypeVar("MsgT") TopicT = TypeVar("TopicT") MsgT_co = TypeVar("MsgT_co", covariant=True) @@ -120,100 +115,7 @@ def subscribe( # # - DiscoveryPubSub: Native support for discovering new topics as they appear. # Provides a default subscribe_all() by subscribing to each discovered topic. -class SubscribeLatestMixin(Generic[TopicT, MsgT], ABC): - """Conflated subscribe-all, built on top of subscribe_all(). - - Mixed into AllPubSub and DiscoveryPubSub so both expose subscribe_latest(). - """ - - @abstractmethod - def subscribe_all(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: ... - - def _register_drain_stop( - self, stop_drain: Callable[[], None], thread: threading.Thread - ) -> bool: - """Start ``thread`` and record ``stop_drain`` for transport-level shutdown. - - Default: just start the thread. Transports whose stop() must join live - drains (e.g. zenoh) override this to register under their own lock and - return False if already stopped (subscribe_latest then bails). - """ - thread.start() - return True - - def _unregister_drain_stop(self, stop_drain: Callable[[], None]) -> bool: - """Forget a drain stopper. Returns False if already removed.""" - return True - - def subscribe_latest(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: - """Subscribe to all topics, conflated: newest message per topic wins. - - For consumers that only need the current value per topic and must not - lag behind a fast producer (e.g. the rerun bridge). When the callback - is slower than the message flow, intermediate messages on a topic are - dropped and only the latest is delivered. - - Contract: - - The callback runs on a dedicated drain thread, never on the - transport's delivery thread. - - Per topic, the newest pending message is always eventually delivered - (no starvation); intermediate ones may be skipped. - - The returned callable unsubscribes the underlying subscribe_all and - stops+joins the drain thread. - """ - latest: dict[str, tuple[MsgT, TopicT]] = {} - lock = threading.Lock() - wake = threading.Event() - stop = threading.Event() - - def collect(msg: MsgT, topic: TopicT) -> None: - # Fast path on the transport's delivery thread: keep only newest per topic. - with lock: - latest[str(topic)] = (msg, topic) - wake.set() - - def drain() -> None: - while not stop.is_set(): - wake.wait() - wake.clear() - with lock: - batch = list(latest.values()) - latest.clear() - for msg, topic in batch: - try: - callback(msg, topic) - except Exception: - logger.error("Error in subscribe_latest callback", exc_info=True) - - thread = threading.Thread(target=drain, name="subscribe-latest-drain", daemon=True) - - def stop_drain() -> None: - stop.set() - wake.set() # unblock the drain so it observes the stop flag - thread.join(timeout=2.0) - - # Register + start the drain atomically w.r.t. transport shutdown, then - # subscribe. If the transport already stopped, bail without subscribing. - if not self._register_drain_stop(stop_drain, thread): - return lambda: None - try: - inner_unsub = self.subscribe_all(collect) - except BaseException: - # Don't leak the drain thread if the underlying subscribe fails. - if self._unregister_drain_stop(stop_drain): - stop_drain() - raise - - def unsubscribe() -> None: - if not self._unregister_drain_stop(stop_drain): - return # Already removed by stop() or a concurrent unsubscribe - inner_unsub() - stop_drain() - - return unsubscribe - - -class AllPubSub(SubscribeLatestMixin[TopicT, MsgT], PubSub[TopicT, MsgT], ABC): +class AllPubSub(PubSub[TopicT, MsgT], ABC): """Mixin for PubSub that supports subscribing to all topics. Subclass from this if you support native subscribe-all (e.g. MQTT #, Redis *). @@ -222,13 +124,7 @@ class AllPubSub(SubscribeLatestMixin[TopicT, MsgT], PubSub[TopicT, MsgT], ABC): @abstractmethod def subscribe_all(self, callback: Callable[[MsgT, TopicT], Any]) -> Callable[[], None]: - """Subscribe to all topics. - - Contract: delivers EVERY message on every topic this transport can - observe — implementations must not conflate, sample, or drop beyond - the transport's own delivery semantics. Consumers that only want the - newest message per topic use subscribe_latest() instead. - """ + """Subscribe to all topics.""" ... def subscribe_new_topics(self, callback: Callable[[TopicT], Any]) -> Callable[[], None]: @@ -248,7 +144,7 @@ def on_msg(msg: MsgT, topic: TopicT) -> None: # This is for ros for now -class DiscoveryPubSub(SubscribeLatestMixin[TopicT, MsgT], PubSub[TopicT, MsgT], ABC): +class DiscoveryPubSub(PubSub[TopicT, MsgT], ABC): """Mixin for PubSub that supports discovery of topics. Subclass from this if you support topic discovery (e.g. MQTT, Redis, NATS, RabbitMQ). @@ -291,9 +187,5 @@ class SubscribeAllCapable(Protocol[MsgT_co, TopicT_co]): """ def subscribe_all(self, callback: Callable[[Any, Any], Any]) -> Callable[[], None]: - """Subscribe to all messages on all topics (every message, no conflation).""" - ... - - def subscribe_latest(self, callback: Callable[[Any, Any], Any]) -> Callable[[], None]: - """Subscribe to all topics, conflated to the newest message per topic.""" + """Subscribe to all messages on all topics.""" ... diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index 7e7c97bbf6..eb2f01d091 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -131,8 +131,9 @@ class SpySource(Protocol): """One transport's raw firehose feeding the spy. Invariants for implementations: - - tap() delivers EVERY observable message exactly once per callback - (non-conflating; built on the raw-bytes bus's subscribe_all). + - tap() rides the raw-bytes bus's subscribe_all; delivery scope and + conflation follow that transport's semantics (LCM delivers every + message; zenoh's subscribe_all is latest-per-topic over dimos/**). - The tap callback receives (topic_str, nbytes) where topic_str is the uniform str(Topic) form and nbytes is the wire payload length. - Never decodes payloads, never retains them past the callback. @@ -182,11 +183,11 @@ def subscribe_decoded(self, topic: str, callback: Callable[[Any], None]) -> Call class ZenohSpySource: - """Spy source over zenoh, via ZenohPubSubBase.subscribe_all (all keys, '**'). + """Spy source over zenoh, via ZenohPubSubBase.subscribe_all. - Depends on the subscribe_all non-conflation fix (see TASK.md): every sample - must reach the tap. nbytes = payload length; topics arrive as str(Topic) - with the type suffix reconstructed from the key expression. + zenoh's subscribe_all is latest-per-topic over dimos/** (best-effort), so + same-topic bursts between drains conflate away. nbytes = payload length; + topics arrive as str(Topic) with the type suffix reconstructed from the key. """ name = "zenoh" diff --git a/dimos/protocol/pubsub/test_spy.py b/dimos/protocol/pubsub/test_spy.py index fb3215d769..65d8124031 100644 --- a/dimos/protocol/pubsub/test_spy.py +++ b/dimos/protocol/pubsub/test_spy.py @@ -16,8 +16,7 @@ These encode the acceptance criteria and FAIL until the task is implemented: - TopicStats: deterministic windowed stats from injected timestamps. -- subscribe_all: delivers EVERY message on LCM and zenoh (non-conflating). -- subscribe_latest: conflates to the newest message per topic. +- subscribe_all: LCM delivers every message (non-conflating regex '.*'). - SpySources: count every message without ever decoding a payload. - TransportSpy: merges sources into (transport, topic)-keyed stats + totals. - subscribe_decoded: spec'd hook, must stay NotImplementedError in v1. @@ -34,7 +33,6 @@ from dimos.msgs.geometry_msgs.Vector3 import Vector3 from dimos.protocol.pubsub.impl.lcmpubsub import LCMPubSubBase, Topic from dimos.protocol.pubsub.impl.zenohpubsub import ZenohPubSubBase -from dimos.protocol.pubsub.spec import AllPubSub from dimos.protocol.pubsub.spy import ( SOURCE_FACTORIES, LCMSpySource, @@ -119,24 +117,12 @@ def lcm_base_ctx(): bus.stop() -@contextmanager -def zenoh_base_ctx(): - pool = ZenohSessionPool() - bus = ZenohPubSubBase(session_pool=pool) - bus.start() - try: - yield bus, Topic("dimos/spy_contract", Vector3) - finally: - bus.stop() - pool.close_all() - - -@pytest.mark.parametrize("bus_ctx", [lcm_base_ctx, zenoh_base_ctx], ids=["lcm", "zenoh"]) +@pytest.mark.parametrize("bus_ctx", [lcm_base_ctx], ids=["lcm"]) def test_subscribe_all_delivers_every_message(bus_ctx): """A gated (slow) consumer must still receive every message eventually. - This is the sharp edge of the non-conflation contract: zenoh's original - subscribe_all conflated to latest-per-topic and fails this with ~2 of 50. + LCM's subscribe_all is non-conflating (regex '.*', 10k queue), so all 50 + messages are delivered even when the consumer lags the burst. """ n = 50 with bus_ctx() as (bus, topic): @@ -164,125 +150,6 @@ def cb(msg, t): unsub() -# subscribe_latest contract: conflation is the explicit opt-in - - -class FakeAllPubSub(AllPubSub[str, bytes]): - """In-memory AllPubSub: publish() synchronously fans out to subscribers.""" - - def __init__(self): - self._subs: list[Callable[[bytes, str], None]] = [] - - def publish(self, topic: str, message: bytes) -> None: - for cb in list(self._subs): - cb(message, topic) - - def subscribe(self, topic, callback): - return self.subscribe_all(callback) - - def subscribe_all(self, callback): - self._subs.append(callback) - - def unsub(): - if callback in self._subs: - self._subs.remove(callback) - - return unsub - - -def test_subscribe_latest_conflates_to_newest(): - bus = FakeAllPubSub() - delivered = [] - first = threading.Event() - gate = threading.Event() - quiet = threading.Event() - - def cb(msg, topic): - delivered.append((topic, msg)) - first.set() - gate.wait(10.0) # block the drain thread while the burst happens - if msg == b"99": - quiet.set() - - unsub = bus.subscribe_latest(cb) - bus.publish("t", b"0") - assert first.wait(5.0) - for i in range(1, 100): - bus.publish("t", str(i).encode()) - gate.set() - - assert quiet.wait(5.0) - # Newest message must arrive; the 98 intermediate ones conflate away. - assert delivered[-1] == ("t", b"99") - assert len(delivered) <= 5 - unsub() - - -def test_subscribe_latest_keeps_latest_per_topic(): - bus = FakeAllPubSub() - delivered = [] - first = threading.Event() - gate = threading.Event() - done = threading.Event() - - def cb(msg, topic): - delivered.append((topic, msg)) - first.set() - gate.wait(10.0) - if {t for t, _ in delivered} >= {"a", "b"} and len(delivered) >= 3: - done.set() - - unsub = bus.subscribe_latest(cb) - bus.publish("a", b"a0") - assert first.wait(5.0) - for i in range(1, 50): - bus.publish("a", f"a{i}".encode()) - bus.publish("b", f"b{i}".encode()) - gate.set() - - assert done.wait(5.0) - latest = dict(delivered) # last write per topic wins - assert latest["a"] == b"a49" - assert latest["b"] == b"b49" - unsub() - - -def test_subscribe_latest_unsubscribe_stops_delivery(): - bus = FakeAllPubSub() - delivered = [] - got_one = threading.Event() - - def cb(msg, topic): - delivered.append(msg) - got_one.set() - - unsub = bus.subscribe_latest(cb) - bus.publish("t", b"1") - assert got_one.wait(5.0) - unsub() - count = len(delivered) - bus.publish("t", b"2") - time.sleep(0.2) - assert len(delivered) == count - - -def test_subscribe_latest_failed_subscribe_stops_drain_thread(): - class FailingBus(FakeAllPubSub): - def subscribe_all(self, callback): - raise RuntimeError("subscriber declaration failed") - - bus = FailingBus() - before = set(threading.enumerate()) - with pytest.raises(RuntimeError, match="subscriber declaration failed"): - bus.subscribe_latest(lambda msg, topic: None) - leaked = [ - t - for t in threading.enumerate() - if t not in before and t.name == "subscribe-latest-drain" and t.is_alive() - ] - assert not leaked # drain thread must be stopped and joined on subscribe failure - - # SpySources: every message counted, payloads never decoded diff --git a/dimos/visualization/rerun/bridge.py b/dimos/visualization/rerun/bridge.py index 52821b99f3..960d5e7826 100644 --- a/dimos/visualization/rerun/bridge.py +++ b/dimos/visualization/rerun/bridge.py @@ -458,7 +458,7 @@ def start(self) -> None: logger.info(f"bridge listening on {pubsub.__class__.__name__}") if hasattr(pubsub, "start"): pubsub.start() - unsub = pubsub.subscribe_latest(self._on_message) + unsub = pubsub.subscribe_all(self._on_message) self.register_disposable(Disposable(unsub)) # Add pubsub stop as disposable diff --git a/dimos/visualization/rerun/test_viewer_integration.py b/dimos/visualization/rerun/test_viewer_integration.py index a4d5489e35..d6a0ba1e95 100644 --- a/dimos/visualization/rerun/test_viewer_integration.py +++ b/dimos/visualization/rerun/test_viewer_integration.py @@ -126,9 +126,6 @@ class ExplicitPubSubOverride: def subscribe_all(self, callback): return lambda: None - def subscribe_latest(self, callback): - return lambda: None - class TestBridgePubsubResolution: def test_legacy_lcm_pubsubs_defers_to_transport_default(self): From 851d86486cac0f1c76ec53d82c711fdeb9fa0772 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 20:10:30 +0300 Subject: [PATCH 09/33] feat: remove spy web mode; fix lcmspy console entry arg handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per maintainer decision, drop the browser 'web' mode entirely: - run_spy: remove the web branch + _web_command; main() just runs the TUI - docs: drop the 'dimos spy web' line Fix the standalone lcmspy console entry (Greptile P1): it discarded sys.argv[1:]. Extract _lcm_only_argv() — shared by lcm_main() and the dimos lcmspy alias — which forwards args to spy and rejects an explicit --transport override (LCM-only). Tests for forwarding + rejection on both entry points; drop the now-obsolete web tests. --- dimos/robot/cli/dimos.py | 17 ++----------- dimos/robot/cli/test_dimos.py | 9 ++++--- dimos/utils/cli/spy/run_spy.py | 38 ++++++++++++----------------- dimos/utils/cli/spy/test_run_spy.py | 34 ++++++++++++++++++++------ docs/usage/transports/spy.md | 1 - 5 files changed, 49 insertions(+), 50 deletions(-) diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index 7f98584b93..2a95951622 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -632,22 +632,9 @@ def spy(ctx: typer.Context) -> None: @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def lcmspy(ctx: typer.Context) -> None: """Alias for `dimos spy --transport lcm`.""" - args = list(ctx.args) - if any(arg == "--transport" or arg.startswith("--transport=") for arg in args): - typer.echo( - "Error: dimos lcmspy is LCM-only; use `dimos spy --transport ...` " - "to choose transports.", - err=True, - ) - raise typer.Exit(2) - from dimos.utils.cli.spy.run_spy import main as spy_main + from dimos.utils.cli.spy.run_spy import _lcm_only_argv, main as spy_main - # `web` must stay first for run_spy.main() to enter web mode; the LCM - # filter then applies inside the served child. - if args and args[0] == "web": - sys.argv = ["spy", "web", "--transport", "lcm", *args[1:]] - else: - sys.argv = ["spy", "--transport", "lcm", *args] + sys.argv = _lcm_only_argv(list(ctx.args)) spy_main() diff --git a/dimos/robot/cli/test_dimos.py b/dimos/robot/cli/test_dimos.py index 6b5dd9b18c..323ea33e5f 100644 --- a/dimos/robot/cli/test_dimos.py +++ b/dimos/robot/cli/test_dimos.py @@ -180,13 +180,14 @@ def test_lcmspy_alias_prepends_lcm_transport(spy_main_argv): assert spy_main_argv == [["spy", "--transport", "lcm"]] -def test_lcmspy_alias_keeps_web_mode_first(spy_main_argv): - result = CliRunner().invoke(main, ["lcmspy", "web"]) +def test_lcmspy_alias_forwards_extra_args(spy_main_argv): + result = CliRunner().invoke(main, ["lcmspy", "--foo"]) assert result.exit_code == 0, result.output - assert spy_main_argv == [["spy", "web", "--transport", "lcm"]] + assert spy_main_argv == [["spy", "--transport", "lcm", "--foo"]] def test_lcmspy_alias_rejects_transport_override(spy_main_argv): result = CliRunner().invoke(main, ["lcmspy", "--transport", "zenoh"]) - assert result.exit_code == 2 + assert result.exit_code == 1 + assert "LCM-only" in result.output assert spy_main_argv == [] # never reaches the spy diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 8e7b5342ad..957a498399 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -14,9 +14,8 @@ """`dimos spy` TUI: live table of all topics across all pubsub transports. -Textual app (DataTable, 0.5s refresh, theme colors, 'q' to quit, optional -`spy web` mode via textual-serve). One row per (transport, topic) from -TransportSpy.snapshot(): +Textual app (DataTable, 0.5s refresh, theme colors, 'q' to quit). One row per +(transport, topic) from TransportSpy.snapshot(): Transport | Topic | Type | Freq (Hz) | Bandwidth | Total | Age @@ -208,36 +207,31 @@ def refresh_table(self) -> None: ) -def _web_command(argv: list[str]) -> str: - """Command line for the textual-serve child: rerun this script with the filter args.""" - import os - import shlex +def main() -> None: + """Entry point for `dimos spy` (argv: --transport filters).""" import sys - return shlex.join([sys.executable, os.path.abspath(__file__), *argv]) - + SpyApp(transports=_parse_transports(sys.argv[1:])).run() -def main() -> None: - """Entry point for `dimos spy` (argv: optional 'web', --transport filters).""" - import sys - argv = sys.argv[1:] - if argv and argv[0] == "web": - from textual_serve.server import Server # type: ignore[import-not-found] +def _lcm_only_argv(args: list[str]) -> list[str]: + """Build the `spy` argv for the LCM-only entry points (`lcmspy` / `dimos lcmspy`). - rest = argv[1:] - _parse_transports(rest) # reject unknown transports before serving - server = Server(_web_command(rest)) - server.serve() - else: - SpyApp(transports=_parse_transports(argv)).run() + Rejects an explicit --transport override (these entry points can't choose + transports); all other args pass through to `spy`. + """ + if any(a == "--transport" or a.startswith("--transport=") for a in args): + raise SystemExit( + "Error: lcmspy is LCM-only; use `dimos spy --transport ...` to choose transports." + ) + return ["spy", "--transport", "lcm", *args] def lcm_main() -> None: """`lcmspy` console-script shim: the spy over the LCM source only.""" import sys - sys.argv = ["lcmspy", "--transport", "lcm"] + sys.argv = _lcm_only_argv(sys.argv[1:]) main() diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 17a73d04c8..1ac32d161b 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -12,19 +12,20 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""CLI arg handling for `dimos spy`: --transport parsing, validation, web mode.""" +"""CLI arg handling for `dimos spy`: --transport parsing, validation, LCM-only alias.""" +import sys from typing import Any import pytest from dimos.protocol.pubsub.spy import SOURCE_FACTORIES -from dimos.utils.cli.spy.run_spy import SpyApp, _parse_transports, _web_command +from dimos.utils.cli.spy import run_spy +from dimos.utils.cli.spy.run_spy import SpyApp, _lcm_only_argv, _parse_transports def test_parse_transports_default_is_none(): assert _parse_transports([]) is None - assert _parse_transports(["web"]) is None def test_parse_transports_collects_repeated_flags(): @@ -92,10 +93,27 @@ def unavailable(): SpyApp(transports=["zenoh"]) -def test_web_command_forwards_transport_filters(): - cmd = _web_command(["--transport", "zenoh"]) - assert cmd.endswith("run_spy.py --transport zenoh") +def test_lcm_only_argv_forwards_plain_args(): + assert _lcm_only_argv([]) == ["spy", "--transport", "lcm"] + assert _lcm_only_argv(["--foo", "bar"]) == ["spy", "--transport", "lcm", "--foo", "bar"] -def test_web_command_without_filters_serves_bare_script(): - assert _web_command([]).endswith("run_spy.py") +def test_lcm_only_argv_rejects_transport_override(): + with pytest.raises(SystemExit, match="LCM-only"): + _lcm_only_argv(["--transport", "zenoh"]) + with pytest.raises(SystemExit, match="LCM-only"): + _lcm_only_argv(["--transport=zenoh"]) + + +def test_lcm_main_forwards_args_to_spy(monkeypatch): + seen: list[str] = [] + monkeypatch.setattr(run_spy, "main", lambda: seen.extend(sys.argv)) + monkeypatch.setattr(sys, "argv", ["lcmspy", "--foo"]) + run_spy.lcm_main() + assert seen == ["spy", "--transport", "lcm", "--foo"] + + +def test_lcm_main_rejects_transport_override(monkeypatch): + monkeypatch.setattr(sys, "argv", ["lcmspy", "--transport", "zenoh"]) + with pytest.raises(SystemExit, match="LCM-only"): + run_spy.lcm_main() diff --git a/docs/usage/transports/spy.md b/docs/usage/transports/spy.md index d80a05b005..78cea53142 100644 --- a/docs/usage/transports/spy.md +++ b/docs/usage/transports/spy.md @@ -92,7 +92,6 @@ because unknown/untyped keys already fail type resolution and are skipped by its ```bash dimos spy # everything, all transports dimos spy --transport zenoh # filter to one transport (repeatable flag) -dimos spy web # serve the TUI in a browser (textual-serve) dimos lcmspy # deprecated alias for: dimos spy --transport lcm ``` From 1937f19ace99fdf97a5433f684f33292a0f02d49 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 20:19:16 +0300 Subject: [PATCH 10/33] fix: spy rejects stray positionals; default_sources degrades on any error Two Greptile P1s on PR #2735: - run_spy._parse_transports now rejects unexpected positionals/flags (and a --transport with no value) instead of silently ignoring them, so an unrecognized argument fails loudly rather than quietly launching the TUI. - default_sources() catches any construction failure (native init, ctor error), not just ImportError: warn + skip on the default path, error only if nothing constructs. Explicit --transport still hard-fails. Tests: stray-positional rejection (unit + CLI for spy and lcmspy), --transport missing-value, and a non-ImportError backend-skip case. --- dimos/protocol/pubsub/spy.py | 9 ++++--- dimos/robot/cli/test_dimos.py | 21 ++++++++++----- dimos/utils/cli/spy/run_spy.py | 15 +++++++++-- dimos/utils/cli/spy/test_run_spy.py | 41 +++++++++++++++++++++++++++-- 4 files changed, 72 insertions(+), 14 deletions(-) diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index eb2f01d091..82fe54a5dc 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -293,15 +293,16 @@ def snapshot(self) -> dict[SpyKey, TopicStats]: def default_sources() -> list[SpySource]: """The spy observes ALL transports simultaneously, regardless of DIMOS_TRANSPORT. - A backend that is not importable is skipped with a warning, so the default - spy degrades to whatever transports are available. Requesting a transport - explicitly (constructing its SOURCE_FACTORIES entry) keeps the hard error. + A backend that fails to construct (missing import, native init error, …) is + skipped with a warning, so the default spy degrades to whatever transports + are available. Requesting a transport explicitly (constructing its + SOURCE_FACTORIES entry) keeps the hard error. """ sources: list[SpySource] = [] for name, factory in SOURCE_FACTORIES.items(): try: sources.append(factory()) - except ImportError as exc: + except Exception as exc: # degrade on any backend init failure, not just imports print(f"spy: skipping unavailable transport {name!r}: {exc}", file=sys.stderr) if not sources: raise RuntimeError(f"no spy transports available (tried: {', '.join(SOURCE_FACTORIES)})") diff --git a/dimos/robot/cli/test_dimos.py b/dimos/robot/cli/test_dimos.py index 323ea33e5f..0c0f1ba33c 100644 --- a/dimos/robot/cli/test_dimos.py +++ b/dimos/robot/cli/test_dimos.py @@ -180,14 +180,23 @@ def test_lcmspy_alias_prepends_lcm_transport(spy_main_argv): assert spy_main_argv == [["spy", "--transport", "lcm"]] -def test_lcmspy_alias_forwards_extra_args(spy_main_argv): - result = CliRunner().invoke(main, ["lcmspy", "--foo"]) - assert result.exit_code == 0, result.output - assert spy_main_argv == [["spy", "--transport", "lcm", "--foo"]] - - def test_lcmspy_alias_rejects_transport_override(spy_main_argv): result = CliRunner().invoke(main, ["lcmspy", "--transport", "zenoh"]) assert result.exit_code == 1 assert "LCM-only" in result.output assert spy_main_argv == [] # never reaches the spy + + +def test_spy_cmd_rejects_stray_positional(monkeypatch): + # A stray positional must fail loudly, not silently launch the TUI. + monkeypatch.setattr(sys, "argv", ["dimos"]) + result = CliRunner().invoke(main, ["spy", "foo"]) + assert result.exit_code == 1 + assert "unexpected" in result.output.lower() + + +def test_lcmspy_rejects_stray_positional(monkeypatch): + monkeypatch.setattr(sys, "argv", ["dimos"]) + result = CliRunner().invoke(main, ["lcmspy", "foo"]) + assert result.exit_code == 1 + assert "unexpected" in result.output.lower() diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 957a498399..17c3294bc6 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -70,10 +70,12 @@ def topic_text(base: str) -> Text: def _parse_transports(argv: list[str]) -> list[str] | None: """Parse repeated --transport flags; None means all default sources. - Exits with an error (rather than launching an empty spy) if any requested - transport is not a known source name. + `dimos spy` accepts only `--transport ` (repeatable). Any other token + — a stray positional or unknown flag — is rejected with a clear error rather + than silently ignored. """ transports: list[str] = [] + extra: list[str] = [] i = 0 while i < len(argv): arg = argv[i] @@ -81,9 +83,18 @@ def _parse_transports(argv: list[str]) -> list[str] | None: i += 1 if i < len(argv): transports.append(argv[i]) + else: + raise SystemExit("Error: --transport requires a transport name") elif arg.startswith("--transport="): transports.append(arg.split("=", 1)[1]) + else: + extra.append(arg) i += 1 + if extra: + raise SystemExit( + f"Error: unexpected argument(s) {', '.join(extra)} — " + "`dimos spy` accepts only --transport (repeatable)." + ) unknown = [t for t in transports if t not in SOURCE_FACTORIES] if unknown: valid = ", ".join(SOURCE_FACTORIES) diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 1ac32d161b..02a5f248b8 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -44,6 +44,19 @@ def test_parse_transports_error_lists_valid_choices(): _parse_transports(["--transport=nope"]) +def test_parse_transports_rejects_unexpected_positional(): + # A stray positional must fail loudly, not be silently ignored. + with pytest.raises(SystemExit, match="unexpected"): + _parse_transports(["foo"]) + with pytest.raises(SystemExit, match="unexpected"): + _parse_transports(["--transport", "lcm", "foo"]) + + +def test_parse_transports_missing_value_errors(): + with pytest.raises(SystemExit, match="requires a transport name"): + _parse_transports(["--transport"]) + + class _StubSource: name = "zenoh" @@ -84,12 +97,36 @@ def stub_factory() -> _StubSource: assert "lcm" in capsys.readouterr().err +def test_spy_app_default_skips_non_import_backend_failure(monkeypatch, capsys): + # Any construction failure (e.g. a native init error), not just ImportError, + # must degrade rather than kill the default spy. + def broken(): + raise RuntimeError("zenoh native init failed") + + created: list[_StubSource] = [] + + def stub_factory() -> _StubSource: + source = _StubSource() + created.append(source) + return source + + monkeypatch.setitem(SOURCE_FACTORIES, "lcm", broken) + monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", stub_factory) + app = SpyApp() # no --transport: degrades past the broken backend + try: + assert [s.started for s in created] == [True] + finally: + app.spy.stop() + err = capsys.readouterr().err + assert "lcm" in err and "native init failed" in err + + def test_spy_app_explicit_unavailable_transport_is_hard_error(monkeypatch): def unavailable(): - raise ImportError("zenoh backend missing") + raise RuntimeError("zenoh backend missing") monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", unavailable) - with pytest.raises(ImportError, match="zenoh backend missing"): + with pytest.raises(RuntimeError, match="zenoh backend missing"): SpyApp(transports=["zenoh"]) From 2d710f7188f9158c821e0df3836cd99d5557d960 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 20:30:17 +0300 Subject: [PATCH 11/33] fix: spy default path survives per-source start failures Greptile P1 on PR #2735: default `dimos spy` could construct both sources then die if one failed during start()/tap(), because TransportSpy.start() always rolled everything back and re-raised. Add a best_effort mode to TransportSpy.start(): a source that fails to start/tap is stopped, warned, and skipped, leaving the survivors running; it raises only if no source starts at all. Track live sources so stop() only touches ones that actually started. SpyApp uses best_effort on the default (no --transport) path; an explicit --transport keeps strict all-or-nothing rollback. Ctrl-C/SystemExit still roll back and propagate in both modes. Tests: best-effort skip-with-survivor + all-fail-raises at the TransportSpy level, and SpyApp default-degrades vs explicit-hard-fails wiring. --- dimos/protocol/pubsub/spy.py | 48 +++++++++++++++++++++-------- dimos/protocol/pubsub/test_spy.py | 25 +++++++++++++++ dimos/utils/cli/spy/run_spy.py | 4 ++- dimos/utils/cli/spy/test_run_spy.py | 34 ++++++++++++++++++++ 4 files changed, 98 insertions(+), 13 deletions(-) diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index 82fe54a5dc..35def26ac8 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -29,6 +29,7 @@ from __future__ import annotations from collections import deque +import contextlib from dataclasses import dataclass import sys import threading @@ -235,6 +236,7 @@ def __init__( self._stats: dict[SpyKey, TopicStats] = {} self._lock = threading.Lock() self._untaps: list[Callable[[], None]] = [] + self._live: list[SpySource] = [] # sources currently started + tapped self.totals = TopicStats(history_window=history_window) def _tap_callback(self, transport: str) -> Callable[[str, int], None]: @@ -251,28 +253,50 @@ def on_message(topic: str, nbytes: int) -> None: return on_message - def start(self) -> None: - """Start and tap every source; on failure roll back the ones already started.""" - started: list[SpySource] = [] + def _start_one(self, source: SpySource) -> None: + """Start + tap one source; if tap fails, stop it before propagating.""" + source.start() try: - for source in self._sources: - source.start() - started.append(source) - self._untaps.append(source.tap(self._tap_callback(source.name))) + untap = source.tap(self._tap_callback(source.name)) except BaseException: - for untap in self._untaps: - untap() - self._untaps.clear() - for source in reversed(started): + with contextlib.suppress(Exception): source.stop() raise + self._live.append(source) + self._untaps.append(untap) + + def start(self, best_effort: bool = False) -> None: + """Start and tap every source. + + Strict (default): all-or-nothing — if any source fails to start or tap, + roll back the ones already started and re-raise. + best_effort: a source that fails start()/tap() is warned and skipped + (its own partial start is undone); the survivors keep running. Raises + only if no source starts at all. + """ + try: + for source in self._sources: + try: + self._start_one(source) + except Exception as exc: + if not best_effort: + raise + print(f"spy: skipping transport {source.name!r}: {exc}", file=sys.stderr) + except BaseException: + self.stop() # roll back everything started so far, then propagate + raise + if best_effort and not self._live: + raise RuntimeError( + f"no spy transports could start (tried: {', '.join(s.name for s in self._sources)})" + ) def stop(self) -> None: for untap in self._untaps: untap() self._untaps.clear() - for source in self._sources: + for source in self._live: source.stop() + self._live.clear() def snapshot(self) -> dict[SpyKey, TopicStats]: """Current per-topic stats, safe to iterate while messages keep arriving.""" diff --git a/dimos/protocol/pubsub/test_spy.py b/dimos/protocol/pubsub/test_spy.py index 65d8124031..9bd894ab81 100644 --- a/dimos/protocol/pubsub/test_spy.py +++ b/dimos/protocol/pubsub/test_spy.py @@ -344,6 +344,31 @@ def tap(self, callback): assert not a.taps +class _FailingStartSource(FakeSource): + def start(self) -> None: + raise RuntimeError("backend down") + + +def test_transport_spy_best_effort_skips_failed_source(capsys): + good, bad = FakeSource("lcm"), _FailingStartSource("zenoh") + spy = TransportSpy(sources=[good, bad]) + spy.start(best_effort=True) # degrade to the survivor instead of dying + try: + assert good.started and good.taps # survivor is live and tapped + assert not bad.started + good.emit("/t", 5) + assert spy.snapshot()[SpyKey("lcm", "/t")].total_bytes == 5 + finally: + spy.stop() + assert "zenoh" in capsys.readouterr().err # skipped source is warned about + + +def test_transport_spy_best_effort_raises_when_all_fail(): + spy = TransportSpy(sources=[_FailingStartSource("lcm"), _FailingStartSource("zenoh")]) + with pytest.raises(RuntimeError, match="no spy transports could start"): + spy.start(best_effort=True) + + def test_default_sources_skips_unavailable_backend(monkeypatch, capsys): def unavailable(): raise ImportError("zenoh backend missing") diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 17c3294bc6..ecae2ad3f5 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -158,7 +158,9 @@ def __init__(self, transports: list[str] | None = None, **kwargs) -> None: # ty autoconf(check_only=True) self.spy = TransportSpy(sources=sources) - self.spy.start() + # Default (all transports): a backend that fails to start is skipped so + # the spy still shows the others. An explicit --transport hard-fails. + self.spy.start(best_effort=transports is None) self.table: DataTable | None = None # type: ignore[type-arg] def compose(self) -> ComposeResult: diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 02a5f248b8..561d879d5b 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -130,6 +130,40 @@ def unavailable(): SpyApp(transports=["zenoh"]) +class _FailingStartSource(_StubSource): + name = "zenoh" + + def start(self) -> None: + raise RuntimeError("zenoh start failed") + + +def test_spy_app_default_survives_backend_start_failure(monkeypatch, capsys): + # A backend that constructs but dies during start() must not kill the + # default spy — the survivor keeps running. + survivors: list[_StubSource] = [] + + def good_factory() -> _StubSource: + source = _StubSource() + source.name = "lcm" + survivors.append(source) + return source + + monkeypatch.setitem(SOURCE_FACTORIES, "lcm", good_factory) + monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", _FailingStartSource) + app = SpyApp() # default path: best-effort + try: + assert survivors and survivors[0].started + finally: + app.spy.stop() + assert "zenoh" in capsys.readouterr().err + + +def test_spy_app_explicit_start_failure_is_hard_error(monkeypatch): + monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", _FailingStartSource) + with pytest.raises(RuntimeError, match="zenoh start failed"): + SpyApp(transports=["zenoh"]) # explicit: strict, no degrade + + def test_lcm_only_argv_forwards_plain_args(): assert _lcm_only_argv([]) == ["spy", "--transport", "lcm"] assert _lcm_only_argv(["--foo", "bar"]) == ["spy", "--transport", "lcm", "--foo", "bar"] From d517f286e3a0ee3c9d11bbf1bd739904061a382f Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 20:36:36 +0300 Subject: [PATCH 12/33] docs: fold dimos spy page into the transports doc The standalone transports/spy.md is now a few lines, so merge it into the Transports doc's CLI section (retitled 'Inspecting traffic') and drop the page from the nav. Update the spy.py module docstring to point at the transports doc instead of the removed page. --- dimos/protocol/pubsub/spy.py | 2 +- docs/docs.json | 3 +- docs/usage/transports/index.md | 14 ++++- docs/usage/transports/spy.md | 103 --------------------------------- 4 files changed, 13 insertions(+), 109 deletions(-) delete mode 100644 docs/usage/transports/spy.md diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index 35def26ac8..180541792b 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -14,7 +14,7 @@ """Transport-agnostic pubsub spy: topic discovery, rates, sizes, liveness. -Design doc: docs/usage/transports/spy.md. Task spec: TASK.md (branch agent/spy-architect). +Docs: docs/usage/transports/index.md ("Inspecting traffic"). HARD CONSTRAINT: the spy never decodes message payloads. Sources tap the raw-bytes pubsub layer (LCMPubSubBase, ZenohPubSubBase — beneath the encoder diff --git a/docs/docs.json b/docs/docs.json index f1cb8c2b86..7cdbc96204 100644 --- a/docs/docs.json +++ b/docs/docs.json @@ -73,8 +73,7 @@ "group": "Transports", "pages": [ "usage/transports/index", - "usage/transports/dds", - "usage/transports/spy" + "usage/transports/dds" ] } ] diff --git a/docs/usage/transports/index.md b/docs/usage/transports/index.md index 06d0b62636..889881b4c1 100644 --- a/docs/usage/transports/index.md +++ b/docs/usage/transports/index.md @@ -248,11 +248,19 @@ Received: (480, 640, 3) See [Modules](/docs/usage/modules.md) for more on module architecture. -## Inspecting LCM traffic (CLI) +## Inspecting traffic (CLI) -`lcmspy` shows topic frequency/bandwidth stats: +`dimos spy` is the universal transport spy: one live view of every topic moving on every +DimOS pubsub transport — names, message rates, bandwidth, sizes, and liveness — whether the +system runs on LCM, Zenoh, or both. -![lcmspy](../assets/lcmspy.png) +```bash +dimos spy # everything, all transports +dimos spy --transport zenoh # filter to one transport (repeatable flag) +dimos lcmspy # deprecated alias for: dimos spy --transport lcm +``` + +![dimos spy](../assets/lcmspy.png) `dimos topic echo /topic` listens on typed channels like `/topic#pkg.Msg` and decodes automatically: diff --git a/docs/usage/transports/spy.md b/docs/usage/transports/spy.md deleted file mode 100644 index 78cea53142..0000000000 --- a/docs/usage/transports/spy.md +++ /dev/null @@ -1,103 +0,0 @@ ---- -title: "dimos spy" ---- - -`dimos spy` is the universal transport spy: one live view of every topic moving on every -DimOS pubsub transport — names, message rates, bandwidth, sizes, and liveness — regardless -of whether the system runs on LCM, Zenoh, or both. - -> Status: design ratified, implementation in progress. This page is the design doc; -> it becomes the user doc as the tool lands. - -## Motivation - -`dimos lcmspy` only sees LCM. Since the Zenoh transport merged (#2362), a robot can run -with `--transport=zenoh` (default on macOS) and the spy goes blind. Debugging "is data -flowing?" should not require knowing which backend is active — the spy should observe -everything at once. - -## The hard constraint: no decoding on the hot path - -Decoding messages is expensive (LCM decode, image payloads, pointclouds). The spy's job -is *flow* visibility — topic names, Hz, bytes/s, last-seen — none of which needs message -contents. Therefore: - -**The spy never decodes a payload.** Its per-message hot path is -`(topic string, payload length, timestamp)` — an O(1) stats update. - -This falls out of the pubsub layering: encoding/decoding lives in `PubSubEncoderMixin` -subclasses (`LCM`, `Zenoh`), while the base classes (`LCMPubSubBase`, `ZenohPubSubBase`) -move raw bytes. The spy taps the base layer. Message *types* are still displayed for -free because DimOS embeds them in topic strings (`/cmd_vel#geometry_msgs.Twist` on LCM, -`dimos/cmd_vel/geometry_msgs.Twist` as a Zenoh key — both normalize to the `#` form via -`str(Topic)`). - -Per-topic decoded inspection (echo/preview) is a separate, opt-in concern with its own -subscription (`SpySource.subscribe_decoded`), spec'd now and implemented in a follow-up. - -## Architecture - -``` -LCMPubSubBase.subscribe_all(".*") ZenohPubSubBase.subscribe_all("**") - │ raw bytes │ raw bytes - LCMSpySource ("lcm") ZenohSpySource ("zenoh") - │ (topic_str, nbytes) │ (topic_str, nbytes) - └──────────────┬───────────────────────┘ - TransportSpy - dict[SpyKey(transport, topic) → TopicStats] + totals - │ snapshot() - dimos spy TUI - Transport | Topic | Type | Freq | Bandwidth | Total | Age -``` - -- **`SpySource`** (Protocol): one transport's raw firehose. `tap(cb)` delivers - `(topic, nbytes)` for *every* observable message. v1 ships `lcm` and `zenoh` sources; - SHM/ROS/DDS/Redis plug in later by implementing the protocol. -- **`TopicStats`**: sliding-window `(timestamp, nbytes)` history per topic — freq, - bytes/s, avg size over a window, plus lifetime totals and `last_seen` for liveness. - Explicit timestamps keep it deterministic under test. -- **`TransportSpy`**: owns sources, merges taps, serves thread-safe snapshots to the UI. - -Code: `dimos/protocol/pubsub/spy.py` (core), `dimos/utils/cli/spy/run_spy.py` (TUI). - -## The subscribe_all contract fix - -The spy builds on the pubsub layer's subscribe-all abstraction -(`dimos/protocol/pubsub/spec.py`), which exposed a semantic asymmetry: - -- LCM's `subscribe_all` delivered every message (regex `.*`). -- Zenoh's `subscribe_all` **conflated** — latest-wins per topic with a drain thread, - scoped to `dimos/**` — because it was built for its only consumer, the rerun bridge. - -A spy counting rates through a conflating tap silently under-reports. Ratified decision -(Ivan, ticket f6c74d39): **`subscribe_all` is defined non-conflating on every -transport.** Conflation becomes the explicit opt-in `subscribe_latest()` (same -latest-wins + drain-thread semantics, hosted in the spec as a default), and the rerun -bridge migrates to it. Zenoh's `subscribe_all` also widens from `dimos/**` to `**` — -"all topics" means everything the transport can see; the rerun bridge is unaffected -because unknown/untyped keys already fail type resolution and are skipped by its decoder. - -## Decisions (ticket f6c74d39) - -| # | Decision | -|---|----------| -| 1 | `subscribe_all` = every message, non-conflating, on all transports; conflation moves to opt-in `subscribe_latest()`; rerun bridge migrates. Confirmed by Ivan. | -| 2 | v1 sources: LCM + Zenoh, behind the pluggable `SpySource` protocol. SHM/ROS/DDS/Redis later (SHM needs a discovery mechanism first). | -| 3 | `dimos spy` shows **all transports simultaneously**, rows tagged by transport; `dimos lcmspy` becomes a deprecated alias. | -| 4 | Lazy per-topic decode is spec'd (`subscribe_decoded`) but not implemented in v1 — decode stays off the hot path. | -| 5 | This design doc lives in dimos `docs/`. | - -## CLI - -```bash -dimos spy # everything, all transports -dimos spy --transport zenoh # filter to one transport (repeatable flag) -dimos lcmspy # deprecated alias for: dimos spy --transport lcm -``` - -## Future work - -- SHM source (needs channel discovery), ROS/DDS/Redis sources. -- Per-topic decoded echo/preview from the TUI row (`subscribe_decoded` follow-up). -- Zero-copy zenoh sizing (`len(sample.payload)` without `to_bytes()`), if profiling - shows the copy matters for pointcloud-heavy systems. From a2e02c1b0b4eb912dcbcba1e382ebbc141c00788 Mon Sep 17 00:00:00 2001 From: Ivan Nikolic Date: Mon, 6 Jul 2026 20:37:01 +0300 Subject: [PATCH 13/33] comment correction --- dimos/protocol/pubsub/spy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dimos/protocol/pubsub/spy.py b/dimos/protocol/pubsub/spy.py index 180541792b..901873e265 100644 --- a/dimos/protocol/pubsub/spy.py +++ b/dimos/protocol/pubsub/spy.py @@ -21,9 +21,6 @@ mixins), so the hot path per message is (topic string, payload length, timestamp) and nothing else. Message *types* are still visible because they are embedded in the topic string ("/cmd_vel#geometry_msgs.Twist"). - -Decoding is a separate, per-topic, opt-in concern: SpySource.subscribe_decoded -is the spec'd hook, not implemented in v1. """ from __future__ import annotations @@ -281,7 +278,10 @@ def start(self, best_effort: bool = False) -> None: except Exception as exc: if not best_effort: raise - print(f"spy: skipping transport {source.name!r}: {exc}", file=sys.stderr) + print( + f"spy: skipping transport {source.name!r}: {exc}", + file=sys.stderr, + ) except BaseException: self.stop() # roll back everything started so far, then propagate raise From 00071addde121ac1dc7a354e2ac01826b44b4a2a Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Mon, 6 Jul 2026 20:41:13 +0300 Subject: [PATCH 14/33] refactor: move spy core out of protocol/pubsub into the spy tool package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spy core (TopicStats, SpySource, LCM/Zenoh sources, TransportSpy) is an observability tool built on top of the pubsub layer, not part of it — it only depends on the pubsub impls, and its only consumers are the spy CLI and tests. Sitting in protocol/pubsub/ was a layering inversion. Move it next to run_spy: dimos/protocol/pubsub/spy.py -> dimos/utils/cli/spy/core.py dimos/protocol/pubsub/test_spy.py -> dimos/utils/cli/spy/test_spy.py Imports updated; no behavior change. pubsub suite and spy tests green. --- dimos/{protocol/pubsub/spy.py => utils/cli/spy/core.py} | 0 dimos/utils/cli/spy/run_spy.py | 4 ++-- dimos/utils/cli/spy/test_run_spy.py | 2 +- dimos/{protocol/pubsub => utils/cli/spy}/test_spy.py | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) rename dimos/{protocol/pubsub/spy.py => utils/cli/spy/core.py} (100%) rename dimos/{protocol/pubsub => utils/cli/spy}/test_spy.py (99%) diff --git a/dimos/protocol/pubsub/spy.py b/dimos/utils/cli/spy/core.py similarity index 100% rename from dimos/protocol/pubsub/spy.py rename to dimos/utils/cli/spy/core.py diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index ecae2ad3f5..4545f6ec35 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -35,7 +35,8 @@ from textual.color import Color from textual.widgets import DataTable -from dimos.protocol.pubsub.spy import ( +from dimos.utils.cli import theme +from dimos.utils.cli.spy.core import ( SOURCE_FACTORIES, SpyKey, TopicStats, @@ -43,7 +44,6 @@ default_sources, split_type_suffix, ) -from dimos.utils.cli import theme from dimos.utils.human import human_bytes # Rows older than this (seconds since last message) render dimmed as "stale". diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 561d879d5b..1d5ccb7971 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -19,8 +19,8 @@ import pytest -from dimos.protocol.pubsub.spy import SOURCE_FACTORIES from dimos.utils.cli.spy import run_spy +from dimos.utils.cli.spy.core import SOURCE_FACTORIES from dimos.utils.cli.spy.run_spy import SpyApp, _lcm_only_argv, _parse_transports diff --git a/dimos/protocol/pubsub/test_spy.py b/dimos/utils/cli/spy/test_spy.py similarity index 99% rename from dimos/protocol/pubsub/test_spy.py rename to dimos/utils/cli/spy/test_spy.py index 9bd894ab81..95af9c3167 100644 --- a/dimos/protocol/pubsub/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -33,7 +33,8 @@ from dimos.msgs.geometry_msgs.Vector3 import Vector3 from dimos.protocol.pubsub.impl.lcmpubsub import LCMPubSubBase, Topic from dimos.protocol.pubsub.impl.zenohpubsub import ZenohPubSubBase -from dimos.protocol.pubsub.spy import ( +from dimos.protocol.service.zenohservice import ZenohSessionPool +from dimos.utils.cli.spy.core import ( SOURCE_FACTORIES, LCMSpySource, SpyKey, @@ -43,7 +44,6 @@ default_sources, split_type_suffix, ) -from dimos.protocol.service.zenohservice import ZenohSessionPool VEC = Vector3(1.0, 2.0, 3.0) VEC_BYTES = VEC.lcm_encode() @@ -391,7 +391,7 @@ def unavailable(): def test_fake_source_satisfies_protocol(): - from dimos.protocol.pubsub.spy import SpySource + from dimos.utils.cli.spy.core import SpySource assert isinstance(FakeSource("x"), SpySource) From c97b24f818507ab5e969dc2c6968f083ef635a5e Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Tue, 7 Jul 2026 01:26:21 +0300 Subject: [PATCH 15/33] docs: document dimos spy in cli.md, mark lcmspy as deprecated alias Follow-up to folding the spy page into the transports doc: cli.md still described lcmspy as the LCM monitor. Add a `dimos spy` command entry (universal transport spy) and demote the standalone `lcmspy` entry to a deprecated alias for `dimos spy --transport lcm`. --- docs/usage/cli.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/usage/cli.md b/docs/usage/cli.md index dc4de5d3c9..33329c7feb 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -201,6 +201,16 @@ Print resolved GlobalConfig values and their sources. dimos show-config ``` +### `dimos spy` + +Universal transport spy: a live table of every topic on every pubsub transport (LCM, Zenoh, or both), with per-topic message rate, bandwidth, size, and liveness. + +```bash +dimos spy # everything, all transports +dimos spy --transport zenoh # filter to one transport (repeatable flag) +dimos lcmspy # deprecated alias for: dimos spy --transport lcm +``` + --- ## Agent & MCP Commands @@ -297,7 +307,7 @@ humancli ### `lcmspy` -Monitor LCM messages in real time. +Deprecated alias for `dimos spy --transport lcm` (the LCM-only view of the spy). Prefer [`dimos spy`](#dimos-spy). ```bash lcmspy From 3608002fbf2900dbb861ee00b941a68caf40a42c Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:35:42 +0300 Subject: [PATCH 16/33] refactor(spy): rename _lcm_only_argv to lcm_only_argv It is imported by dimos/robot/cli/dimos.py, so it is public API of run_spy, not a module-private helper. --- dimos/robot/cli/dimos.py | 4 ++-- dimos/utils/cli/spy/run_spy.py | 4 ++-- dimos/utils/cli/spy/test_run_spy.py | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index 2a95951622..c274406f82 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -632,9 +632,9 @@ def spy(ctx: typer.Context) -> None: @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def lcmspy(ctx: typer.Context) -> None: """Alias for `dimos spy --transport lcm`.""" - from dimos.utils.cli.spy.run_spy import _lcm_only_argv, main as spy_main + from dimos.utils.cli.spy.run_spy import lcm_only_argv, main as spy_main - sys.argv = _lcm_only_argv(list(ctx.args)) + sys.argv = lcm_only_argv(list(ctx.args)) spy_main() diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 4545f6ec35..4dc53386b1 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -227,7 +227,7 @@ def main() -> None: SpyApp(transports=_parse_transports(sys.argv[1:])).run() -def _lcm_only_argv(args: list[str]) -> list[str]: +def lcm_only_argv(args: list[str]) -> list[str]: """Build the `spy` argv for the LCM-only entry points (`lcmspy` / `dimos lcmspy`). Rejects an explicit --transport override (these entry points can't choose @@ -244,7 +244,7 @@ def lcm_main() -> None: """`lcmspy` console-script shim: the spy over the LCM source only.""" import sys - sys.argv = _lcm_only_argv(sys.argv[1:]) + sys.argv = lcm_only_argv(sys.argv[1:]) main() diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 1d5ccb7971..5cf8d64909 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -21,7 +21,7 @@ from dimos.utils.cli.spy import run_spy from dimos.utils.cli.spy.core import SOURCE_FACTORIES -from dimos.utils.cli.spy.run_spy import SpyApp, _lcm_only_argv, _parse_transports +from dimos.utils.cli.spy.run_spy import SpyApp, _parse_transports, lcm_only_argv def test_parse_transports_default_is_none(): @@ -165,15 +165,15 @@ def test_spy_app_explicit_start_failure_is_hard_error(monkeypatch): def test_lcm_only_argv_forwards_plain_args(): - assert _lcm_only_argv([]) == ["spy", "--transport", "lcm"] - assert _lcm_only_argv(["--foo", "bar"]) == ["spy", "--transport", "lcm", "--foo", "bar"] + assert lcm_only_argv([]) == ["spy", "--transport", "lcm"] + assert lcm_only_argv(["--foo", "bar"]) == ["spy", "--transport", "lcm", "--foo", "bar"] def test_lcm_only_argv_rejects_transport_override(): with pytest.raises(SystemExit, match="LCM-only"): - _lcm_only_argv(["--transport", "zenoh"]) + lcm_only_argv(["--transport", "zenoh"]) with pytest.raises(SystemExit, match="LCM-only"): - _lcm_only_argv(["--transport=zenoh"]) + lcm_only_argv(["--transport=zenoh"]) def test_lcm_main_forwards_args_to_spy(monkeypatch): From 4f7a6f644ad97ffc49fef99180e35a3b727ec6cc Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:36:49 +0300 Subject: [PATCH 17/33] refactor(spy): move inline imports to the top of their modules import sys in run_spy and the test-only imports had no reason to be inline. The lazy backend imports in core.py stay inline but now carry a comment saying why (an unavailable backend must not break the others). --- dimos/robot/cli/test_dimos.py | 3 +-- dimos/utils/cli/spy/core.py | 4 ++++ dimos/utils/cli/spy/run_spy.py | 5 +---- dimos/utils/cli/spy/test_spy.py | 3 +-- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/dimos/robot/cli/test_dimos.py b/dimos/robot/cli/test_dimos.py index 0c0f1ba33c..8bcd6d3d8a 100644 --- a/dimos/robot/cli/test_dimos.py +++ b/dimos/robot/cli/test_dimos.py @@ -22,6 +22,7 @@ from dimos.core.global_config import global_config from dimos.core.module import Module, ModuleConfig from dimos.robot.cli.dimos import _normalize_simulation_argv, arg_help, main +import dimos.utils.cli.spy.run_spy as run_spy @pytest.mark.parametrize( @@ -166,8 +167,6 @@ class TestModule(Module): @pytest.fixture def spy_main_argv(monkeypatch): """Stub run_spy.main and capture the sys.argv the lcmspy alias hands it.""" - import dimos.utils.cli.spy.run_spy as run_spy - captured: list[list[str]] = [] monkeypatch.setattr(sys, "argv", ["dimos"]) monkeypatch.setattr(run_spy, "main", lambda: captured.append(list(sys.argv))) diff --git a/dimos/utils/cli/spy/core.py b/dimos/utils/cli/spy/core.py index 901873e265..be51e52927 100644 --- a/dimos/utils/cli/spy/core.py +++ b/dimos/utils/cli/spy/core.py @@ -162,6 +162,8 @@ class LCMSpySource: def __init__(self, **lcm_kwargs: Any) -> None: """lcm_kwargs forwarded to LCMPubSubBase (e.g. lcm_url).""" + # Inline import: an unavailable LCM backend must not break the other + # spy sources (see default_sources). from dimos.protocol.pubsub.impl.lcmpubsub import LCMPubSubBase self._bus = LCMPubSubBase(**lcm_kwargs) @@ -192,6 +194,8 @@ class ZenohSpySource: def __init__(self, **zenoh_kwargs: Any) -> None: """zenoh_kwargs forwarded to ZenohPubSubBase (e.g. mode/connect/listen, session_pool).""" + # Inline import: an unavailable zenoh backend must not break the other + # spy sources (see default_sources). from dimos.protocol.pubsub.impl.zenohpubsub import ZenohPubSubBase self._bus = ZenohPubSubBase(**zenoh_kwargs) diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 4dc53386b1..130973f21f 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -28,6 +28,7 @@ from __future__ import annotations +import sys import time from rich.text import Text @@ -222,8 +223,6 @@ def refresh_table(self) -> None: def main() -> None: """Entry point for `dimos spy` (argv: --transport filters).""" - import sys - SpyApp(transports=_parse_transports(sys.argv[1:])).run() @@ -242,8 +241,6 @@ def lcm_only_argv(args: list[str]) -> list[str]: def lcm_main() -> None: """`lcmspy` console-script shim: the spy over the LCM source only.""" - import sys - sys.argv = lcm_only_argv(sys.argv[1:]) main() diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index 95af9c3167..7d7a9d7654 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -38,6 +38,7 @@ SOURCE_FACTORIES, LCMSpySource, SpyKey, + SpySource, TopicStats, TransportSpy, ZenohSpySource, @@ -391,8 +392,6 @@ def unavailable(): def test_fake_source_satisfies_protocol(): - from dimos.utils.cli.spy.core import SpySource - assert isinstance(FakeSource("x"), SpySource) From 92695befdabae68bda4cf306dac254963fe69560 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:37:29 +0300 Subject: [PATCH 18/33] refactor(spy): share the default history window as one constant The 60.0s default was duplicated in TopicStats.__init__ and TransportSpy.__init__. --- dimos/utils/cli/spy/core.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dimos/utils/cli/spy/core.py b/dimos/utils/cli/spy/core.py index be51e52927..026bb69548 100644 --- a/dimos/utils/cli/spy/core.py +++ b/dimos/utils/cli/spy/core.py @@ -39,6 +39,9 @@ # The entire hot-path event: (topic string incl. '#type' suffix, wire payload length). TapCallback = Callable[[str, int], None] +# Seconds of per-message history kept for windowed stats. +DEFAULT_HISTORY_WINDOW = 60.0 + @dataclass(frozen=True, slots=True) class SpyKey: @@ -79,7 +82,7 @@ class TopicStats: total_msgs: int last_seen: float | None # timestamp of newest recorded message, None if none yet - def __init__(self, history_window: float = 60.0) -> None: + def __init__(self, history_window: float = DEFAULT_HISTORY_WINDOW) -> None: """history_window: seconds of per-message history kept for windowed stats. total_bytes/total_msgs/last_seen survive eviction; only windowed stats @@ -229,7 +232,9 @@ class TransportSpy: totals: TopicStats # all messages across all sources def __init__( - self, sources: Sequence[SpySource] | None = None, history_window: float = 60.0 + self, + sources: Sequence[SpySource] | None = None, + history_window: float = DEFAULT_HISTORY_WINDOW, ) -> None: """sources=None means default_sources().""" self._sources = list(sources) if sources is not None else default_sources() From 7ab39dfea51bf6b9949c1a12fc2d6b55a64f09a5 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:37:29 +0300 Subject: [PATCH 19/33] refactor(spy): name the gradient saturation magic numbers --- dimos/utils/cli/spy/run_spy.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 130973f21f..ac9f461bdc 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -51,6 +51,9 @@ STALE_AGE = 3.0 # Window for freq / bandwidth readouts. STAT_WINDOW = 5.0 +# Values at which the freq / bandwidth cell colors saturate. +FREQ_GRADIENT_MAX_HZ = 10.0 +BANDWIDTH_GRADIENT_MAX_BPS = 3 * 1024 def gradient(max_value: float, value: float) -> str: @@ -214,8 +217,8 @@ def refresh_table(self) -> None: Text(key.transport, style=theme.BLUE), topic_text(base), Text(msg_type or "", style=theme.BLUE), - Text(f"{freq:.1f}", style=gradient(10, freq)), - Text(f"{human_bytes(bps)}/s", style=gradient(1024 * 3, bps)), + Text(f"{freq:.1f}", style=gradient(FREQ_GRADIENT_MAX_HZ, freq)), + Text(f"{human_bytes(bps)}/s", style=gradient(BANDWIDTH_GRADIENT_MAX_BPS, bps)), Text(human_bytes(stats.total_bytes)), Text(age_str, style=theme.ACCENT), ) From 6cc7b26cfeb83fed65b5f64acffc4dfb2700ab99 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:38:20 +0300 Subject: [PATCH 20/33] fix(spy): resolve type ignores in run_spy with real types - App -> App[None], **kwargs: Any, DataTable -> DataTable[Text] - cursor_type=None -> the documented "none" CursorType literal; drop zebra_stripes=False (the default) - narrow self.table once at the top of refresh_table instead of three union-attr ignores --- dimos/utils/cli/spy/run_spy.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index ac9f461bdc..ee1d179ded 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -30,6 +30,7 @@ import sys import time +from typing import Any from rich.text import Text from textual.app import App, ComposeResult @@ -108,7 +109,7 @@ def _parse_transports(argv: list[str]) -> list[str] | None: return transports or None -class SpyApp(App): # type: ignore[type-arg] +class SpyApp(App[None]): """A real-time dashboard for all-transport pubsub traffic using Textual.""" CSS_PATH = "../dimos.tcss" @@ -138,7 +139,7 @@ class SpyApp(App): # type: ignore[type-arg] ("ctrl+c", "quit"), ] - def __init__(self, transports: list[str] | None = None, **kwargs) -> None: # type: ignore[no-untyped-def] + def __init__(self, transports: list[str] | None = None, **kwargs: Any) -> None: super().__init__(**kwargs) if transports is None: # Default: every available transport; unavailable backends are @@ -165,10 +166,10 @@ def __init__(self, transports: list[str] | None = None, **kwargs) -> None: # ty # Default (all transports): a backend that fails to start is skipped so # the spy still shows the others. An explicit --transport hard-fails. self.spy.start(best_effort=transports is None) - self.table: DataTable | None = None # type: ignore[type-arg] + self.table: DataTable[Text] | None = None def compose(self) -> ComposeResult: - self.table = DataTable(zebra_stripes=False, cursor_type=None) # type: ignore[arg-type] + self.table = DataTable(cursor_type="none") self.table.add_column("Transport") self.table.add_column("Topic") self.table.add_column("Type") @@ -185,12 +186,15 @@ async def on_unmount(self) -> None: self.spy.stop() def refresh_table(self) -> None: + table = self.table + if table is None: + return now = time.time() snap = self.spy.snapshot() rows: list[tuple[SpyKey, TopicStats]] = sorted( snap.items(), key=lambda kv: kv[1].total_bytes, reverse=True ) - self.table.clear(columns=False) # type: ignore[union-attr] + table.clear(columns=False) for key, stats in rows: base, msg_type = split_type_suffix(key.topic) @@ -201,7 +205,7 @@ def refresh_table(self) -> None: if stale: # Liveness: dim the whole row for topics gone quiet. - self.table.add_row( # type: ignore[union-attr] + table.add_row( Text(key.transport, style=theme.DIM), Text(base, style=theme.DIM), Text(msg_type or "", style=theme.DIM), @@ -213,7 +217,7 @@ def refresh_table(self) -> None: continue age_str = f"{age:.1f}s" if age is not None else "-" - self.table.add_row( # type: ignore[union-attr] + table.add_row( Text(key.transport, style=theme.BLUE), topic_text(base), Text(msg_type or "", style=theme.BLUE), From 9a19f0393c768c043433aa0c3a47c36aba082cf3 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:38:57 +0300 Subject: [PATCH 21/33] refactor(spy): extract duplicated unknown-transport validation _parse_transports and SpyApp.__init__ built near-identical error messages; both now use validate_transport_names in core, with the CLI converting the ValueError to SystemExit. --- dimos/utils/cli/spy/core.py | 10 ++++++++++ dimos/utils/cli/spy/run_spy.py | 18 ++++++------------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/dimos/utils/cli/spy/core.py b/dimos/utils/cli/spy/core.py index 026bb69548..7b865d62ec 100644 --- a/dimos/utils/cli/spy/core.py +++ b/dimos/utils/cli/spy/core.py @@ -323,6 +323,16 @@ def snapshot(self) -> dict[SpyKey, TopicStats]: """ +def validate_transport_names(names: Sequence[str]) -> None: + """Raise ValueError if any name is not a known SOURCE_FACTORIES transport.""" + unknown = [n for n in names if n not in SOURCE_FACTORIES] + if unknown: + raise ValueError( + f"unknown transport(s) {', '.join(unknown)} — valid choices: " + f"{', '.join(SOURCE_FACTORIES)}" + ) + + def default_sources() -> list[SpySource]: """The spy observes ALL transports simultaneously, regardless of DIMOS_TRANSPORT. diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index ee1d179ded..acc3734f73 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -45,6 +45,7 @@ TransportSpy, default_sources, split_type_suffix, + validate_transport_names, ) from dimos.utils.human import human_bytes @@ -100,12 +101,10 @@ def _parse_transports(argv: list[str]) -> list[str] | None: f"Error: unexpected argument(s) {', '.join(extra)} — " "`dimos spy` accepts only --transport (repeatable)." ) - unknown = [t for t in transports if t not in SOURCE_FACTORIES] - if unknown: - valid = ", ".join(SOURCE_FACTORIES) - raise SystemExit( - f"Error: unknown transport(s) {', '.join(unknown)} — valid choices: {valid}" - ) + try: + validate_transport_names(transports) + except ValueError as exc: + raise SystemExit(f"Error: {exc}") from None return transports or None @@ -146,12 +145,7 @@ def __init__(self, transports: list[str] | None = None, **kwargs: Any) -> None: # skipped with a warning (see default_sources). sources = default_sources() else: - unknown = [n for n in transports if n not in SOURCE_FACTORIES] - if unknown: - raise ValueError( - f"unknown transport(s) {', '.join(unknown)} — valid choices: " - f"{', '.join(SOURCE_FACTORIES)}" - ) + validate_transport_names(transports) # Construct only the requested sources: a filtered-out transport is # never imported or instantiated, and an unavailable one that was # explicitly requested stays a hard error. From 2e0df3e2a5f88eda633c15cbd6b572327f46e7aa Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:40:28 +0300 Subject: [PATCH 22/33] refactor(spy): stop doing work in SpyApp.__init__ Source construction/validation, the autoconf warning, and spy.start() move to a new start_spy() called from main(), which now owns the spy's lifecycle with try/finally around app.run(). SpyApp just receives the started TransportSpy, so a failed start can no longer leave a half-built app. Tests exercise start_spy() through a fixture that always stops the spy on teardown. --- dimos/utils/cli/spy/run_spy.py | 61 +++++++++++++++++------------ dimos/utils/cli/spy/test_run_spy.py | 55 ++++++++++++++------------ 2 files changed, 67 insertions(+), 49 deletions(-) diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index acc3734f73..22bf3a4d34 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -138,28 +138,10 @@ class SpyApp(App[None]): ("ctrl+c", "quit"), ] - def __init__(self, transports: list[str] | None = None, **kwargs: Any) -> None: + def __init__(self, spy: TransportSpy, **kwargs: Any) -> None: + """spy: an already-started TransportSpy; the caller owns its lifecycle.""" super().__init__(**kwargs) - if transports is None: - # Default: every available transport; unavailable backends are - # skipped with a warning (see default_sources). - sources = default_sources() - else: - validate_transport_names(transports) - # Construct only the requested sources: a filtered-out transport is - # never imported or instantiated, and an unavailable one that was - # explicitly requested stays a hard error. - sources = [SOURCE_FACTORIES[name]() for name in transports] - # Warn about missing system config before entering TUI raw mode (LCM only). - if any(s.name == "lcm" for s in sources): - from dimos.protocol.service.lcmservice import autoconf - - autoconf(check_only=True) - - self.spy = TransportSpy(sources=sources) - # Default (all transports): a backend that fails to start is skipped so - # the spy still shows the others. An explicit --transport hard-fails. - self.spy.start(best_effort=transports is None) + self.spy = spy self.table: DataTable[Text] | None = None def compose(self) -> ComposeResult: @@ -176,9 +158,6 @@ def compose(self) -> ComposeResult: def on_mount(self) -> None: self.set_interval(self.refresh_interval, self.refresh_table) - async def on_unmount(self) -> None: - self.spy.stop() - def refresh_table(self) -> None: table = self.table if table is None: @@ -222,9 +201,41 @@ def refresh_table(self) -> None: ) +def start_spy(transports: list[str] | None = None) -> TransportSpy: + """Build sources for the requested transports and start a TransportSpy. + + Runs before the TUI enters raw mode, so autoconf and degrade warnings + print to a normal terminal. transports=None means every available + transport, best-effort: a backend that fails to construct or start is + skipped with a warning so the spy still shows the others. An explicit + transport list is strict: unknown names and construction/start failures + are hard errors. + """ + if transports is None: + sources = default_sources() + else: + validate_transport_names(transports) + # Construct only the requested sources: a filtered-out transport is + # never imported or instantiated, and an unavailable one that was + # explicitly requested stays a hard error. + sources = [SOURCE_FACTORIES[name]() for name in transports] + # Warn about missing system config before entering TUI raw mode (LCM only). + if any(s.name == "lcm" for s in sources): + from dimos.protocol.service.lcmservice import autoconf + + autoconf(check_only=True) + spy = TransportSpy(sources=sources) + spy.start(best_effort=transports is None) + return spy + + def main() -> None: """Entry point for `dimos spy` (argv: --transport filters).""" - SpyApp(transports=_parse_transports(sys.argv[1:])).run() + spy = start_spy(_parse_transports(sys.argv[1:])) + try: + SpyApp(spy).run() + finally: + spy.stop() def lcm_only_argv(args: list[str]) -> list[str]: diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 5cf8d64909..7838a3317d 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -20,8 +20,8 @@ import pytest from dimos.utils.cli.spy import run_spy -from dimos.utils.cli.spy.core import SOURCE_FACTORIES -from dimos.utils.cli.spy.run_spy import SpyApp, _parse_transports, lcm_only_argv +from dimos.utils.cli.spy.core import SOURCE_FACTORIES, TransportSpy +from dimos.utils.cli.spy.run_spy import _parse_transports, lcm_only_argv, start_spy def test_parse_transports_default_is_none(): @@ -76,7 +76,23 @@ def subscribe_decoded(self, topic: str, callback: Any) -> Any: raise NotImplementedError -def test_spy_app_default_skips_unavailable_backend(monkeypatch, capsys): +@pytest.fixture +def spy_starter(request): + """start_spy wrapper whose spies are stopped on teardown, even on failure. + + A factory (not a started spy) so each test can patch SOURCE_FACTORIES + before construction happens. + """ + + def _start(transports: list[str] | None = None) -> TransportSpy: + spy = start_spy(transports) + request.addfinalizer(spy.stop) + return spy + + return _start + + +def test_start_spy_default_skips_unavailable_backend(monkeypatch, capsys, spy_starter): def unavailable(): raise ImportError("lcm backend missing") @@ -89,15 +105,12 @@ def stub_factory() -> _StubSource: monkeypatch.setitem(SOURCE_FACTORIES, "lcm", unavailable) monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", stub_factory) - app = SpyApp() # no --transport: degrades to the available backend - try: - assert [s.started for s in created] == [True] - finally: - app.spy.stop() + spy_starter() # no --transport: degrades to the available backend + assert [s.started for s in created] == [True] assert "lcm" in capsys.readouterr().err -def test_spy_app_default_skips_non_import_backend_failure(monkeypatch, capsys): +def test_start_spy_default_skips_non_import_backend_failure(monkeypatch, capsys, spy_starter): # Any construction failure (e.g. a native init error), not just ImportError, # must degrade rather than kill the default spy. def broken(): @@ -112,22 +125,19 @@ def stub_factory() -> _StubSource: monkeypatch.setitem(SOURCE_FACTORIES, "lcm", broken) monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", stub_factory) - app = SpyApp() # no --transport: degrades past the broken backend - try: - assert [s.started for s in created] == [True] - finally: - app.spy.stop() + spy_starter() # no --transport: degrades past the broken backend + assert [s.started for s in created] == [True] err = capsys.readouterr().err assert "lcm" in err and "native init failed" in err -def test_spy_app_explicit_unavailable_transport_is_hard_error(monkeypatch): +def test_start_spy_explicit_unavailable_transport_is_hard_error(monkeypatch): def unavailable(): raise RuntimeError("zenoh backend missing") monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", unavailable) with pytest.raises(RuntimeError, match="zenoh backend missing"): - SpyApp(transports=["zenoh"]) + start_spy(transports=["zenoh"]) class _FailingStartSource(_StubSource): @@ -137,7 +147,7 @@ def start(self) -> None: raise RuntimeError("zenoh start failed") -def test_spy_app_default_survives_backend_start_failure(monkeypatch, capsys): +def test_start_spy_default_survives_backend_start_failure(monkeypatch, capsys, spy_starter): # A backend that constructs but dies during start() must not kill the # default spy — the survivor keeps running. survivors: list[_StubSource] = [] @@ -150,18 +160,15 @@ def good_factory() -> _StubSource: monkeypatch.setitem(SOURCE_FACTORIES, "lcm", good_factory) monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", _FailingStartSource) - app = SpyApp() # default path: best-effort - try: - assert survivors and survivors[0].started - finally: - app.spy.stop() + spy_starter() # default path: best-effort + assert survivors and survivors[0].started assert "zenoh" in capsys.readouterr().err -def test_spy_app_explicit_start_failure_is_hard_error(monkeypatch): +def test_start_spy_explicit_start_failure_is_hard_error(monkeypatch): monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", _FailingStartSource) with pytest.raises(RuntimeError, match="zenoh start failed"): - SpyApp(transports=["zenoh"]) # explicit: strict, no degrade + start_spy(transports=["zenoh"]) # explicit: strict, no degrade def test_lcm_only_argv_forwards_plain_args(): From a01cbe191cafc7f70b1714ccd753a67fc2ce9d74 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:42:15 +0300 Subject: [PATCH 23/33] fix(spy): emit degrade warnings via the project logger Replace the two print(..., file=sys.stderr) warnings in core with structured logger.warning calls. Tests assert on caplog (via a shared spy_warnings fixture that hooks the non-propagating dimos logger) instead of capsys. --- dimos/utils/cli/spy/conftest.py | 39 +++++++++++++++++++++++++++++ dimos/utils/cli/spy/core.py | 14 +++++++---- dimos/utils/cli/spy/test_run_spy.py | 13 +++++----- dimos/utils/cli/spy/test_spy.py | 8 +++--- 4 files changed, 58 insertions(+), 16 deletions(-) create mode 100644 dimos/utils/cli/spy/conftest.py diff --git a/dimos/utils/cli/spy/conftest.py b/dimos/utils/cli/spy/conftest.py new file mode 100644 index 0000000000..ae182fbfd1 --- /dev/null +++ b/dimos/utils/cli/spy/conftest.py @@ -0,0 +1,39 @@ +# Copyright 2026 Dimensional Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Shared fixtures for the spy tests.""" + +import logging + +import pytest + +# core.py logs through this stdlib logger name (setup_logger() derives it +# from the module's file path). +_CORE_LOGGER = "dimos/utils/cli/spy/core.py" + + +@pytest.fixture +def spy_warnings(caplog): + """Capture spy core log lines via ``caplog``. + + The dimos logger is structlog over a stdlib logger with + ``propagate=False``, so caplog's root-level handler never sees it. + """ + lg = logging.getLogger(_CORE_LOGGER) + lg.addHandler(caplog.handler) + caplog.set_level(logging.WARNING, logger=_CORE_LOGGER) + try: + yield caplog + finally: + lg.removeHandler(caplog.handler) diff --git a/dimos/utils/cli/spy/core.py b/dimos/utils/cli/spy/core.py index 7b865d62ec..fdf5cc799f 100644 --- a/dimos/utils/cli/spy/core.py +++ b/dimos/utils/cli/spy/core.py @@ -28,17 +28,20 @@ from collections import deque import contextlib from dataclasses import dataclass -import sys import threading import time from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable +from dimos.utils.logging_config import setup_logger + if TYPE_CHECKING: from collections.abc import Callable, Sequence # The entire hot-path event: (topic string incl. '#type' suffix, wire payload length). TapCallback = Callable[[str, int], None] +logger = setup_logger() + # Seconds of per-message history kept for windowed stats. DEFAULT_HISTORY_WINDOW = 60.0 @@ -287,9 +290,10 @@ def start(self, best_effort: bool = False) -> None: except Exception as exc: if not best_effort: raise - print( - f"spy: skipping transport {source.name!r}: {exc}", - file=sys.stderr, + logger.warning( + "Skipping spy transport that failed to start", + transport=source.name, + error=str(exc), ) except BaseException: self.stop() # roll back everything started so far, then propagate @@ -346,7 +350,7 @@ def default_sources() -> list[SpySource]: try: sources.append(factory()) except Exception as exc: # degrade on any backend init failure, not just imports - print(f"spy: skipping unavailable transport {name!r}: {exc}", file=sys.stderr) + logger.warning("Skipping unavailable spy transport", transport=name, error=str(exc)) if not sources: raise RuntimeError(f"no spy transports available (tried: {', '.join(SOURCE_FACTORIES)})") return sources diff --git a/dimos/utils/cli/spy/test_run_spy.py b/dimos/utils/cli/spy/test_run_spy.py index 7838a3317d..bda5cf6112 100644 --- a/dimos/utils/cli/spy/test_run_spy.py +++ b/dimos/utils/cli/spy/test_run_spy.py @@ -92,7 +92,7 @@ def _start(transports: list[str] | None = None) -> TransportSpy: return _start -def test_start_spy_default_skips_unavailable_backend(monkeypatch, capsys, spy_starter): +def test_start_spy_default_skips_unavailable_backend(monkeypatch, spy_warnings, spy_starter): def unavailable(): raise ImportError("lcm backend missing") @@ -107,10 +107,10 @@ def stub_factory() -> _StubSource: monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", stub_factory) spy_starter() # no --transport: degrades to the available backend assert [s.started for s in created] == [True] - assert "lcm" in capsys.readouterr().err + assert "lcm" in spy_warnings.text -def test_start_spy_default_skips_non_import_backend_failure(monkeypatch, capsys, spy_starter): +def test_start_spy_default_skips_non_import_backend_failure(monkeypatch, spy_warnings, spy_starter): # Any construction failure (e.g. a native init error), not just ImportError, # must degrade rather than kill the default spy. def broken(): @@ -127,8 +127,7 @@ def stub_factory() -> _StubSource: monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", stub_factory) spy_starter() # no --transport: degrades past the broken backend assert [s.started for s in created] == [True] - err = capsys.readouterr().err - assert "lcm" in err and "native init failed" in err + assert "lcm" in spy_warnings.text and "native init failed" in spy_warnings.text def test_start_spy_explicit_unavailable_transport_is_hard_error(monkeypatch): @@ -147,7 +146,7 @@ def start(self) -> None: raise RuntimeError("zenoh start failed") -def test_start_spy_default_survives_backend_start_failure(monkeypatch, capsys, spy_starter): +def test_start_spy_default_survives_backend_start_failure(monkeypatch, spy_warnings, spy_starter): # A backend that constructs but dies during start() must not kill the # default spy — the survivor keeps running. survivors: list[_StubSource] = [] @@ -162,7 +161,7 @@ def good_factory() -> _StubSource: monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", _FailingStartSource) spy_starter() # default path: best-effort assert survivors and survivors[0].started - assert "zenoh" in capsys.readouterr().err + assert "zenoh" in spy_warnings.text def test_start_spy_explicit_start_failure_is_hard_error(monkeypatch): diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index 7d7a9d7654..e624b200d0 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -350,7 +350,7 @@ def start(self) -> None: raise RuntimeError("backend down") -def test_transport_spy_best_effort_skips_failed_source(capsys): +def test_transport_spy_best_effort_skips_failed_source(spy_warnings): good, bad = FakeSource("lcm"), _FailingStartSource("zenoh") spy = TransportSpy(sources=[good, bad]) spy.start(best_effort=True) # degrade to the survivor instead of dying @@ -361,7 +361,7 @@ def test_transport_spy_best_effort_skips_failed_source(capsys): assert spy.snapshot()[SpyKey("lcm", "/t")].total_bytes == 5 finally: spy.stop() - assert "zenoh" in capsys.readouterr().err # skipped source is warned about + assert "zenoh" in spy_warnings.text # skipped source is warned about def test_transport_spy_best_effort_raises_when_all_fail(): @@ -370,7 +370,7 @@ def test_transport_spy_best_effort_raises_when_all_fail(): spy.start(best_effort=True) -def test_default_sources_skips_unavailable_backend(monkeypatch, capsys): +def test_default_sources_skips_unavailable_backend(monkeypatch, spy_warnings): def unavailable(): raise ImportError("zenoh backend missing") @@ -378,7 +378,7 @@ def unavailable(): monkeypatch.setitem(SOURCE_FACTORIES, "zenoh", unavailable) sources = default_sources() assert [s.name for s in sources] == ["lcm"] # degrades instead of crashing - assert "zenoh" in capsys.readouterr().err + assert "zenoh" in spy_warnings.text def test_default_sources_errors_when_no_backend_available(monkeypatch): From e914ff948ed7e7836deabc2940067bfd73f1f704 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:42:54 +0300 Subject: [PATCH 24/33] fix(spy): don't abort TransportSpy.stop() on the first error If one untap() or source.stop() raised, the remaining sources were never stopped. Each failure is now logged and the shutdown loop continues. --- dimos/utils/cli/spy/core.py | 11 +++++++++-- dimos/utils/cli/spy/test_spy.py | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/dimos/utils/cli/spy/core.py b/dimos/utils/cli/spy/core.py index fdf5cc799f..eb186506ff 100644 --- a/dimos/utils/cli/spy/core.py +++ b/dimos/utils/cli/spy/core.py @@ -304,11 +304,18 @@ def start(self, best_effort: bool = False) -> None: ) def stop(self) -> None: + """Untap and stop everything; an error in one source never skips the rest.""" for untap in self._untaps: - untap() + try: + untap() + except Exception as exc: + logger.warning("Error untapping spy source", error=str(exc)) self._untaps.clear() for source in self._live: - source.stop() + try: + source.stop() + except Exception as exc: + logger.warning("Error stopping spy source", transport=source.name, error=str(exc)) self._live.clear() def snapshot(self) -> dict[SpyKey, TopicStats]: diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index e624b200d0..450cfc9e21 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -345,6 +345,20 @@ def tap(self, callback): assert not a.taps +def test_transport_spy_stop_continues_past_failing_source(spy_warnings): + class FailingStopSource(FakeSource): + def stop(self) -> None: + raise RuntimeError("stop exploded") + + a, b = FailingStopSource("lcm"), FakeSource("zenoh") + spy = TransportSpy(sources=[a, b]) + spy.start() + spy.stop() # must not raise, and must still stop the healthy source + assert not b.started + assert not a.taps and not b.taps + assert "stop exploded" in spy_warnings.text + + class _FailingStartSource(FakeSource): def start(self) -> None: raise RuntimeError("backend down") From 095feec85405430ea8785802cabde14b9b31c5c7 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Tue, 7 Jul 2026 01:43:04 +0300 Subject: [PATCH 25/33] test: guarantee spy subscription/lifecycle cleanup on assertion failure Wrap the subscribe_all delivery test's unsub() and the two FakeSource TransportSpy tests' stop() in try/finally so a failing assertion no longer leaks the subscription / leaves the spy running. The source-counting tests and the SpyApp tests already clean up in finally. --- dimos/utils/cli/spy/test_spy.py | 67 ++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index 95af9c3167..c32dca7d9b 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -137,17 +137,19 @@ def cb(msg, t): done.set() unsub = bus.subscribe_all(cb) - time.sleep(0.5) # let the wildcard subscription establish + try: + time.sleep(0.5) # let the wildcard subscription establish - for _ in range(n): - bus.publish(topic, VEC_BYTES) - gate.set() + for _ in range(n): + bus.publish(topic, VEC_BYTES) + gate.set() - done.wait(10.0) - ours = [g for g in got if g[0] == str(topic)] - assert len(ours) == n - assert all(size == len(VEC_BYTES) for _, size in ours) - unsub() + done.wait(10.0) + ours = [g for g in got if g[0] == str(topic)] + assert len(ours) == n + assert all(size == len(VEC_BYTES) for _, size in ours) + finally: + unsub() # SpySources: every message counted, payloads never decoded @@ -286,22 +288,23 @@ def test_transport_spy_merges_sources_and_totals(): a, b = FakeSource("lcm"), FakeSource("zenoh") spy = TransportSpy(sources=[a, b]) spy.start() - assert a.started and b.started - - a.emit("/t#geometry_msgs.Twist", 10) - a.emit("/t#geometry_msgs.Twist", 10) - b.emit("dimos/t#geometry_msgs.Twist", 20) - a.emit("/u", 5) - - snap = spy.snapshot() - assert snap[SpyKey("lcm", "/t#geometry_msgs.Twist")].total_msgs == 2 - assert snap[SpyKey("lcm", "/t#geometry_msgs.Twist")].total_bytes == 20 - assert snap[SpyKey("zenoh", "dimos/t#geometry_msgs.Twist")].total_msgs == 1 - assert snap[SpyKey("lcm", "/u")].total_bytes == 5 - assert spy.totals.total_msgs == 4 - assert spy.totals.total_bytes == 45 - - spy.stop() + try: + assert a.started and b.started + + a.emit("/t#geometry_msgs.Twist", 10) + a.emit("/t#geometry_msgs.Twist", 10) + b.emit("dimos/t#geometry_msgs.Twist", 20) + a.emit("/u", 5) + + snap = spy.snapshot() + assert snap[SpyKey("lcm", "/t#geometry_msgs.Twist")].total_msgs == 2 + assert snap[SpyKey("lcm", "/t#geometry_msgs.Twist")].total_bytes == 20 + assert snap[SpyKey("zenoh", "dimos/t#geometry_msgs.Twist")].total_msgs == 1 + assert snap[SpyKey("lcm", "/u")].total_bytes == 5 + assert spy.totals.total_msgs == 4 + assert spy.totals.total_bytes == 45 + finally: + spy.stop() assert not a.started and not b.started assert not a.taps and not b.taps # untapped on stop @@ -310,12 +313,14 @@ def test_transport_spy_snapshot_is_stable_copy(): a = FakeSource("lcm") spy = TransportSpy(sources=[a]) spy.start() - a.emit("/t", 1) - snap = spy.snapshot() - a.emit("/new_topic_after_snapshot", 1) - assert SpyKey("lcm", "/new_topic_after_snapshot") not in snap # snapshot doesn't mutate - assert SpyKey("lcm", "/new_topic_after_snapshot") in spy.snapshot() - spy.stop() + try: + a.emit("/t", 1) + snap = spy.snapshot() + a.emit("/new_topic_after_snapshot", 1) + assert SpyKey("lcm", "/new_topic_after_snapshot") not in snap # snapshot doesn't mutate + assert SpyKey("lcm", "/new_topic_after_snapshot") in spy.snapshot() + finally: + spy.stop() def test_transport_spy_start_failure_stops_started_sources(): From 79967db40bbf3152051688364132df43344d81c9 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:43:43 +0300 Subject: [PATCH 26/33] refactor(spy): remove unused TopicStats.avg_size Only tests called it; the TUI shows freq, bandwidth, total and age. --- dimos/utils/cli/spy/core.py | 7 +------ dimos/utils/cli/spy/test_spy.py | 2 -- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/dimos/utils/cli/spy/core.py b/dimos/utils/cli/spy/core.py index eb186506ff..408f65d4be 100644 --- a/dimos/utils/cli/spy/core.py +++ b/dimos/utils/cli/spy/core.py @@ -89,7 +89,7 @@ def __init__(self, history_window: float = DEFAULT_HISTORY_WINDOW) -> None: """history_window: seconds of per-message history kept for windowed stats. total_bytes/total_msgs/last_seen survive eviction; only windowed stats - (freq/bytes_per_sec/avg_size) forget evicted messages. + (freq/bytes_per_sec) forget evicted messages. """ self.history_window = history_window self._history: deque[tuple[float, int]] = deque() # (timestamp, nbytes) @@ -124,11 +124,6 @@ def bytes_per_sec(self, window: float, now: float) -> float: msgs = self._in_window(window, now) return sum(n for _, n in msgs) / window if msgs else 0.0 - def avg_size(self, window: float, now: float) -> float: - """Mean payload size in bytes over [now - window, now]. 0.0 if none.""" - msgs = self._in_window(window, now) - return sum(n for _, n in msgs) / len(msgs) if msgs else 0.0 - @runtime_checkable class SpySource(Protocol): diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index 450cfc9e21..5ea4cdb3a4 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -71,7 +71,6 @@ def test_topic_stats_windowed_rates(): now = t0 + 9.0 assert s.freq(5.0, now) == pytest.approx(1.0, rel=0.25) assert s.bytes_per_sec(5.0, now) == pytest.approx(50.0, rel=0.25) - assert s.avg_size(5.0, now) == pytest.approx(50.0) def test_topic_stats_empty(): @@ -81,7 +80,6 @@ def test_topic_stats_empty(): assert s.last_seen is None assert s.freq(5.0, now=123.0) == 0.0 assert s.bytes_per_sec(5.0, now=123.0) == 0.0 - assert s.avg_size(5.0, now=123.0) == 0.0 def test_topic_stats_history_eviction_keeps_totals(): From f705bce6e810a7b36bdc406f7855d53eebb7fd34 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:44:53 +0300 Subject: [PATCH 27/33] fix(spy): read shared TopicStats fields under the lock, in one pass total_bytes/total_msgs/last_seen are written under the stats lock by transport threads but were read unlocked from the UI thread, and each refresh traversed+copied the history twice per row (freq + bytes_per_sec). window_stats() now returns a consistent WindowStats reading computed in a single locked pass without building intermediate lists; refresh_table sorts and renders from it. freq/bytes_per_sec stay as thin wrappers for the contract tests. --- dimos/utils/cli/spy/core.py | 41 +++++++++++++++++++++++++++------ dimos/utils/cli/spy/run_spy.py | 12 ++++++---- dimos/utils/cli/spy/test_spy.py | 13 +++++++++++ 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/dimos/utils/cli/spy/core.py b/dimos/utils/cli/spy/core.py index 408f65d4be..843cfb5032 100644 --- a/dimos/utils/cli/spy/core.py +++ b/dimos/utils/cli/spy/core.py @@ -70,6 +70,21 @@ def split_type_suffix(topic: str) -> tuple[str, str | None]: return (base, suffix) if sep else (topic, None) +@dataclass(frozen=True, slots=True) +class WindowStats: + """One consistent reading of a TopicStats: windowed rates + lifetime totals. + + Built in a single pass under the stats lock (see TopicStats.window_stats), + so a UI thread never sees values torn by a concurrent record(). + """ + + freq: float # messages per second over the window + bytes_per_sec: float # payload bytes per second over the window + total_bytes: int + total_msgs: int + last_seen: float | None + + class TopicStats: """Sliding-window traffic statistics for one spied topic. @@ -78,7 +93,8 @@ class TopicStats: values), so all stats are deterministic functions of recorded data. Thread-safety: record() may be called from transport threads while readers - query from a UI thread. + query from a UI thread; readers must go through window_stats() (or the + freq/bytes_per_sec wrappers), which take the lock. """ total_bytes: int @@ -109,20 +125,31 @@ def record(self, nbytes: int, timestamp: float) -> None: while self._history and self._history[0][0] < cutoff: self._history.popleft() - def _in_window(self, window: float, now: float) -> list[tuple[float, int]]: + def window_stats(self, window: float, now: float) -> WindowStats: + """Rates over [now - window, now] plus totals, in one locked pass.""" cutoff = now - window + count = 0 + nbytes = 0 with self._lock: - return [(ts, n) for ts, n in self._history if ts >= cutoff] + for ts, n in self._history: + if ts >= cutoff: + count += 1 + nbytes += n + return WindowStats( + freq=count / window if count else 0.0, + bytes_per_sec=nbytes / window if count else 0.0, + total_bytes=self.total_bytes, + total_msgs=self.total_msgs, + last_seen=self.last_seen, + ) def freq(self, window: float, now: float) -> float: """Messages per second over [now - window, now]. 0.0 if none.""" - msgs = self._in_window(window, now) - return len(msgs) / window if msgs else 0.0 + return self.window_stats(window, now).freq def bytes_per_sec(self, window: float, now: float) -> float: """Payload bytes per second over [now - window, now]. 0.0 if none.""" - msgs = self._in_window(window, now) - return sum(n for _, n in msgs) / window if msgs else 0.0 + return self.window_stats(window, now).bytes_per_sec @runtime_checkable diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 22bf3a4d34..e38644d05d 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -41,8 +41,8 @@ from dimos.utils.cli.spy.core import ( SOURCE_FACTORIES, SpyKey, - TopicStats, TransportSpy, + WindowStats, default_sources, split_type_suffix, validate_transport_names, @@ -164,15 +164,17 @@ def refresh_table(self) -> None: return now = time.time() snap = self.spy.snapshot() - rows: list[tuple[SpyKey, TopicStats]] = sorted( - snap.items(), key=lambda kv: kv[1].total_bytes, reverse=True + rows: list[tuple[SpyKey, WindowStats]] = sorted( + ((key, stats.window_stats(STAT_WINDOW, now)) for key, stats in snap.items()), + key=lambda kv: kv[1].total_bytes, + reverse=True, ) table.clear(columns=False) for key, stats in rows: base, msg_type = split_type_suffix(key.topic) - freq = stats.freq(STAT_WINDOW, now) - bps = stats.bytes_per_sec(STAT_WINDOW, now) + freq = stats.freq + bps = stats.bytes_per_sec age = now - stats.last_seen if stats.last_seen is not None else None stale = age is not None and age > STALE_AGE diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index 5ea4cdb3a4..d9514d49cc 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -73,6 +73,19 @@ def test_topic_stats_windowed_rates(): assert s.bytes_per_sec(5.0, now) == pytest.approx(50.0, rel=0.25) +def test_topic_stats_window_stats_single_reading(): + s = TopicStats(history_window=60.0) + t0 = 1000.0 + for i in range(10): # 1 Hz, 50 bytes each + s.record(50, t0 + i) + view = s.window_stats(5.0, now=t0 + 9.0) + assert view.freq == pytest.approx(1.0, rel=0.25) + assert view.bytes_per_sec == pytest.approx(50.0, rel=0.25) + assert view.total_bytes == 500 + assert view.total_msgs == 10 + assert view.last_seen == pytest.approx(t0 + 9.0) + + def test_topic_stats_empty(): s = TopicStats() assert s.total_msgs == 0 From 0329aafcadabb3d5235885ba65c0b1ccba5df8d2 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:45:10 +0300 Subject: [PATCH 28/33] chore(spy): drop stale TASK.md reference from test docstring The feature is implemented on this branch; TASK.md and the agent/spy-architect branch no longer exist. --- dimos/utils/cli/spy/test_spy.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index d9514d49cc..12af8778a0 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -12,9 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Contract tests for `dimos spy` (TASK.md on branch agent/spy-architect). +"""Contract tests for the spy core (`dimos spy`). -These encode the acceptance criteria and FAIL until the task is implemented: - TopicStats: deterministic windowed stats from injected timestamps. - subscribe_all: LCM delivers every message (non-conflating regex '.*'). - SpySources: count every message without ever decoding a payload. From 510a8ea33bb60054f2ac932c913566466520dab8 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:46:24 +0300 Subject: [PATCH 29/33] refactor(spy): tear down test resources via fixtures Buses, sources, publishers and the zenoh session pool were cleaned up with hand-rolled try/finally (or a contextmanager) in test bodies; move them to yield-fixtures / request.addfinalizer so teardown also runs when an assertion fails before the cleanup line. --- dimos/utils/cli/spy/test_spy.py | 185 ++++++++++++++++---------------- 1 file changed, 94 insertions(+), 91 deletions(-) diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index 12af8778a0..37f5d42917 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -22,7 +22,6 @@ """ from collections.abc import Callable -from contextlib import contextmanager import threading import time from typing import Any @@ -118,47 +117,43 @@ def test_split_type_suffix(): # subscribe_all contract: EVERY message, no conflation (spec-level fix) -@contextmanager -def lcm_base_ctx(): +@pytest.fixture +def lcm_bus(): bus = LCMPubSubBase() bus.start() - try: - yield bus, Topic("/spy_contract", Vector3) - finally: - bus.stop() + yield bus, Topic("/spy_contract", Vector3) + bus.stop() -@pytest.mark.parametrize("bus_ctx", [lcm_base_ctx], ids=["lcm"]) -def test_subscribe_all_delivers_every_message(bus_ctx): +def test_subscribe_all_delivers_every_message(lcm_bus): """A gated (slow) consumer must still receive every message eventually. LCM's subscribe_all is non-conflating (regex '.*', 10k queue), so all 50 messages are delivered even when the consumer lags the burst. """ n = 50 - with bus_ctx() as (bus, topic): - gate = threading.Event() - got = [] - done = threading.Event() + bus, topic = lcm_bus + gate = threading.Event() + got = [] + done = threading.Event() - def cb(msg, t): - gate.wait(15.0) # simulate a consumer slower than the burst - got.append((str(t), len(msg))) - if len(got) >= n: - done.set() + def cb(msg, t): + gate.wait(15.0) # simulate a consumer slower than the burst + got.append((str(t), len(msg))) + if len(got) >= n: + done.set() - unsub = bus.subscribe_all(cb) - time.sleep(0.5) # let the wildcard subscription establish + bus.subscribe_all(cb) + time.sleep(0.5) # let the wildcard subscription establish - for _ in range(n): - bus.publish(topic, VEC_BYTES) - gate.set() + for _ in range(n): + bus.publish(topic, VEC_BYTES) + gate.set() - done.wait(10.0) - ours = [g for g in got if g[0] == str(topic)] - assert len(ours) == n - assert all(size == len(VEC_BYTES) for _, size in ours) - unsub() + done.wait(10.0) + ours = [g for g in got if g[0] == str(topic)] + assert len(ours) == n + assert all(size == len(VEC_BYTES) for _, size in ours) # SpySources: every message counted, payloads never decoded @@ -189,76 +184,86 @@ def boom(*a, **k): monkeypatch.setattr(Vector3, "lcm_decode", staticmethod(boom)) -def test_lcm_source_counts_all_without_decoding(monkeypatch): - _assert_no_decode(monkeypatch) - topic = Topic("/spy_e2e", Vector3) - collector = _TapCollector(str(topic), 20) - +@pytest.fixture +def lcm_spy_source(): src = LCMSpySource() src.start() - untap = src.tap(collector) + yield src + src.stop() + + +@pytest.fixture +def lcm_pub(): pub = LCMPubSubBase() pub.start() - try: - time.sleep(0.3) - for _ in range(20): - pub.publish(topic, VEC_BYTES) - time.sleep(0.01) - assert collector.done.wait(10.0) - assert [n for _, n in collector.ours()] == [len(VEC_BYTES)] * 20 - finally: - untap() - src.stop() - pub.stop() - - -def test_lcm_source_counts_undecodable_garbage(monkeypatch): + yield pub + pub.stop() + + +def test_lcm_source_counts_all_without_decoding(monkeypatch, lcm_spy_source, lcm_pub): + _assert_no_decode(monkeypatch) + topic = Topic("/spy_e2e", Vector3) + collector = _TapCollector(str(topic), 20) + + lcm_spy_source.tap(collector) + time.sleep(0.3) + for _ in range(20): + lcm_pub.publish(topic, VEC_BYTES) + time.sleep(0.01) + assert collector.done.wait(10.0) + assert [n for _, n in collector.ours()] == [len(VEC_BYTES)] * 20 + + +def test_lcm_source_counts_undecodable_garbage(monkeypatch, lcm_spy_source, lcm_pub): """Payloads that would crash a decoder must still be counted (proves raw tap).""" _assert_no_decode(monkeypatch) topic = Topic("/spy_garbage", Vector3) collector = _TapCollector(str(topic), 5) - src = LCMSpySource() + lcm_spy_source.tap(collector) + time.sleep(0.3) + for _ in range(5): + lcm_pub.publish(topic, b"\x00garbage-not-lcm") + time.sleep(0.01) + assert collector.done.wait(10.0) + assert [n for _, n in collector.ours()] == [len(b"\x00garbage-not-lcm")] * 5 + + +@pytest.fixture +def zenoh_pool(): + pool = ZenohSessionPool() + yield pool + pool.close_all() + + +@pytest.fixture +def zenoh_spy_source(zenoh_pool): + src = ZenohSpySource(session_pool=zenoh_pool) src.start() - untap = src.tap(collector) - pub = LCMPubSubBase() + yield src + src.stop() + + +@pytest.fixture +def zenoh_pub(zenoh_pool): + pub = ZenohPubSubBase(session_pool=zenoh_pool) pub.start() - try: - time.sleep(0.3) - for _ in range(5): - pub.publish(topic, b"\x00garbage-not-lcm") - time.sleep(0.01) - assert collector.done.wait(10.0) - assert [n for _, n in collector.ours()] == [len(b"\x00garbage-not-lcm")] * 5 - finally: - untap() - src.stop() - pub.stop() - - -def test_zenoh_source_counts_all_without_decoding(monkeypatch): + yield pub + pub.stop() + + +def test_zenoh_source_counts_all_without_decoding(monkeypatch, zenoh_spy_source, zenoh_pub): _assert_no_decode(monkeypatch) topic = Topic("dimos/spy_e2e", Vector3) collector = _TapCollector(str(topic), 20) - pool = ZenohSessionPool() - src = ZenohSpySource(session_pool=pool) - src.start() - untap = src.tap(collector) - pub = ZenohPubSubBase(session_pool=pool) - pub.start() - try: - time.sleep(0.5) - for _ in range(20): - pub.publish(topic, VEC_BYTES) - time.sleep(0.01) - assert collector.done.wait(10.0) - assert [n for _, n in collector.ours()] == [len(VEC_BYTES)] * 20 - finally: - untap() - src.stop() - pub.stop() - pool.close_all() + zenoh_spy_source.tap(collector) + time.sleep(0.5) + for _ in range(20): + zenoh_pub.publish(topic, VEC_BYTES) + time.sleep(0.01) + assert collector.done.wait(10.0) + assert [n for _, n in collector.ours()] == [len(VEC_BYTES)] * 20 # TransportSpy: merging + totals + lifecycle (fake sources, deterministic) @@ -374,17 +379,15 @@ def start(self) -> None: raise RuntimeError("backend down") -def test_transport_spy_best_effort_skips_failed_source(spy_warnings): +def test_transport_spy_best_effort_skips_failed_source(request, spy_warnings): good, bad = FakeSource("lcm"), _FailingStartSource("zenoh") spy = TransportSpy(sources=[good, bad]) spy.start(best_effort=True) # degrade to the survivor instead of dying - try: - assert good.started and good.taps # survivor is live and tapped - assert not bad.started - good.emit("/t", 5) - assert spy.snapshot()[SpyKey("lcm", "/t")].total_bytes == 5 - finally: - spy.stop() + request.addfinalizer(spy.stop) + assert good.started and good.taps # survivor is live and tapped + assert not bad.started + good.emit("/t", 5) + assert spy.snapshot()[SpyKey("lcm", "/t")].total_bytes == 5 assert "zenoh" in spy_warnings.text # skipped source is warned about From fe5859e776fc884599d766b6957bb94ecdf526b6 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Tue, 7 Jul 2026 01:47:17 +0300 Subject: [PATCH 30/33] fix: dedupe sys import and type SpyApp's App generic in run_spy Two review nits on PR #2735: - import sys was a local import in both main() and lcm_main(); hoist it to the module imports. - SpyApp subclassed the generic App with a type: ignore[type-arg]; parameterize it as App[None] (the app's run() return type) instead of ignoring. --- dimos/utils/cli/spy/run_spy.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/dimos/utils/cli/spy/run_spy.py b/dimos/utils/cli/spy/run_spy.py index 4545f6ec35..122f0dc8ea 100644 --- a/dimos/utils/cli/spy/run_spy.py +++ b/dimos/utils/cli/spy/run_spy.py @@ -28,6 +28,7 @@ from __future__ import annotations +import sys import time from rich.text import Text @@ -104,7 +105,7 @@ def _parse_transports(argv: list[str]) -> list[str] | None: return transports or None -class SpyApp(App): # type: ignore[type-arg] +class SpyApp(App[None]): """A real-time dashboard for all-transport pubsub traffic using Textual.""" CSS_PATH = "../dimos.tcss" @@ -222,8 +223,6 @@ def refresh_table(self) -> None: def main() -> None: """Entry point for `dimos spy` (argv: --transport filters).""" - import sys - SpyApp(transports=_parse_transports(sys.argv[1:])).run() @@ -242,8 +241,6 @@ def _lcm_only_argv(args: list[str]) -> list[str]: def lcm_main() -> None: """`lcmspy` console-script shim: the spy over the LCM source only.""" - import sys - sys.argv = _lcm_only_argv(sys.argv[1:]) main() From 86c3f56880f477db684b589e39e64c3d8b59626d Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:47:50 +0300 Subject: [PATCH 31/33] refactor(spy): poll for subscription establishment instead of sleeping Replace the fixed establishment sleeps with a probe loop: publish on a probe topic until the tap/wildcard subscription delivers one, with a deadline. The 0.01s pacing sleeps in publish loops stay (throttling, not waiting). Cuts the spy test suite from ~3.2s to ~1.4s. --- dimos/utils/cli/spy/test_spy.py | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/dimos/utils/cli/spy/test_spy.py b/dimos/utils/cli/spy/test_spy.py index 37f5d42917..efe694398e 100644 --- a/dimos/utils/cli/spy/test_spy.py +++ b/dimos/utils/cli/spy/test_spy.py @@ -125,6 +125,15 @@ def lcm_bus(): bus.stop() +def _publish_probes_until(pub, probe_topic: Topic, established: Callable[[], bool]) -> None: + """Publish probes until the subscription delivers one, instead of a fixed sleep.""" + deadline = time.time() + 10.0 + while not established(): + assert time.time() < deadline, f"subscription never saw a probe on {probe_topic}" + pub.publish(probe_topic, VEC_BYTES) + time.sleep(0.01) + + def test_subscribe_all_delivers_every_message(lcm_bus): """A gated (slow) consumer must still receive every message eventually. @@ -133,18 +142,23 @@ def test_subscribe_all_delivers_every_message(lcm_bus): """ n = 50 bus, topic = lcm_bus + probe_topic = Topic("/spy_contract_probe", Vector3) + probe_seen = threading.Event() gate = threading.Event() got = [] done = threading.Event() def cb(msg, t): + if str(t) == str(probe_topic): + probe_seen.set() + return gate.wait(15.0) # simulate a consumer slower than the burst got.append((str(t), len(msg))) if len(got) >= n: done.set() bus.subscribe_all(cb) - time.sleep(0.5) # let the wildcard subscription establish + _publish_probes_until(bus, probe_topic, probe_seen.is_set) for _ in range(n): bus.publish(topic, VEC_BYTES) @@ -174,6 +188,9 @@ def __call__(self, topic: str, nbytes: int) -> None: def ours(self) -> list[tuple[str, int]]: return [e for e in self.events if e[0] == self.topic_str] + def saw(self, topic_str: str) -> bool: + return any(t == topic_str for t, _ in self.events) + def _assert_no_decode(monkeypatch): """Make any Vector3 decode explode — the spy must never trigger one.""" @@ -203,10 +220,11 @@ def lcm_pub(): def test_lcm_source_counts_all_without_decoding(monkeypatch, lcm_spy_source, lcm_pub): _assert_no_decode(monkeypatch) topic = Topic("/spy_e2e", Vector3) + probe = Topic("/spy_e2e_probe", Vector3) collector = _TapCollector(str(topic), 20) lcm_spy_source.tap(collector) - time.sleep(0.3) + _publish_probes_until(lcm_pub, probe, lambda: collector.saw(str(probe))) for _ in range(20): lcm_pub.publish(topic, VEC_BYTES) time.sleep(0.01) @@ -218,10 +236,11 @@ def test_lcm_source_counts_undecodable_garbage(monkeypatch, lcm_spy_source, lcm_ """Payloads that would crash a decoder must still be counted (proves raw tap).""" _assert_no_decode(monkeypatch) topic = Topic("/spy_garbage", Vector3) + probe = Topic("/spy_garbage_probe", Vector3) collector = _TapCollector(str(topic), 5) lcm_spy_source.tap(collector) - time.sleep(0.3) + _publish_probes_until(lcm_pub, probe, lambda: collector.saw(str(probe))) for _ in range(5): lcm_pub.publish(topic, b"\x00garbage-not-lcm") time.sleep(0.01) @@ -255,10 +274,11 @@ def zenoh_pub(zenoh_pool): def test_zenoh_source_counts_all_without_decoding(monkeypatch, zenoh_spy_source, zenoh_pub): _assert_no_decode(monkeypatch) topic = Topic("dimos/spy_e2e", Vector3) + probe = Topic("dimos/spy_e2e_probe", Vector3) collector = _TapCollector(str(topic), 20) zenoh_spy_source.tap(collector) - time.sleep(0.5) + _publish_probes_until(zenoh_pub, probe, lambda: collector.saw(str(probe))) for _ in range(20): zenoh_pub.publish(topic, VEC_BYTES) time.sleep(0.01) From bab0fe4c3ce6e3b6c2c082a03bff1adb1271c6d9 Mon Sep 17 00:00:00 2001 From: "spy-worker (osvetnik)" Date: Tue, 7 Jul 2026 01:54:07 +0300 Subject: [PATCH 32/33] fix: reject root-level --transport before the spy subcommand Greptile P1 on PR #2735: 'dimos --transport zenoh spy' let the root callback consume --transport into GlobalConfig, so the spy saw no args and silently ran the default all-transport view (including LCM) despite the zenoh request. Root --transport sets the stack's pubsub backend (which single transport dimos processes participate on); the spy is an observer that watches every transport and takes its own repeatable --transport filter after the subcommand. They mean different things, so reject the root placement with a clear error pointing at 'dimos spy --transport ' rather than silently ignoring the filter. Env/ default DIMOS_TRANSPORT is unaffected (only an explicit root CLI flag triggers it). --- dimos/robot/cli/dimos.py | 12 ++++++++++++ dimos/robot/cli/test_dimos.py | 15 +++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/dimos/robot/cli/dimos.py b/dimos/robot/cli/dimos.py index 2a95951622..4aa44c0a92 100644 --- a/dimos/robot/cli/dimos.py +++ b/dimos/robot/cli/dimos.py @@ -623,6 +623,18 @@ def list_blueprints() -> None: @main.command(context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def spy(ctx: typer.Context) -> None: """Universal transport spy: topics, rates, sizes across all pubsub transports.""" + # A root-level `--transport` (before the subcommand) sets the stack's pubsub + # backend — which single transport dimos processes participate on. The spy is an + # observer: it watches every transport and takes its own repeatable `--transport` + # filter *after* the subcommand. The two look alike but mean different things, so + # reject the root placement rather than silently ignoring the requested filter. + if (ctx.obj or {}).get("transport") is not None: + typer.echo( + "Error: `--transport` before `spy` sets the stack backend, which the spy " + "ignores. Put the filter after the subcommand: `dimos spy --transport `.", + err=True, + ) + raise typer.Exit(2) from dimos.utils.cli.spy.run_spy import main as spy_main sys.argv = ["spy", *ctx.args] diff --git a/dimos/robot/cli/test_dimos.py b/dimos/robot/cli/test_dimos.py index 0c0f1ba33c..cf61cec7b8 100644 --- a/dimos/robot/cli/test_dimos.py +++ b/dimos/robot/cli/test_dimos.py @@ -200,3 +200,18 @@ def test_lcmspy_rejects_stray_positional(monkeypatch): result = CliRunner().invoke(main, ["lcmspy", "foo"]) assert result.exit_code == 1 assert "unexpected" in result.output.lower() + + +def test_spy_rejects_root_transport(monkeypatch, spy_main_argv): + # A root-level `--transport` (before the subcommand) sets the stack backend, + # which the spy ignores. Rather than silently show all transports, error and + # point at the subcommand-level filter. + monkeypatch.setattr(sys, "argv", ["dimos"]) + original = global_config.transport + try: + result = CliRunner().invoke(main, ["--transport", "zenoh", "spy"]) + finally: + global_config.update(transport=original) + assert result.exit_code == 2 + assert "dimos spy --transport" in result.output + assert spy_main_argv == [] # never reaches the spy From 52e6de09739a6ce3fd7b8bbd038a83d3beff7c96 Mon Sep 17 00:00:00 2001 From: Paul Nechifor Date: Tue, 7 Jul 2026 01:56:22 +0300 Subject: [PATCH 33/33] add fix --- dimos/codebase_checks/test_get_logger.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dimos/codebase_checks/test_get_logger.py b/dimos/codebase_checks/test_get_logger.py index 26db11ab8f..f2308cf9c0 100644 --- a/dimos/codebase_checks/test_get_logger.py +++ b/dimos/codebase_checks/test_get_logger.py @@ -48,6 +48,10 @@ "dimos/visualization/rerun/websocket_server.py", 'ws_logger = logging.getLogger("websockets.server")', ), + ( + "dimos/utils/cli/spy/conftest.py", + "lg = logging.getLogger(_CORE_LOGGER)", + ), ]