Repository navigation
feat(obd_ai): add read-only diagnostic tool surface - #12
virtuscyber wants to merge 1 commit into
Conversation
🤖 Augment PR SummarySummary: This PR introduces a constrained, read-only diagnostic tool interface intended for LLM/MCP consumers to safely connect to an OBD adapter and run approved queries. Changes:
Technical Notes: All reads are gated through the approved catalog and 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| ) | ||
|
|
||
| self.close() | ||
| self._session = self._session_manager.open_session(**payload) |
There was a problem hiding this comment.
obd_ai/tools.py:56: OBDAISessionManager.open_session(**payload) can raise (e.g., serial/permission/port errors) and currently bubbles out of connect_vehicle, breaking the structured {ok: False, error: ...} contract expected by LLM/MCP callers. Consider whether connection failures should be translated into a tool error response instead of raising.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| self._session = self._session_manager.open_session(**payload) | ||
|
|
||
| metadata = self._session.connection_metadata() | ||
| if not metadata["is_connected"]: |
There was a problem hiding this comment.
obd_ai/tools.py:59-65: When metadata["is_connected"] is false, connect_vehicle returns an error but leaves _session set (and the underlying connection potentially open). If callers treat this as a failed connect and don’t call close(), this can keep resources open unexpectedly.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| message="Timed out waiting for a response from the adapter.", | ||
| details={"command_key": command_key}, | ||
| ) | ||
| raise |
There was a problem hiding this comment.
obd_ai/tools.py:323-331: _query_approved_command re-raises any non-timeout exception from session.query(), which can crash the tool surface instead of returning a structured error payload. This seems inconsistent with the rest of the surface returning {ok: False, error: ...} for adapter/data issues.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| """List approved commands, optionally including adapter support status.""" | ||
|
|
||
| payload = dict(input or {}) | ||
| include_support = bool(payload.get("include_support", True)) |
There was a problem hiding this comment.
obd_ai/tools.py:89-90: bool(payload.get("include_support")) (and only_supported) will treat non-empty strings like "false" as True, which can surprise tool callers and change behavior unexpectedly. If these inputs can come from loosely-typed sources, stricter type validation may be needed to preserve intent.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
1 participant
Refs #3
Stacked on top of #2.
Includes: