fix: post-v0.5.0 external audit — Lots 0–5 (DDS correctness, memory bounds, tests, docs) - #4
Merged
Merged
Conversation
added 6 commits
July 8, 2026 20:03
…+ tests
Highest-value item from the 2026-07-08 audit: the two real DDS adapters
(cyclone ~743 LOC, fast ~712 LOC) import their vendor binding at module
top level, so they — and their pure QoS-normalization logic that feeds the
flagship detect_qos_mismatches diagnostic — were never exercised by the
suite. A renamed policy key could silently make every QoS profile resolve
to None (no mismatch ever reported) with tests still green.
- Move _cyclone/_fast_qos_to_profile and _extract_guid/_vendor_id/
_hostname/_topic_name/_is_removal into binding-free
adapters/common/{qos_normalize,dds_introspection}.py. Adapters import
them back under their original private names — call sites unchanged
(behavior-preserving; verified via ruff since the adapters aren't
importable without their SDKs). fast_qos_to_profile takes the binding's
int->str enum maps as params to stay import-free.
- Add tests/test_dds_qos_normalization.py + tests/test_dds_introspection.py
(synthetic duck-typed objects, no SDK), incl. regression guards for the
renamed-key silent-None failure. Extracted modules now ~91-92% covered.
- Add pytest-cov + rosbags to [dev]; [tool.coverage] with fail_under=85
(binding-only adapter shells omitted). rosbags un-skips the real .db3
bag I/O test — which exposed stale rosbags-API usage in
test_bag_service.py (Writer version kwarg + typestore key format); fixed.
Baseline: 399 -> 457 passed, 24 -> 23 skipped, ruff clean, coverage 88.75%.
Claude-Session: https://claude.ai/code/session_01NLL19vZcEYRwru3AV3gtaU
… RxO Three functional bugs in shipped DDS diagnostics, all in pure logic now testable after Lot 0. Red-then-green regression tests added. - topic_metrics frequency (audit C5): was count / (now - oldest), folding in idle time since the last peek, over N intervals not N-1. Now (N-1) / (newest - oldest) over the samples' own arrival span. Samples from one opportunistic peek share a single receive_ns (span 0) → yields frequency None instead of a fabricated rate. - topic_metrics sequence gaps (audit C6): merging independent writers' counters on one topic read each offset as a huge phantom gap, and a 16-bit wrap / publisher restart counted as tens of thousands of losses. Now grouped per writer (new best-effort MetricsSample.writer_guid) and a jump wider than 10_000 is treated as a reset/wrap discontinuity, not losses. - QoS Deadline (audit C3/P1-3): detect_qos_mismatches now flags a reader with a finite Deadline against a writer offering none (absent = infinite = loosest period, cannot satisfy a finite request). The old rule required both sides non-null and missed this incompatibility class. Baseline: 457 -> 464 passed, 23 skipped, ruff clean, coverage 88.83%. Claude-Session: https://claude.ai/code/session_01NLL19vZcEYRwru3AV3gtaU
Two unbounded in-memory maps flagged by the audit (memory growth on long-running servers). Pure logic; eviction tests added. P1-5 (take_iter -> read_iter, anti-flapping) is deferred to the integration-rig batch — it touches the untested Cyclone adapter and its read-vs-take semantics can't be verified without a real bus. - LifecycleBuffer._participants (audit P1-4/M1/P1): only the event ring was capped; the participant dict grew one entry per GUID ever seen and list_participants returned every tombstone forever. Now capped at MAX_PARTICIPANTS=4096, evicting "left" tombstones first, then oldest inserted. The docstring's "Bounded" claim is now accurate. - MetricsBuffer._samples (audit P2-5): per-topic rings were bounded but the topic-key count was not. Now capped at MAX_TOPICS=4096, oldest-inserted topic evicted on overflow. Baseline: 464 -> 470 passed, 23 skipped, ruff clean, coverage 88.90%. Claude-Session: https://claude.ai/code/session_01NLL19vZcEYRwru3AV3gtaU
Grab-bag of low-risk audit minors, all in testable code. - OpenDDS stub is_available() -> False (audit S1): a stub advertising availability (when a pyopendds module is importable) could be auto-selected by the factory, after which every tool call raised. Now matches the Dust stub. - iter_field_names string __slots__ (audit C2): __slots__ = "value" was exploded into ['v','a','l','u','e']; a bare-string slot is now one field. - decode_field_value depth cap at 32 (audit M6): pathologically deep graphs collapse to repr() instead of risking RecursionError. - _encode_raw_bytes slices bytes before hex (audit M5): no longer allocates a full 2x hex string just to truncate to the 4096-char preview. - Tests: e2e AdapterError -> ToolError (thin-handler contract, CLAUDE.md §8); vendor-tag <-> ParticipantInfo/Event Literal consistency pin (audit P2-3); _KNOWN_TOOLS gains peek_bag_samples (11th tool). - Removed dead annotate_full/annotate_partial imports + _ = (...) suppressor from services/bag_service.py. Baseline: 470 -> 476 passed, 23 skipped, ruff clean, coverage 88.90%. Claude-Session: https://claude.ai/code/session_01NLL19vZcEYRwru3AV3gtaU
… honesty
- product-plan.md realigned on the shipped 11-tool surface (audit M6/P1-6).
§1/§4 still said "five typed tools today" / DDS "roadmapped" while six
DDS/observability tools had shipped across v0.2.0-v0.4.0. §11's risk
register carried a self-imposed governance gate ("any 9th tool needs a
re-scope discussion documented in this register before code lands") that was
crossed in v0.4.0 without being recorded. Added a retroactive re-scope
decision closing the gap: the 3 ceiling-breaking tools are accepted, new
ceiling is 11, a 12th needs a documented re-scope.
- README + peek_dds_samples tool description no longer over-promise the raw
fallback (audit C1): on the current user-topic raw path _raw_bytes_hex is
empty ("present but not decoded"); capturing on-wire CDR bytes is stated as
roadmapped, not done.
- CLAUDE.md left untouched (gitignored/local; §2 "MVP locked" may be
intentionally frozen) — flagged for the maintainer.
Baseline unchanged: 476 passed, 23 skipped, ruff clean, coverage 88.90%.
Claude-Session: https://claude.ai/code/session_01NLL19vZcEYRwru3AV3gtaU
…date on rig Dedup between the two DDS adapters. The tested part is done; the adapter wiring + read_iter switch are ruff/py_compile-validated only and MUST be confirmed on the real-bus integration rig before release (cyclonedds/fastdds absent here, so the adapters are not importable/unit-testable). Done & unit-tested (binding-free): - Extract the identical detect_qos_mismatches endpoint-pairing logic into common/qos_endpoints.detect_mismatches_across_endpoints (~40 dup lines per adapter removed). Fixes the O(readers*writers) QoS re-parse: each writer's profile is computed once per topic, not once per reader (audit D1/M7/P2). New tests/test_qos_endpoints.py drives it with synthetic endpoints + once through the real cyclone helpers. - Shared validate_domain_id in common/dds_helpers, used by all four DDS adapter constructors (the OpenDDS/Dust wiring IS test-covered). Prepared, needs rig validation: - Both adapters now delegate detect_qos_mismatches to the shared function. - Cyclone discovery/sample reads switched take_iter -> read_iter (non- destructive; fixes participant flapping, audit A1/P1-5). read-vs-take semantics need a real-bus run on scripts/integration/. Not done: full _DdsObservabilityBase base class (low-value remaining dup on untested code) — the high-value pairing dedup is captured. Baseline: 476 -> 485 passed, 23 skipped, ruff clean, coverage 89.12%. Claude-Session: https://claude.ai/code/session_01NLL19vZcEYRwru3AV3gtaU
There was a problem hiding this comment.
Pull request overview
This PR implements a multi-lot post-v0.5.0 audit remediation: it extracts DDS “pure logic” into binding-free modules for unit testing, fixes correctness issues in QoS/metrics analysis, adds memory bounds to long-lived buffers, hardens decoding behavior, reconciles docs with shipped behavior, and raises overall test/coverage rigor.
Changes:
- Extract DDS QoS normalization, endpoint introspection, and QoS endpoint pairing into
adapters/common/with new unit tests and coverage gating. - Fix observed-frequency math, deadline RxO compatibility, and sequence-gap counting (per-writer + wrap/reset guard), and bound participant/topic maps to prevent unbounded growth.
- Update docs/changelog/readme and tests to reflect the 11-tool surface and current runtime behaviors/APIs (e.g., rosbags Writer API, user-topic raw decode caveat).
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tools_integration.py | Adds integration-style assertions for ToolError propagation and successful tool calls. |
| tests/test_qos_endpoints.py | New unit tests for extracted endpoint-pairing QoS mismatch detector. |
| tests/test_qos_analyzer.py | Adds regression tests for Deadline “absent = infinite” compatibility semantics. |
| tests/test_opendds_adapter.py | Pins OpenDDS stub availability behavior. |
| tests/test_metrics_buffer.py | Adds regression tests for frequency math, per-writer gaps, wrap/reset handling, and bounded topic map eviction. |
| tests/test_lifecycle_buffer.py | Adds tests for bounded participant map and eviction preference rules. |
| tests/test_dds_qos_normalization.py | New binding-free tests for Cyclone/Fast QoS normalization helpers. |
| tests/test_dds_introspection.py | New binding-free tests for Cyclone/Fast discovery sample field extractors. |
| tests/test_dds_helpers.py | Tests for shared domain-id validation and vendor-tag/Literal coherence. |
| tests/test_cdr_decoder.py | Adds tests for __slots__-as-string handling and recursion depth cap. |
| tests/test_bag_service.py | Updates bag-writing test helper for newer rosbags Writer API and typestore usage. |
| tests/integration/test_scenarios_schema.py | Updates known-tool allowlist to include peek_bag_samples. |
| src/topicforge/tools/handlers.py | Updates tool description text to accurately describe user-topic raw decode behavior. |
| src/topicforge/services/bag_service.py | Removes unused imports / dead code related to annotate_full/partial. |
| src/topicforge/adapters/dds_opendds/adapter.py | Makes OpenDDS stub always unavailable; centralizes domain-id validation. |
| src/topicforge/adapters/dds_fast/adapter.py | Delegates QoS mismatch pairing to common module; uses shared introspection/normalization helpers and domain-id validation. |
| src/topicforge/adapters/dds_dust/adapter.py | Uses shared validate_domain_id in constructor. |
| src/topicforge/adapters/dds_cyclone/adapter.py | Switches discovery reads to read_iter; delegates QoS mismatch pairing to common module; uses shared helpers and domain-id validation. |
| src/topicforge/adapters/common/xtypes.py | Optimizes raw-bytes preview by slicing bytes before hex-encoding. |
| src/topicforge/adapters/common/qos_normalize.py | New binding-free QoS normalizers for Cyclone/Fast. |
| src/topicforge/adapters/common/qos_endpoints.py | New binding-free endpoint pairing + mismatch reporting helper. |
| src/topicforge/adapters/common/qos_analyzer.py | Fixes Deadline RxO logic by treating absent deadline as infinite. |
| src/topicforge/adapters/common/metrics_buffer.py | Fixes frequency calculation; counts gaps per writer; adds topic-map cap and reset/wrap guard. |
| src/topicforge/adapters/common/lifecycle.py | Adds participant-map cap with eviction preference (tombstones first). |
| src/topicforge/adapters/common/dds_introspection.py | New binding-free defensive extractors for Cyclone/Fast discovery samples. |
| src/topicforge/adapters/common/dds_helpers.py | Adds shared validate_domain_id helper. |
| src/topicforge/adapters/common/cdr_decoder.py | Fixes __slots__ string handling; adds recursion depth cap for decoding. |
| src/topicforge/adapters/common/init.py | Re-exports newly extracted common helpers. |
| README.md | Documents the user-topic raw decode caveat (_raw_bytes_hex empty on raw path). |
| pyproject.toml | Adds pytest-cov + rosbags dev deps and coverage configuration with fail-under threshold. |
| docs/projet-file/action-plan-audit-2026-07-08.md | Adds audit action plan document capturing findings and lot structure. |
| docs/product-plan.md | Reconciles plan with shipped 11-tool surface and documents governance re-scope. |
| CHANGELOG.md | Detailed unreleased notes for audit lots, fixes, coverage, and adapter deduplication. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+65
to
+71
| def __init__( | ||
| self, *, max_events: int = MAX_EVENTS, max_participants: int = MAX_PARTICIPANTS | ||
| ) -> None: | ||
| self._lock = threading.RLock() | ||
| self._participants: dict[str, ParticipantInfo] = {} | ||
| self._events: deque[ParticipantEvent] = deque(maxlen=max_events) | ||
| self._max_participants = max_participants |
Comment on lines
+250
to
+256
| for guid, info in self._participants.items(): | ||
| if info.status == "left": | ||
| del self._participants[guid] | ||
| return | ||
| oldest = next(iter(self._participants), None) | ||
| if oldest is not None: | ||
| del self._participants[oldest] |
Comment on lines
+75
to
84
| def __init__( | ||
| self, | ||
| *, | ||
| max_samples_per_topic: int = MAX_SAMPLES_PER_TOPIC, | ||
| max_topics: int = MAX_TOPICS, | ||
| ) -> None: | ||
| self._lock = threading.RLock() | ||
| self._cap = max_samples_per_topic | ||
| self._max_topics = max_topics | ||
| self._samples: dict[str, deque[MetricsSample]] = {} |
Comment on lines
+17
to
+34
| from collections.abc import Callable | ||
| from typing import Any | ||
|
|
||
| from topicforge.adapters.common.dds_helpers import format_guid | ||
| from topicforge.adapters.common.qos_analyzer import detect_mismatches | ||
| from topicforge.models import MismatchReport, QosProfile | ||
|
|
||
|
|
||
| def detect_mismatches_across_endpoints( | ||
| *, | ||
| subs: list[Any], | ||
| pubs: list[Any], | ||
| topic: str | None, | ||
| qos_to_profile: Callable[[Any], QosProfile | None], | ||
| extract_topic_name: Callable[[Any], str | None], | ||
| extract_guid: Callable[[Any], bytes | None], | ||
| mode_effective: str = "live", | ||
| ) -> list[MismatchReport]: |
| writer_guid=format_guid(extract_guid(writer_sample)), | ||
| incompatible_policies=policies, | ||
| severity=severity, | ||
| mode_effective=mode_effective, # type: ignore[arg-type] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the 2026-07-08 external audit. Six cohesive lots, each committed
atomically and green. Baseline 399 → 485 passed, 23 skipped, ruff clean,
coverage tooling added (89.12%,
fail_under=85).Lots
bcbbeff) — Extract the DDS QoS-normalization + sampleintrospection out of the binding-gated adapters into binding-free
adapters/common/, so the flagshipdetect_qos_mismatchesfeeder is finallyunit-tested. Add
pytest-cov+rosbagsto dev; the latter un-skipped abag-I/O test that had gone silently stale (fixed).
a1abbb7) —topic_metricscorrectness: frequency(N-1)/span(snapshot →
None), per-writer sequence-gap counting with wrap/reset guard,and the QoS Deadline RxO false negative (absent = infinite).
0f9059b) — BoundLifecycleBuffer(MAX_PARTICIPANTS) andMetricsBuffer(MAX_TOPICS) so a long-running server on a churny buscannot grow memory without bound.
c386048) — OpenDDS stubis_available()→ False, string__slots__decode, recursion depth cap, slice-before-hex, e2eAdapterError→isError test, vendor-Literal consistency pin, dead-code removal.
a60c1ae) — Reconciledocs/product-plan.mdon the shipped11-tool surface and close the crossed-but-undocumented governance gate;
user-topic
rawdecode honesty in README + tool description.8869cb7) — Dedup the QoS endpoint-pairing intocommon/qos_endpoints(+ O(R·W) → O(R+W) profile parse), sharedvalidate_domain_id, and Cyclonetake_iter → read_iter.Lot 5 touches the Cyclone/Fast adapters, which are not importable without
cyclonedds/fastdds— theirdetect_qos_mismatchesdelegation and thetake_iter → read_iterswitch are validated only byruff+py_compile.Run
scripts/integration/(real-bus rig) to validate them before tagging arelease. All other lots are fully unit-verified. Details in
docs/projet-file/action-plan-audit-2026-07-08.md.https://claude.ai/code/session_01NLL19vZcEYRwru3AV3gtaU