Skip to content

Commit a6439b8

Browse files
committed
fix(policy): logger.info on silent decide-False; debug log on gt/lt type-mismatch; strengthen event-wake test (#966)
- ApprovalQueue.approve/deny: emit logger.info when the call returns False (unknown token or already decided) — was silent. Helps debug the case where the middleware's consume_and_maybe_remember races with an out-of-band decide. - Evaluator gt/lt TypeError fallback now logs at debug so a user whose 'battery_level < 20' rule never fires can see that the arg came in as a string and tighten the rule. - test_event_wakes_waiter now measures elapsed wait time and asserts < 200ms, ruling out a hidden poll-loop impl that would still pass the previous decision-only check.
1 parent a91ba0f commit a6439b8

3 files changed

Lines changed: 41 additions & 2 deletions

File tree

src/ha_mcp/policy/approval_queue.py

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,16 @@
44

55
import hashlib
66
import json
7+
import logging
78
import secrets
89
from dataclasses import dataclass, field
910
from datetime import UTC, datetime, timedelta
1011
from typing import Any, Literal
1112

1213
import anyio
1314

15+
logger = logging.getLogger(__name__)
16+
1417
Decision = Literal["pending", "approved", "denied"]
1518

1619

@@ -175,15 +178,31 @@ def approve(self, token: str) -> bool:
175178
"""Mark the entry approved. Returns False if unknown or already decided."""
176179
entry = self._by_token.get(token)
177180
if entry is None:
181+
logger.info("approval_queue.approve: unknown token %s", token)
178182
return False
179-
return entry.decide("approved")
183+
ok = entry.decide("approved")
184+
if not ok:
185+
logger.info(
186+
"approval_queue.approve: token %s already decided as %s",
187+
token,
188+
entry.decision,
189+
)
190+
return ok
180191

181192
def deny(self, token: str) -> bool:
182193
"""Mark the entry denied. Returns False if unknown or already decided."""
183194
entry = self._by_token.get(token)
184195
if entry is None:
196+
logger.info("approval_queue.deny: unknown token %s", token)
185197
return False
186-
return entry.decide("denied")
198+
ok = entry.decide("denied")
199+
if not ok:
200+
logger.info(
201+
"approval_queue.deny: token %s already decided as %s",
202+
token,
203+
entry.decision,
204+
)
205+
return ok
187206

188207
def remove(self, token: str) -> None:
189208
self._by_token.pop(token, None)

src/ha_mcp/policy/evaluator.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
"""Evaluate a tool call against a Policy. Pure functions — no I/O, no state."""
22

3+
import logging
34
import re
45
from collections.abc import Iterator
56
from enum import StrEnum
67
from typing import Any
78

89
from .model import Policy, Predicate, Rule
910

11+
logger = logging.getLogger(__name__)
12+
1013

1114
class Verdict(StrEnum):
1215
ALLOW = "allow"
@@ -86,11 +89,20 @@ def _op_matches(val: Any, op: str, pv: Any) -> bool:
8689
try:
8790
return bool(val > pv)
8891
except TypeError:
92+
# Numeric rule against a non-numeric arg value — log so
93+
# users can tell their "battery_level < 20" rule isn't
94+
# silently never firing because the arg is a string.
95+
logger.debug(
96+
"policy: gt type-mismatch (val=%r pv=%r) — predicate skipped", val, pv
97+
)
8998
return False
9099
case "lt":
91100
try:
92101
return bool(val < pv)
93102
except TypeError:
103+
logger.debug(
104+
"policy: lt type-mismatch (val=%r pv=%r) — predicate skipped", val, pv
105+
)
94106
return False
95107
return False
96108

tests/src/unit/policy/test_approval_queue.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,19 @@ async def approver():
169169
await anyio.sleep(0.05)
170170
q.approve(p.token)
171171

172+
elapsed: float = 0.0
172173
async with anyio.create_task_group() as tg:
173174
tg.start_soon(approver)
175+
start = anyio.current_time()
174176
decision = await p.wait()
177+
elapsed = anyio.current_time() - start
175178
assert decision == "approved"
176179
assert p.decision == "approved"
180+
# Event-driven wake should land within ~50ms of the approver firing
181+
# (plus scheduler jitter). A polling impl with a 1s tick would
182+
# easily exceed 200ms here, so this asserts the event path actually
183+
# runs and isn't a hidden poll loop.
184+
assert elapsed < 0.2, f"wait() took {elapsed:.3f}s; expected event wake"
177185

178186

179187
# --- find_or_create + concurrency ---

0 commit comments

Comments
 (0)