Read snapshots from where the gateway puts them

[mfaferek93]

Summary

The MCP server has never shown a snapshot for any fault, and its rosbag download tool has always answered "no rosbag snapshots found". Both read extended_data_records.rosbagSnapshots / freezeFrameSnapshots - containers the gateway does not populate and, as far as I can tell from a live response, never has. Snapshots arrive in environment_data.snapshots, discriminated by type.

Found while auditing consumers for ros2_medkit#620, which lets one fault keep a recording per occurrence instead of overwriting. It is not caused by that change; it is what that audit walked into.

Why it survived

The tests around this code built their fixtures from the same fiction, so the suite was green the whole time. That is the part worth fixing properly, not just the two call sites.

tests/fixtures/fault_detail_two_recordings.json is now a verbatim response from a running gateway holding a fault that came back twice, and tests/test_fault_snapshot_contract.py asserts against it - including, explicitly, that extended_data_records carries only first_occurrence / last_occurrence and no snapshot containers at all. Reverting the source changes turns 3 of its 5 red.

What changed

• EnvironmentData gains the snapshots list it was missing; GatewaySnapshot models an entry as sent, with captured_at read from the x-medkit extension where the gateway nests it rather than from a top-level field that does not exist.
• The fault formatter reads that list and splits by type. It reports Rosbag Recordings (N), plural: since #620 a fault keeps one black box per occurrence and each has its own bulk_data_uri, which is what an LLM needs to fetch a specific one.
• download_rosbags_for_fault reads the same list, so it now downloads every recording a fault kept. Each is identified by its name - the recording id - because there is no snapshotId on the wire and falling back to the fault code would give every recording of one fault the same identity in the report.
• Freeze frames were lost to the identical mistake and come back with it.

Testing

174 passed, ruff and mypy clean. Seven existing tests encoding the old shape were rewritten against the real one rather than adjusted to keep passing.

[Copilot]

Pull request overview

Fixes snapshot handling in the MCP server to match the gateway’s actual on-the-wire fault payload shape: snapshots are read from environment_data.snapshots (typed entries) rather than from extended_data_records.*Snapshots, enabling both fault formatting and rosbag downloads to surface real freeze frames and per-occurrence recordings.

Changes:

• Add GatewaySnapshot and EnvironmentData.snapshots (including captured_at extraction from the x-medkit extension).
• Update fault environment-data formatting to split snapshots by type and report “Rosbag Recordings (N)” (plural, per occurrence).
• Update download_rosbags_for_fault to download rosbag entries from environment_data.snapshots, identified by name, and refresh tests/fixtures to reflect a real gateway response.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/ros2_medkit_mcp/models.py	Introduces GatewaySnapshot and adds EnvironmentData.snapshots to model the gateway’s real snapshot contract.
src/ros2_medkit_mcp/mcp_app.py	Switches formatting and rosbag download logic to consume environment_data.snapshots and format per-type output.
tests/test_fault_snapshot_contract.py	Adds a contract test suite pinned to a verbatim gateway fault payload to prevent shape regressions.
tests/test_fault_formatting.py	Updates formatter tests to assert against the real snapshot shape and per-occurrence rosbag reporting.
tests/test_bulkdata_tools.py	Updates rosbag-download tests to use environment_data.snapshots and validate multi-recording downloads.
tests/fixtures/fault_detail_two_recordings.json	Adds a real gateway fixture demonstrating one fault with multiple rosbag recordings plus a freeze frame.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/ros2_medkit_mcp/mcp_app.py:271

• In format_gateway_snapshot, rosbag entries always render a Download URI line even when bulk_data_uri is missing, producing Download URI: None and making the output misleading. Also, if snapshot.size_bytes: skips valid 0 values; prefer an explicit is not None check.

    if snapshot.type == "rosbag":
        # The URI carries the recording id, which is what distinguishes one
        # occurrence's black box from another's.
        lines.append(f"    Download URI: {snapshot.bulk_data_uri}")
        if snapshot.size_bytes:

src/ros2_medkit_mcp/mcp_app.py:278

• format_gateway_snapshot renders freeze-frame data via Python repr, which yields single-quoted dicts/lists and is inconsistent with format_snapshot’s JSON formatting. Using json.dumps(..., default=str) keeps the output valid JSON and easier for LLMs/tools to parse.

    elif snapshot.data is not None:
        lines.append(f"    Data: {snapshot.data}")

src/ros2_medkit_mcp/models.py:649

• EnvironmentData.extended_data_records is still typed as ExtendedDataRecords (which models snapshot containers), but its field description was changed to “First and last occurrence timestamps”. As-is, this is misleading for API consumers reading the model/schema.

    extended_data_records: ExtendedDataRecords | None = Field(
        default=None,
        alias="extendedDataRecords",
        description="First and last occurrence timestamps",
    )

src/ros2_medkit_mcp/mcp_app.py:570

• Terminology is inconsistent: this branch talks about “rosbag recordings”, but a few lines below the fallback still says “No rosbag snapshots found…”. Updating that message keeps user-facing output aligned with the new environment_data.snapshots / recordings model.

        freeze_frames = [s for s in snapshots if s.get("type") == "freeze_frame"]
        if freeze_frames:
            return [
                TextContent(

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/ros2_medkit_mcp/models.py:649

• EnvironmentData.extended_data_records is now described as carrying first/last occurrence timestamps, but ExtendedDataRecords doesn’t model those fields (they’d only be accepted via extra="allow" and then be inaccessible as attributes). This makes the typed API misleading and risks silently dropping occurrence data from the model.

    extended_data_records: ExtendedDataRecords | None = Field(
        default=None,
        alias="extendedDataRecords",
        description="First and last occurrence timestamps",
    )

tests/test_fault_formatting.py:417

• This file now pins that environment_data.extended_data_records does not contain snapshot lists, but earlier in the same test module TestEnvironmentData.test_environment_from_api still asserts that EnvironmentData.model_validate() can receive extendedDataRecords.freezeFrameSnapshots/rosbagSnapshots. That keeps coverage for the fictional shape this PR is trying to prevent and makes it easier for regressions to slip back in.

    def test_extended_data_records_alone_yields_no_snapshots(self) -> None:
        """The container the gateway populates carries occurrences, not snapshots.

        Reading snapshots out of extended_data_records is what made this
        formatter render nothing at all for every fault; pin that it is not
        where they come from.
        """
        env = EnvironmentData.model_validate(

src/ros2_medkit_mcp/mcp_app.py:600

• Now that rosbag snapshots are identified by name and may carry a format field, the fallback filename generation should use that format when the gateway doesn’t return a filename; otherwise sqlite3/db3 recordings will still be saved with a misleading .mcap extension in the not filename branch below.

        # The recording's own name. There is no snapshotId on the wire, and a
        # fault can hold several recordings, so falling back to the fault code
        # would give every one of them the same identity in the report below.
        snap_id = snap.get("name") or "unknown"
        bulk_uri = snap.get("bulkDataUri") or snap.get("bulk_data_uri")

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/ros2_medkit_mcp/mcp_app.py:666

• The ros2_medkit_fault_snapshots / ros2_medkit_system_fault_snapshots tools (and their sovd_* aliases) appear to have been removed from the public tool surface (TOOL_ALIASES + tool registration/dispatch). That’s a breaking change for existing MCP clients, even if the tools were previously incorrect. Consider keeping them as deprecated wrappers that call get_fault (or faults_get) and return the snapshot section from environment_data, so existing clients keep working while still reading snapshots from environment_data.snapshots.

    "ros2_medkit_delete_all_configurations": "ros2_medkit_delete_all_configurations",
    "ros2_medkit_all_faults_list": "ros2_medkit_all_faults_list",
    "ros2_medkit_clear_all_faults": "ros2_medkit_clear_all_faults",
    "ros2_medkit_data_categories": "ros2_medkit_data_categories",
    "ros2_medkit_data_groups": "ros2_medkit_data_groups",