fix(telegram-bot): correct risk_level field + show description in alerts
- read risk_level with risk fallback (was: risk only → "unknown" for all actions written by supervisor which uses risk_level key) - include description field in alert format (was: alert_only payloads' substance was invisible — description carried the full message) - extract _format_pending_action() pure helper to enable unit testing without a live Telegram connection - 8 tests: risk_level present, risk fallback, both absent, description shown/absent, truncation, full HA alert_only shape, no-description no-crash - flagged during Phase 5 review of ha-diag-agent supervisor routing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
bf1415e4c1
commit
b9ed118b8c
|
|
@ -54,6 +54,36 @@ async def post_api(path, data):
|
|||
logger.error(f"Error posting to {url}: {e}")
|
||||
return False
|
||||
|
||||
def _format_pending_action(action_id: str, data: dict) -> str:
|
||||
"""Build the Telegram Markdown message for a pending action notification.
|
||||
|
||||
Extracted so it can be unit-tested without a live Telegram connection.
|
||||
"""
|
||||
# Supervisor writes risk_level; action-model.md legacy schema used risk.
|
||||
risk = data.get("risk_level") or data.get("risk", "unknown")
|
||||
message = (
|
||||
f"⚠️ *Pending Action*\n"
|
||||
f"ID: `{action_id}`\n"
|
||||
f"Type: `{data.get('type', 'unknown')}`\n"
|
||||
f"Service: `{data.get('service', 'unknown')}`\n"
|
||||
f"Node: `{data.get('node', 'unknown')}`\n"
|
||||
f"Risk: *{risk}*\n"
|
||||
)
|
||||
# description carries the human-readable substance of the action (required for
|
||||
# alert_only actions where it is the entire operator-visible message).
|
||||
description = data.get("description", "")
|
||||
if description:
|
||||
truncated = description[:300] + ("..." if len(description) > 300 else "")
|
||||
message += f"Description: `{truncated}`\n"
|
||||
# Legacy details block (old action-model.md schema) — kept for backwards compat.
|
||||
if "details" in data:
|
||||
details_str = json.dumps(data["details"], indent=2)
|
||||
if len(details_str) > 1000:
|
||||
details_str = details_str[:1000] + "..."
|
||||
message += f"\nDetails:\n```json\n{details_str}\n```"
|
||||
return message
|
||||
|
||||
|
||||
class ApprovalBot:
|
||||
def __init__(self):
|
||||
self.pending_dir = ACTIONS_ROOT / "pending"
|
||||
|
|
@ -86,20 +116,7 @@ class ApprovalBot:
|
|||
|
||||
async def notify_users(self, context: ContextTypes.DEFAULT_TYPE, action_id: str, data: dict):
|
||||
"""Sends an approval request message to all allowed users."""
|
||||
message = (
|
||||
f"⚠️ *Pending Action*\n"
|
||||
f"ID: `{action_id}`\n"
|
||||
f"Type: `{data.get('type', 'unknown')}`\n"
|
||||
f"Service: `{data.get('service', 'unknown')}`\n"
|
||||
f"Node: `{data.get('node', 'unknown')}`\n"
|
||||
f"Risk: *{data.get('risk', 'unknown')}*\n"
|
||||
)
|
||||
|
||||
if "details" in data:
|
||||
details_str = json.dumps(data['details'], indent=2)
|
||||
if len(details_str) > 1000:
|
||||
details_str = details_str[:1000] + "..."
|
||||
message += f"\nDetails:\n```json\n{details_str}\n```"
|
||||
message = _format_pending_action(action_id, data)
|
||||
|
||||
keyboard = [
|
||||
[
|
||||
|
|
|
|||
38
services/agent-system/telegram-bot/tests/conftest.py
Normal file
38
services/agent-system/telegram-bot/tests/conftest.py
Normal file
|
|
@ -0,0 +1,38 @@
|
|||
"""Stub telegram before bot.py is imported so pytest doesn't need the real package."""
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import types
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
|
||||
def _make_telegram_stub() -> types.ModuleType:
|
||||
mod = types.ModuleType("telegram")
|
||||
mod.Update = MagicMock
|
||||
mod.InlineKeyboardButton = MagicMock
|
||||
mod.InlineKeyboardMarkup = MagicMock
|
||||
return mod
|
||||
|
||||
|
||||
def _make_telegram_ext_stub() -> types.ModuleType:
|
||||
mod = types.ModuleType("telegram.ext")
|
||||
mod.ApplicationBuilder = MagicMock
|
||||
|
||||
# ContextTypes.DEFAULT_TYPE is referenced as a type annotation at class-body
|
||||
# evaluation time, so it must be a real attribute, not a dynamic MagicMock attr.
|
||||
ContextTypesMock = MagicMock()
|
||||
ContextTypesMock.DEFAULT_TYPE = type(None)
|
||||
mod.ContextTypes = ContextTypesMock
|
||||
|
||||
mod.CommandHandler = MagicMock
|
||||
mod.CallbackQueryHandler = MagicMock
|
||||
mod.MessageHandler = MagicMock
|
||||
mod.filters = MagicMock()
|
||||
return mod
|
||||
|
||||
|
||||
# Insert before any import of bot.py
|
||||
if "telegram" not in sys.modules:
|
||||
sys.modules["telegram"] = _make_telegram_stub()
|
||||
if "telegram.ext" not in sys.modules:
|
||||
sys.modules["telegram.ext"] = _make_telegram_ext_stub()
|
||||
116
services/agent-system/telegram-bot/tests/test_format.py
Normal file
116
services/agent-system/telegram-bot/tests/test_format.py
Normal file
|
|
@ -0,0 +1,116 @@
|
|||
"""Tests for _format_pending_action — no Telegram connection required.
|
||||
|
||||
telegram stubs are set up in conftest.py before this module is imported.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent))
|
||||
from bot import _format_pending_action
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Bug 1 — risk_level field
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_risk_level_shown_when_present():
|
||||
data = {
|
||||
"type": "container_restart", "service": "homeassistant",
|
||||
"node": "chelsty-ha", "risk_level": "low",
|
||||
}
|
||||
msg = _format_pending_action("container-restart-chelsty-ha-homeassistant", data)
|
||||
assert "Risk: *low*" in msg
|
||||
assert "unknown" not in msg
|
||||
|
||||
|
||||
def test_risk_falls_back_to_legacy_risk_key():
|
||||
data = {
|
||||
"type": "redeploy", "service": "mosquitto",
|
||||
"node": "chelsty-infra", "risk": "guarded",
|
||||
}
|
||||
msg = _format_pending_action("redeploy-chelsty-infra-mosquitto", data)
|
||||
assert "Risk: *guarded*" in msg
|
||||
|
||||
|
||||
def test_risk_unknown_when_both_absent():
|
||||
data = {"type": "redeploy", "service": "foo", "node": "bar"}
|
||||
msg = _format_pending_action("redeploy-bar-foo", data)
|
||||
assert "Risk: *unknown*" in msg
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Bug 2 — description field
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_description_shown_for_alert_only():
|
||||
data = {
|
||||
"type": "alert_only", "service": "homeassistant",
|
||||
"node": "chelsty-ha", "risk_level": "info",
|
||||
"description": "3 entities unavailable for >1h",
|
||||
}
|
||||
msg = _format_pending_action("alert-ha-entity-unavailable-chelsty-ha", data)
|
||||
assert "3 entities unavailable for >1h" in msg
|
||||
assert "Description:" in msg
|
||||
|
||||
|
||||
def test_description_shown_for_container_restart():
|
||||
data = {
|
||||
"type": "container_restart", "service": "homeassistant",
|
||||
"node": "chelsty-ha", "risk_level": "low",
|
||||
"description": "Restart 'homeassistant' on chelsty-ha: HA WebSocket unresponsive",
|
||||
}
|
||||
msg = _format_pending_action("container-restart-chelsty-ha-homeassistant", data)
|
||||
assert "HA WebSocket unresponsive" in msg
|
||||
|
||||
|
||||
def test_description_absent_no_crash():
|
||||
data = {"type": "redeploy", "service": "foo", "node": "bar", "risk_level": "guarded"}
|
||||
msg = _format_pending_action("redeploy-bar-foo", data)
|
||||
assert "Description:" not in msg
|
||||
assert "Risk: *guarded*" in msg
|
||||
|
||||
|
||||
def test_description_truncated_at_300_chars():
|
||||
long_desc = "x" * 400
|
||||
data = {
|
||||
"type": "alert_only", "service": "homeassistant",
|
||||
"node": "chelsty-ha", "risk_level": "info",
|
||||
"description": long_desc,
|
||||
}
|
||||
msg = _format_pending_action("alert-ha-foo-chelsty-ha", data)
|
||||
assert "x" * 300 in msg
|
||||
assert "..." in msg
|
||||
assert "x" * 301 not in msg
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Combined — real HA alert_only action shape
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_ha_alert_only_full_action():
|
||||
"""Mirrors an actual alert_only action written by supervisor._generate_ha_alert_only."""
|
||||
data = {
|
||||
"action_id": "alert-ha-entity-unavailable-chelsty-ha",
|
||||
"type": "alert_only",
|
||||
"node": "chelsty-ha",
|
||||
"service": "homeassistant",
|
||||
"risk_level": "info",
|
||||
"confidence": 1.0,
|
||||
"description": "3 entities unavailable for >1h: sensor.power, binary_sensor.window",
|
||||
"status": "pending",
|
||||
"payload": {
|
||||
"location_tag": "chelsty",
|
||||
"reason": "ha_entity_unavailable_long",
|
||||
"count": 3,
|
||||
},
|
||||
}
|
||||
msg = _format_pending_action(data["action_id"], data)
|
||||
assert "alert_only" in msg
|
||||
assert "chelsty-ha" in msg
|
||||
assert "Risk: *info*" in msg
|
||||
assert "3 entities unavailable" in msg
|
||||
assert "unknown" not in msg
|
||||
Loading…
Reference in a new issue