Ryan Malloy
90227ab391
Closes the four remaining findings from the margaret-hamilton review. 13 new regression tests; all 100 pass; live cluster smoke verified. MAJOR #4 — wildcard regex catastrophic backtracking + silent malformed. Two changes to _wildcard_to_regex(): a) Bounded the `!` and `@` wildcards to \d{1,50} (was \d+). Adjacent `!` patterns previously compiled to (\d+)(\d+)... which has exponential backtracking on near-miss inputs. CUCM dial strings are practically capped well below 50 digits; the bound keeps complexity polynomial without losing real-world coverage. Verified: 10 adjacent `!` against a 30-digit near-miss now finishes in ~240ms (was unbounded; could have been minutes on real pathological cases). b) Unclosed `[` now raises ValueError instead of silently treating the bracket as a literal. _pattern_matches_number catches the error and returns False so a single bad pattern doesn't crash translation_chain — but the bad pattern is no longer invisibly producing wrong matches. The previous silent fallback meant a pattern like `[0-9` (typo, missing `]`) would match input containing the literal characters `[` `0` `-` `9`. 3 new tests covering: bounded-regex shape (`\d{1,N}`), pathological input completes quickly, unclosed bracket raises explicitly, well-formed character class still works. MAJOR #5 — distinguish config errors from operational errors. Pre-fix: any first-time connection failure set `_connection_error` and pinned it forever. A transient network blip or session timeout required restarting the MCP server. Hamilton's framing: Apollo's software was *designed* to recover from transient faults; pinning forever is the antithesis of "design the error path first." Fix: split into two state fields: _config_error — permanent until restart (missing env vars only) _last_error — last operational failure, NOT a pin Operational failures (zeep Client construction, network, TLS, session) clear from the next call's perspective: the next call attempts fresh. Configuration errors (missing AXL_URL etc.) stay pinned because they don't get better on retry. Added _ConfigError as a private subclass to make the distinction explicit at the raise site, and connection_status() to expose connected/connected_at/config_error/last_error for diagnostic transparency. 3 new tests: config errors pin, operational errors don't pin, connection_status() reports state. MINOR #6 — _to_int silent coercion of bad data. Pre-fix: a non-numeric value from the cluster (data corruption, schema drift across CUCM versions) silently became None, which downstream sort logic defaulted to 0 — jumbling the failover order in the displayed result with no warning. Fix: still returns None on bad data (caller error path unchanged), but logs the offending value to stderr so an operator notices something's wrong at the data layer. None itself is silent (legitimately-unset column). 2 new tests: real None is silent, bad string logs to stderr with the offending value visible. MINOR #7 — standardize tool failure shapes; add health() tool. Pre-fix: cache_stats and cache_clear returned `{"error": "..."}` when _cache was None, while AXL-touching tools raised RuntimeError. LLM consumers had to handle two shapes. Fix: _require_cache() helper raises RuntimeError consistently with _client(). All tool failures now use the same exception shape. Added health() tool that reports cache/axl/docs initialization status plus the AXL connection_status — gives operators a self-diagnostic when something fails at bootstrap. 3 new tests: cache_stats raises, cache_clear raises, health() reports each subsystem.
41 lines
1.6 KiB
Python
41 lines
1.6 KiB
Python
"""Hamilton review MINOR #7: standardize tool-failure error shapes.
|
|
|
|
Pre-fix: tools that need state (`_axl`, `_cache`, `_docs`) had inconsistent
|
|
error shapes. Most tools called `_client()` which raises `RuntimeError`.
|
|
But `cache_stats` and `cache_clear` checked `if _cache is None` and
|
|
returned `{"error": "..."}`. An LLM consuming responses had to handle
|
|
two different patterns. After the fix, both shapes converge: all tools
|
|
raise RuntimeError when their dependencies aren't initialized.
|
|
"""
|
|
|
|
import pytest
|
|
|
|
from mcp_cucm_axl import server
|
|
|
|
|
|
def test_cache_stats_raises_when_uninitialized(monkeypatch):
|
|
monkeypatch.setattr(server, "_cache", None)
|
|
with pytest.raises(RuntimeError, match=r"[Cc]ache"):
|
|
# @mcp.tool passes the function through unchanged; call directly.
|
|
server.cache_stats()
|
|
|
|
|
|
def test_cache_clear_raises_when_uninitialized(monkeypatch):
|
|
monkeypatch.setattr(server, "_cache", None)
|
|
with pytest.raises(RuntimeError, match=r"[Cc]ache"):
|
|
server.cache_clear()
|
|
|
|
|
|
def test_health_check_reports_each_subsystem(monkeypatch):
|
|
"""A health-check tool should report which globals are initialized,
|
|
so an operator (or an LLM) can diagnose `RuntimeError: ... not initialized`
|
|
issues without grepping source."""
|
|
# When all are None, health should report all three as down
|
|
monkeypatch.setattr(server, "_cache", None)
|
|
monkeypatch.setattr(server, "_axl", None)
|
|
monkeypatch.setattr(server, "_docs", None)
|
|
info = server.health()
|
|
assert info["cache"] is False
|
|
assert info["axl"] is False
|
|
assert info["docs"] is False
|