feat(obd_ai): add policy guardrails and audit logging - #15
virtuscyber wants to merge 1 commit into
Conversation
🤖 Augment PR SummarySummary: This PR introduces a policy guardrail layer and structured audit logging around the LLM-facing OBD tool surface to prevent unsafe or destructive operations. Changes:
Technical Notes: The tool surface now returns structured error payloads for policy blocks/approval requirements and logs both the decision and the final tool outcome via an injectable audit sink. 🤖 Was this summary useful? React with 👍 or 👎 |
| ) -> Optional[PolicyDecision]: | ||
| request_payload = dict(payload or {}) | ||
|
|
||
| if request_payload.get("force"): |
There was a problem hiding this comment.
obd_ai/policy.py:133 — request_payload.get("force") treats any truthy value as a bypass attempt, so a caller passing something like "false" (string) could be denied even though it’s semantically false. Consider requiring a real boolean True (and/or rejecting non-bools) to prevent accidental policy blocks.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| mutable_by_key[action.key] = action | ||
| ordered.append(action) | ||
|
|
||
| if action.obd_command_name is not None: |
There was a problem hiding this comment.
obd_ai/policy.py:76-78 — _by_obd_command_name silently overwrites entries when multiple actions share the same obd_command_name, which can make find_action() resolve to the wrong policy. It may be safer to detect duplicates and raise so the catalog stays deterministic.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| self._logger = logger or logging.getLogger("obd_ai.audit") | ||
|
|
||
| def record(self, event: AuditEvent) -> None: | ||
| self._logger.info("obd_ai_audit %s", json.dumps(event.to_dict(), sort_keys=True)) |
There was a problem hiding this comment.
obd_ai/audit.py:57 — json.dumps(event.to_dict()) can raise TypeError if request/policy contains non-JSON types (e.g., programmatic callers could pass a Python bytes value for the bytes field), which would make tool calls fail while logging. Consider making logging resilient (e.g., default=str or pre-sanitizing) so auditing never becomes a new failure mode.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| request=request_payload, | ||
| decision="success" if result_payload.get("ok") else "error", | ||
| result="success" if result_payload.get("ok") else "error", | ||
| error_code=result_payload.get("error", {}).get("code"), |
There was a problem hiding this comment.
obd_ai/tools.py:1053 — tool_invocation.error_code is pulled only from top-level payload["error"], but tools like read_sensors can return ok=False with only per-item errors and no top-level error, leaving audit events with decision=error but error_code=None. Consider including an aggregated/representative error code (or counts by code) in the audit metadata so audits can be filtered reliably.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Refs #6
Stacked on top of #5.
Includes: