refactor(mcp): lift render_diff_table + resolve_project to common (TG-1070 review S5)

The diff-table render loop was duplicated three places (table_groups, projects,
test_suites). The verify_access + Project.get + None-check trio in update_project
also duplicated the resolve_* idiom already used for connections, table groups,
and test suites. Both moves were flagged in the TG-1070 review.

- render_diff_table(doc, before, after, *, attrs, labels, secret_attrs, value_renderer)
  in common.py owns the changed-attrs iteration, label lookup, secret redaction,
  and table emit. Each tool retains its own _DIFF_ORDER / _DIFF_LABELS and its
  own snapshot function (different shapes per entity).

- resolve_project(project_code) joins the existing resolve_* family. Uses
  Project.get(code, Project.project_code.in_(allowed_codes)) so the unified
  not-found-or-not-accessible error path matches resolve_test_suite and
  resolve_table_group: out-of-scope projects never reveal whether they exist.

update_connection keeps its own custom diff renderer — the "[secret] (rotated)"
cue it emits for secret writes is semantically distinct from the generic
"[secret]" redaction and isn't worth bending the helper to model both.

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

Author: 2 people

testgen/mcp/tools/common.py

@@ -1,3 +1,4 @@
+from collections.abc import Callable, Mapping, Sequence
 from dataclasses import dataclass, field
 from datetime import date, datetime
 from enum import StrEnum
@@ -35,6 +36,7 @@
     TestRunNotificationTrigger,
 )
 from testgen.common.models.profiling_run import ProfilingRun
+from testgen.common.models.project import Project
 from testgen.common.models.scheduler import SCHEDULABLE_JOB_KEYS, JobSchedule
 from testgen.common.models.scores import ScoreCategory, ScoreDefinition
 from testgen.common.models.table_group import TableGroup
@@ -628,10 +630,89 @@ def format_page_footer(total: int, page: int, limit: int) -> str:
     return f"_Page {page} of {total_pages}. Use `page={page + 1}` for more._"
 
 
+def _default_render_diff_value(value: object) -> str | None:
+    """Default formatter for before/after cells in :func:`render_diff_table`.
+
+    * ``bool`` → ``"Yes"`` / ``"No"``
+    * ``None`` or ``""`` → ``None`` (rendered as em-dash by the table cell)
+    * else → ``str(value)``
+    """
+    if isinstance(value, bool):
+        return "Yes" if value else "No"
+    if value is None or value == "":
+        return None
+    return str(value)
+
+
+def render_diff_table(
+    doc: MdDoc,
+    before: Mapping[str, object],
+    after: Mapping[str, object],
+    *,
+    attrs: Sequence[str],
+    labels: Mapping[str, str],
+    secret_attrs: frozenset[str] = frozenset(),
+    value_renderer: Callable[[object], str | None] | None = None,
+) -> bool:
+    """Emit a ``Field / Before / After`` table for attrs whose values changed.
+
+    Update tools across the MCP surface share this shape: snapshot before, apply
+    field-by-field, snapshot after, render the delta. The shared helper keeps the
+    ordering, label lookup, and secret redaction consistent.
+
+    Args:
+        doc: ``MdDoc`` to append the table to.
+        before / after: same-shape dicts of the snapshot keyed by attr name.
+            Only entries in ``attrs`` are inspected.
+        attrs: ordered tuple of attrs to consider; controls row order.
+        labels: attr → user-facing label for the first column.
+        secret_attrs: attrs whose values must not be echoed (API keys, passwords).
+            Rendered as ``[secret]`` when present, em-dash when absent — distinct
+            from a "rotated" cue. Tools that need to communicate rotation
+            specifically (e.g. ``update_connection``) keep their own renderer.
+        value_renderer: override for the non-secret cells. Defaults to
+            :func:`_default_render_diff_value`.
+
+    Returns ``True`` if at least one row was rendered, ``False`` when nothing
+    changed (lets the caller emit a "No fields changed" message instead).
+    """
+    changed = {attr for attr in attrs if before.get(attr) != after.get(attr)}
+    if not changed:
+        return False
+    render = value_renderer or _default_render_diff_value
+    rows: list[list[object]] = []
+    for attr in attrs:
+        if attr not in changed:
+            continue
+        if attr in secret_attrs:
+            b = "[secret]" if before.get(attr) else None
+            a = "[secret]" if after.get(attr) else None
+        else:
+            b = render(before.get(attr))
+            a = render(after.get(attr))
+        rows.append([labels.get(attr, attr), b, a])
+    doc.table(["Field", "Before", "After"], rows, code=[0])
+    return True
+
+
 # Entity resolution helpers — see mcp-roadmap.md "Entity Resolution Helpers" guideline.
 # Extract a new resolve_<entity> here when a second caller needs the same parse-uuid +
 # perm-scoped lookup + collapsed-error pattern.
 
+
+def resolve_project(project_code: str) -> Project:
+    """Resolve a project code to a Project, scoped to the user's allowed projects.
+
+    Collapses missing-or-inaccessible into a single ``MCPResourceNotAccessible`` so
+    callers can't enumerate whether a project they can't see exists.
+    """
+    perms = get_project_permissions()
+    project = Project.get(project_code, Project.project_code.in_(perms.allowed_codes))
+    if project is None:
+        raise MCPResourceNotAccessible("Project", project_code)
+    return project
+
+
 def resolve_connection(connection_id: int) -> Connection:
     """Resolve a connection ID, collapsing missing-or-inaccessible into one error path."""
     perms = get_project_permissions()

testgen/mcp/tools/projects.py

@@ -18,9 +18,14 @@
     DEFAULT_RETENTION_CRON_TZ,
     JobSchedule,
 )
-from testgen.mcp.exceptions import MCPResourceNotAccessible, MCPUserError
-from testgen.mcp.permissions import get_project_permissions, mcp_permission
-from testgen.mcp.tools.common import DocGroup, raise_validation_error
+from testgen.mcp.exceptions import MCPUserError
+from testgen.mcp.permissions import mcp_permission
+from testgen.mcp.tools.common import (
+    DocGroup,
+    raise_validation_error,
+    render_diff_table,
+    resolve_project,
+)
 from testgen.mcp.tools.markdown import MdDoc
 
 _DOC_GROUP = DocGroup.MANAGE
@@ -77,12 +82,7 @@ def update_project(
     if all(value is None for value in supplied.values()):
         raise MCPUserError("No fields supplied to update.")
 
-    perms = get_project_permissions()
-    perms.verify_access(project_code, not_found=MCPResourceNotAccessible("Project", project_code))
-
-    project = Project.get(project_code)
-    if project is None:
-        raise MCPResourceNotAccessible("Project", project_code)
+    project = resolve_project(project_code)
 
     schedule = JobSchedule.get(
         JobSchedule.project_code == project_code,
@@ -164,12 +164,15 @@ def update_project(
         if effective_enabled
         else None,
     )
-    changed = {attr for attr in before if before[attr] != after[attr]}
 
     doc = MdDoc()
     doc.heading(1, f"Project `{project_code}` updated")
 
-    if not changed:
+    rendered = render_diff_table(
+        doc, before, after,
+        attrs=_DIFF_ORDER, labels=_DIFF_LABELS, secret_attrs=_SECRET_ATTRS,
+    )
+    if not rendered:
         doc.text("No fields changed — supplied values matched the current state.")
         return doc.render()
 
@@ -196,12 +199,6 @@ def update_project(
             project_code=project_code,
         )
 
-    rows: list[list[object]] = []
-    for attr in _DIFF_ORDER:
-        if attr not in changed:
-            continue
-        rows.append([_DIFF_LABELS[attr], _render_field_value(attr, before[attr]), _render_field_value(attr, after[attr])])
-    doc.table(["Field", "Before", "After"], rows, code=[0])
     return doc.render()
 
 
@@ -249,14 +246,4 @@ def _snapshot(project: Project, schedule: Any) -> dict[str, Any]:
     "retention_cron_tz": "Retention timezone",
 }
 
-
-def _render_field_value(attr: str, value: Any) -> str | None:
-    # Redact the API key — mcp-patterns "Secrets in inputs" rule. The diff still tells the LLM
-    # the field changed; it just never echoes the value (cleartext OR masked).
-    if attr == "observability_api_key" and value:
-        return "[secret]"
-    if isinstance(value, bool):
-        return "Yes" if value else "No"
-    if value is None or value == "":
-        return None
-    return str(value)
+_SECRET_ATTRS: frozenset[str] = frozenset({"observability_api_key"})

testgen/mcp/tools/table_groups.py

@@ -25,6 +25,7 @@
     format_flavor_label,
     format_page_footer,
     format_page_info,
+    render_diff_table,
     resolve_connection,
     resolve_table_group,
     validate_limit,
@@ -424,13 +425,13 @@ def update_table_group(
         _raise_validation_error(errors, "Update rejected. No changes saved.")
 
     after = _snapshot(table_group)
-    changed = {attr for attr in before if before[attr] != after[attr]}
 
     doc = MdDoc()
     doc.heading(1, f"Table Group `{table_group.table_groups_name}` updated")
     doc.field("ID", str(table_group.id), code=True)
 
-    if not changed:
+    rendered = render_diff_table(doc, before, after, attrs=_DIFF_ATTRS, labels=_DIFF_LABELS)
+    if not rendered:
         doc.text("No fields changed — supplied values matched the current state.")
         return doc.render()
 
@@ -440,13 +441,6 @@ def update_table_group(
         _maybe_raise_duplicate_name(err)
         raise
 
-    rows: list[list[object]] = []
-    for attr in _DIFF_ATTRS:
-        if attr not in changed:
-            continue
-        label = _DIFF_LABELS.get(attr, attr)
-        rows.append([label, _render_field_value(before[attr]), _render_field_value(after[attr])])
-    doc.table(["Field", "Before", "After"], rows, code=[0])
     return doc.render()
 
 
@@ -722,14 +716,6 @@ def _snapshot(table_group: TableGroup) -> dict[str, Any]:
     return {attr: getattr(table_group, attr, None) for attr in _DIFF_ATTRS}
 
 
-def _render_field_value(value: Any) -> str | None:
-    if isinstance(value, bool):
-        return "Yes" if value else "No"
-    if value is None or value == "":
-        return None
-    return str(value)
-
-
 def _render_created_table_group(table_group: TableGroup, connection: Connection) -> str:
     doc = MdDoc()
     doc.heading(1, f"Table Group `{table_group.table_groups_name}` created")

testgen/mcp/tools/test_suites.py

@@ -19,6 +19,7 @@
     DocGroup,
     parse_severity,
     raise_validation_error,
+    render_diff_table,
     resolve_table_group,
     resolve_test_suite,
 )
@@ -193,24 +194,17 @@ def update_test_suite(
     for attr, value in updates.items():
         setattr(suite, attr, value)
     after = {attr: getattr(suite, attr, None) for attr in updates}
-    changed = {attr for attr in before if before[attr] != after[attr]}
 
     doc = MdDoc()
     doc.heading(1, f"Test Suite `{suite.test_suite}` updated")
     doc.field("ID", str(suite.id), code=True)
 
-    if not changed:
+    rendered = render_diff_table(doc, before, after, attrs=_DIFF_ORDER, labels=_DIFF_LABELS)
+    if not rendered:
         doc.text("No fields changed — supplied values matched the current state.")
         return doc.render()
 
     suite.save()
-
-    rows: list[list[object]] = []
-    for attr in _DIFF_ORDER:
-        if attr not in changed:
-            continue
-        rows.append([_DIFF_LABELS[attr], _render_field_value(before[attr]), _render_field_value(after[attr])])
-    doc.table(["Field", "Before", "After"], rows, code=[0])
     return doc.render()
 
 
@@ -270,11 +264,3 @@ def _render_created_suite(suite: TestSuite, *, table_group_name: str) -> str:
     "component_name": "Component name",
     "dq_score_exclude": "Exclude from quality scoring",
 }
-
-
-def _render_field_value(value: Any) -> str | None:
-    if isinstance(value, bool):
-        return "Yes" if value else "No"
-    if value is None or value == "":
-        return None
-    return str(value)