refactor(mcp): apply TG-1090 round-2 review feedback

- _format_monitor_cell: precedence reordered so a positive anomaly count
  beats training / pending. Previously a row that surfaced via
  anomaly_type=X could show "training" or "pending" in the X column,
  hiding the value that put it in the filtered set.

- get_monitor_group_summary: empty-state lookback signal restored. The
  outer query now COALESCEs MAX(lookback) to 0 (was 1), preserving the
  pre-refactor dashboard signal for "No monitor runs yet". The fabricated
  default_lookback param is dropped.

- parse_monitor_type: accepts an optional label so the error message names
  the caller's public arg. list_monitored_tables exposes the arg as
  anomaly_type, so the error now says "Invalid anomaly_type ..." instead
  of leaking the helper's internal monitor_type name.

- common.py: drop unused format_monitor_type helper (no callers on this
  branch; re-added in TG-1092 where list_monitors will use it).

Tests:
- list_monitored_tables: new test pinning count > 0 precedence over
  training / pending, and verifying error still wins over count.
- get_monitor_summary: new empty-state test (lookback=0; Window
  start/end fields suppressed; all four cells render the full
  "no results yet or not configured" phrase).
- parse_monitor_type: new label-override test for the anomaly_type
  caller path.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: diogodk

testgen/common/models/table_group.py

@@ -689,7 +689,7 @@ def get_monitor_group_summary(
         )
         query = f"""
         SELECT
-            COALESCE(MAX(lookback), :default_lookback)::INTEGER AS lookback,
+            COALESCE(MAX(lookback), 0)::INTEGER AS lookback,
             MIN(lookback_start) AS lookback_start,
             MAX(lookback_end) AS lookback_end,
             COUNT(*)::INTEGER AS total_monitored_tables,
@@ -719,9 +719,10 @@ def get_monitor_group_summary(
             COALESCE(BOOL_AND(metric_is_pending), TRUE) AS metric_is_pending
         FROM ({inner_query}) AS subquery
         """
-        params = {**params, "default_lookback": lookback_override or 1}
         # Outer query has no GROUP BY — aggregates over zero rows still yield one
-        # COALESCE'd row, so .first() never returns None here.
+        # COALESCE'd row, so .first() never returns None here. ``lookback`` is 0
+        # when the per-table CTE has no rows OR no runs against the monitor suite,
+        # so the dashboard / MCP can render the "no monitor runs yet" state.
         row = get_current_session().execute(text(query), params).mappings().first()
         return MonitorGroupSummary(**row)
 

testgen/mcp/tools/common.py

@@ -357,29 +357,22 @@ def next_scheduled_run(
     "schema": MonitorType.SCHEMA,
     "metric": MonitorType.METRIC,
 }
-_MONITOR_TYPE_DB_TO_USER: dict[MonitorType, str] = {v: k for k, v in _MONITOR_TYPE_USER_TO_DB.items()}
 
 
-def parse_monitor_type(value: str) -> MonitorType:
+def parse_monitor_type(value: str, label: str = "monitor_type") -> MonitorType:
     """Validate a user-facing monitor type label and return the stored ``MonitorType``.
 
-    Accepts ``freshness`` / ``volume`` / ``schema`` / ``metric``.
+    Accepts ``freshness`` / ``volume`` / ``schema`` / ``metric``. ``label`` names the
+    caller's argument in the error message — pass ``"anomaly_type"`` when the
+    public arg is named differently from ``monitor_type``.
     """
     db_value = _MONITOR_TYPE_USER_TO_DB.get(value)
     if db_value is None:
         valid = ", ".join(_MONITOR_TYPE_USER_TO_DB)
-        raise MCPUserError(f"Invalid monitor_type `{value}`. Valid values: {valid}")
+        raise MCPUserError(f"Invalid {label} `{value}`. Valid values: {valid}")
     return db_value
 
 
-def format_monitor_type(value: MonitorType | str) -> str:
-    """Map a stored monitor ``test_type`` to its user-facing short label."""
-    try:
-        return _MONITOR_TYPE_DB_TO_USER[MonitorType(value)]
-    except ValueError:
-        return str(value)
-
-
 class MonitorTableSort(StrEnum):
     """User-facing values accepted for the ``sort_by`` argument on ``list_monitored_tables``.
 

testgen/mcp/tools/monitors.py

@@ -150,7 +150,7 @@ def list_monitored_tables(
     validate_page(page)
     validate_limit(limit, 100)
 
-    monitor_type = parse_monitor_type(anomaly_type) if anomaly_type is not None else None
+    monitor_type = parse_monitor_type(anomaly_type, "anomaly_type") if anomaly_type is not None else None
     sort = parse_monitor_table_sort(sort_by) if sort_by is not None else None
 
     tg, monitor_suite = resolve_monitored_table_group(table_group_id)
@@ -245,14 +245,22 @@ def _format_monitor_cell(
     is_training: bool | None,
     has_error: bool,
 ) -> str:
-    """Render a per-table, per-monitor cell (non-schema)."""
+    """Render a per-table, per-monitor cell (non-schema).
+
+    Precedence: error > positive count > pending > training > zero. A positive
+    count wins over training/pending so anomalies stay visible when a monitor is
+    still learning, and so a row that surfaces via ``anomaly_type=X`` actually
+    shows the X count in its column.
+    """
     if has_error:
         return "error"
+    if count > 0:
+        return str(count)
     if is_pending:
         return "pending"
     if is_training:
         return "training"
-    return str(count)
+    return "0"
 
 
 def _format_schema_cell(row: MonitorTableSummary) -> str:

tests/unit/mcp/test_tools_common.py

@@ -873,6 +873,15 @@ def test_parse_monitor_type_lists_valid_values_on_error():
         assert label in msg
 
 
+def test_parse_monitor_type_label_override():
+    """``label`` argument lets callers tailor the error to their public arg name
+    (e.g. ``list_monitored_tables`` exposes it as ``anomaly_type``)."""
+    from testgen.mcp.tools.common import parse_monitor_type
+
+    with pytest.raises(MCPUserError, match=r"Invalid anomaly_type `bogus`"):
+        parse_monitor_type("bogus", "anomaly_type")
+
+
 @pytest.mark.parametrize(
     "value",
     ["table_name", "anomaly_count_desc", "latest_update_desc", "row_count_change_desc"],
@@ -893,15 +902,6 @@ def test_parse_monitor_table_sort_rejects_unknown():
         assert valid in msg
 
 
-def test_format_monitor_type_round_trips():
-    from testgen.common.enums import MonitorType
-    from testgen.mcp.tools.common import format_monitor_type
-
-    for member in MonitorType:
-        formatted = format_monitor_type(member)
-        assert format_monitor_type(member.value) == formatted
-
-
 def test_resolve_monitored_table_group_returns_suite():
     from testgen.common.models.table_group import TableGroup
     from testgen.common.models.test_suite import TestSuite

tests/unit/mcp/test_tools_monitors.py

@@ -204,6 +204,45 @@ def test_get_monitor_summary_lookback_out_of_range(db_session_mock):
     assert "between 1 and 365" in str(exc.value)
 
 
+@patch(f"{MODULE}.next_scheduled_run", return_value=None)
+@patch(f"{MODULE}.TableGroup")
+@patch(f"{MODULE}.resolve_monitored_table_group")
+def test_get_monitor_summary_empty_state_lookback_zero(
+    mock_resolve, mock_tg_cls, mock_next, db_session_mock,
+):
+    """When a monitor suite is configured but no runs have happened yet, the model
+    method returns ``lookback=0`` (preserves the pre-refactor signal the dashboard
+    uses to render "No monitor runs yet"). The MCP output reflects the empty state
+    rather than fabricating a one-run window."""
+    tg = _mock_table_group()
+    mock_resolve.return_value = (tg, _mock_monitor_suite())
+    mock_tg_cls.get_monitor_group_summary.return_value = _group_summary(
+        lookback=0,
+        lookback_start=None,
+        lookback_end=None,
+        total_monitored_tables=0,
+        freshness_anomalies=0, volume_anomalies=0,
+        schema_anomalies=0, metric_anomalies=0,
+        freshness_is_pending=True, volume_is_pending=True,
+        schema_is_pending=True, metric_is_pending=True,
+        freshness_is_training=False, volume_is_training=False,
+        metric_is_training=False,
+    )
+
+    from testgen.mcp.tools.monitors import get_monitor_summary
+
+    with _patch_perms():
+        out = get_monitor_summary(str(tg.id))
+
+    assert "**Lookback:** 0 runs" in out, "must show 0, not a fabricated default like 1"
+    # Window start / end fields are absent in the empty case (None values render as em-dash
+    # at most, but the tool omits them when the value is falsy)
+    assert "**Window start:**" not in out
+    assert "**Window end:**" not in out
+    # All per-type status cells reflect "no results"
+    assert out.count("no results yet or not configured") == 4
+
+
 @patch(f"{MODULE}.next_scheduled_run", return_value=None)
 @patch(f"{MODULE}.TableGroup")
 @patch(f"{MODULE}.resolve_monitored_table_group")
@@ -320,11 +359,15 @@ def test_list_monitored_tables_not_monitored(mock_resolve, db_session_mock):
 
 
 def test_list_monitored_tables_invalid_anomaly_type(db_session_mock):
+    """Error message must reference the caller's public arg name (``anomaly_type``)
+    rather than the helper's internal arg name (``monitor_type``)."""
     from testgen.mcp.tools.monitors import list_monitored_tables
 
     with _patch_perms(), pytest.raises(MCPUserError) as exc:
         list_monitored_tables(str(uuid4()), anomaly_type="bogus")
-    assert "Invalid monitor_type" in str(exc.value)
+    msg = str(exc.value)
+    assert "Invalid anomaly_type" in msg
+    assert "Invalid monitor_type" not in msg
 
 
 def test_list_monitored_tables_invalid_sort_by(db_session_mock):
@@ -403,15 +446,16 @@ def test_list_monitored_tables_schema_change_column(
 def test_list_monitored_tables_training_and_pending_cells(
     mock_resolve, mock_tg_cls, db_session_mock,
 ):
+    """Status words render when the per-type count is zero."""
     tg = _mock_table_group()
     mock_resolve.return_value = (tg, _mock_monitor_suite())
     mock_tg_cls.list_monitor_table_summaries.return_value = (
         [
             _table_summary(
                 table_name="t1",
-                freshness_is_training=True,
-                volume_is_pending=True,
-                metric_error_message="boom",
+                freshness_anomalies=0, freshness_is_training=True,
+                volume_anomalies=0, volume_is_pending=True,
+                metric_anomalies=0, metric_error_message="boom",
             ),
         ],
         1,
@@ -428,6 +472,54 @@ def test_list_monitored_tables_training_and_pending_cells(
     assert "error" in row
 
 
+@patch(f"{MODULE}.TableGroup")
+@patch(f"{MODULE}.resolve_monitored_table_group")
+def test_list_monitored_tables_count_wins_over_training_and_pending(
+    mock_resolve, mock_tg_cls, db_session_mock,
+):
+    """A positive anomaly count must surface even when the monitor is in training
+    or pending state — otherwise the cell hides the value that made the row match
+    an ``anomaly_type`` filter, and the table doesn't agree with itself.
+
+    Precedence is error > positive count > pending > training > zero. Errors still
+    win (the latest measurement is suspect, so the historic count is misleading).
+    """
+    tg = _mock_table_group()
+    mock_resolve.return_value = (tg, _mock_monitor_suite())
+    mock_tg_cls.list_monitor_table_summaries.return_value = (
+        [
+            _table_summary(
+                table_name="t_busy_during_learn",
+                freshness_anomalies=5, freshness_is_training=True,
+                volume_anomalies=3, volume_is_pending=True,
+                metric_anomalies=2, metric_is_training=True,
+            ),
+            _table_summary(
+                table_name="t_error_with_count",
+                freshness_anomalies=4, freshness_error_message="db down",
+            ),
+        ],
+        2,
+    )
+
+    from testgen.mcp.tools.monitors import list_monitored_tables
+
+    with _patch_perms():
+        out = list_monitored_tables(str(tg.id))
+
+    # Row 1: counts visible despite training/pending
+    row_count = next(line for line in out.splitlines() if "`t_busy_during_learn`" in line)
+    assert " 5 " in row_count, "freshness count should render despite is_training"
+    assert " 3 " in row_count, "volume count should render despite is_pending"
+    assert " 2 " in row_count, "metric count should render despite is_training"
+    assert "training" not in row_count
+    assert "pending" not in row_count
+    # Row 2: error still wins over count
+    row_error = next(line for line in out.splitlines() if "`t_error_with_count`" in line)
+    assert "error" in row_error
+    assert " 4 " not in row_error, "error must win over count (measurement is suspect)"
+
+
 @patch(f"{MODULE}.TableGroup")
 @patch(f"{MODULE}.resolve_monitored_table_group")
 def test_list_monitored_tables_empty(mock_resolve, mock_tg_cls, db_session_mock):