fix(mcp): address Copilot review feedback from #42, #43, #44 (#45)

Retroactive follow-up addressing Copilot inline comments that arrived after PRs #42, #43, #44 were merged. Six findings across the three PRs:

**From #42**: orders.py module docstring described `search` as matching "order number" only; per OpenAPI it partially matches order name, order number, customer name, or customer email.

**From #43**: `_to_detail()` with `history_limit=0` silently returned the full list (because `items[-0:]` is `items[0:]`) while setting `history_truncated=True` — now raises `ValueError`. Added `_paginate_history()` helper with 9 unit tests for the pagination contract. Renamed misleading test `test_custom_history_limit_zero_is_not_supported` (which was actually testing limit=1).

**From #44**: `StatusChangePreview.valid` description and `update_order_status` help row both implied no API call happens at preview validation; reworded to clarify the cached read to `viable-statuses` and that direct `confirm=True` calls bypass the UI guard.

Going forward I'm waiting for Copilot's async review pass before merging.

Author: dougborg

statuspro_mcp_server/src/statuspro_mcp/resources/help.py

@@ -15,7 +15,7 @@
 | `get_order` | `GET /orders/{id}` | Full detail for one order, with the most recent `history_limit` history entries (default 50). When `history_truncated` is true, use `get_order_history` for older entries. |
 | `get_order_history` | `GET /orders/{id}` (client-side paged) | Page through the full history timeline of one order. Use when `get_order` indicated truncation. |
 | `get_viable_statuses` | `GET /orders/{id}/viable-statuses` | Valid status transitions for the order's current state. |
-| `update_order_status` | `POST /orders/{id}/status` | Change status. Two-step confirm. Preview self-validates against viable transitions — invalid `status_code` is caught at preview time without the API round-trip. |
+| `update_order_status` | `POST /orders/{id}/status` | Change status. Two-step confirm. Preview self-validates against viable transitions (one extra read to `GET /orders/{id}/viable-statuses`, cached) — invalid `status_code` is surfaced before the write `POST` that would 422. |
 | `add_order_comment` | `POST /orders/{id}/comment` | Add a history comment. Two-step confirm. 5/min. |
 | `update_order_due_date` | `POST /orders/{id}/due-date` | Change due date / date range. Two-step confirm. |
 | `bulk_update_order_status` | `POST /orders/bulk-status` | Update up to 50 orders in one request. Two-step confirm. 5/min. |

statuspro_mcp_server/src/statuspro_mcp/tools/orders.py

@@ -11,7 +11,8 @@
 The ``GET /orders/lookup`` endpoint is intentionally not exposed: it is the
 public, customer-verification path (requires ``number`` + ``email``) and adds
 nothing for an authenticated MCP caller, who can already use ``list_orders``
-(``search`` matches order number) or ``get_order`` (by id).
+(``search`` partially matches order name, order number, customer name, or
+customer email) or ``get_order`` (by id).
 """
 
 from __future__ import annotations
@@ -107,7 +108,15 @@ def _to_detail(
     Callers receive ``history_truncated`` + ``history_total_count`` to
     detect truncation and page through older entries via
     ``get_order_history``.
+
+    ``history_limit`` must be >= 1. The MCP tool layer enforces this via
+    ``Field(ge=1)``, but the helper guards explicitly: Python's
+    ``items[-0:]`` returns the full list (since ``-0 == 0``), which would
+    silently contradict ``history_truncated=True`` when called with 0.
     """
+    if history_limit < 1:
+        msg = f"history_limit must be >= 1, got {history_limit}"
+        raise ValueError(msg)
     summary = _to_summary(order)
     history_items = getattr(order, "history", None) or []
     total = len(history_items)
@@ -122,6 +131,32 @@ def _to_detail(
     )
 
 
+def _paginate_history(
+    items: list[Any], *, page: int, per_page: int
+) -> tuple[list[Any], int, int]:
+    """Slice the in-memory history array into one page.
+
+    Returns ``(page_items, total, total_pages)``. Extracted so the
+    pagination contract is testable without spinning up FastMCP to
+    invoke the registered ``get_order_history`` tool.
+
+    ``page`` and ``per_page`` are assumed >= 1 (enforced at the tool
+    layer via ``Field(ge=1)``); guard explicitly so a misuse raises
+    rather than producing nonsensical slices.
+    """
+    if page < 1:
+        msg = f"page must be >= 1, got {page}"
+        raise ValueError(msg)
+    if per_page < 1:
+        msg = f"per_page must be >= 1, got {per_page}"
+        raise ValueError(msg)
+    total = len(items)
+    total_pages = max(1, (total + per_page - 1) // per_page)
+    start = (page - 1) * per_page
+    end = start + per_page
+    return items[start:end], total, total_pages
+
+
 async def _status_color_catalog(services: Any) -> dict[str, str | None]:
     """Fetch the status catalog once and return a ``{code: color}`` map.
 
@@ -339,14 +374,13 @@ async def get_order_history(
     ) -> OrderHistoryPage:
         services = get_services(context)
         # No server-side history pagination today — fetch the full order and
-        # slice client-side. Cheap relative to LLM context cost.
+        # slice client-side via _paginate_history. Cheap relative to LLM
+        # context cost; the helper makes the slicing math testable.
         order = await services.client.orders.get(order_id)
         all_items = getattr(order, "history", None) or []
-        total = len(all_items)
-        total_pages = max(1, (total + per_page - 1) // per_page)
-        start = (page - 1) * per_page
-        end = start + per_page
-        page_items = all_items[start:end]
+        page_items, total, total_pages = _paginate_history(
+            all_items, page=page, per_page=per_page
+        )
         return OrderHistoryPage(
             order_id=order_id,
             page=page,

statuspro_mcp_server/src/statuspro_mcp/tools/schemas.py

@@ -171,7 +171,10 @@ class StatusChangePreview(BaseModel):
         default=True,
         description=(
             "True when new_status_code is a viable transition from the "
-            "current state. False blocks confirmation."
+            "current state (per GET /orders/{id}/viable-statuses). When "
+            "False, the Prefab UI hides the Confirm button and surfaces "
+            "viable_status_codes; the agent can still call the tool with "
+            "confirm=True directly, in which case the API will likely 422."
         ),
     )
     viable_status_codes: list[str] = Field(

statuspro_mcp_server/tests/tools/test_orders_history_truncation.py

@@ -3,7 +3,8 @@
 Covers the contract from issue #40: get_order returns the most recent
 `history_limit` entries (default 50) with `history_truncated` and
 `history_total_count` flags so callers know to use get_order_history
-for older entries.
+for older entries. The pagination contract for get_order_history itself
+is exercised via the extracted `_paginate_history` helper.
 """
 
 from __future__ import annotations
@@ -14,6 +15,7 @@
 from statuspro_mcp.tools.orders import (
     DEFAULT_HISTORY_LIMIT,
     _history_entry,
+    _paginate_history,
     _to_detail,
 )
 
@@ -93,17 +95,95 @@ def test_custom_history_limit_smaller(self):
         assert detail.history_truncated is True
         assert detail.history_total_count == 20
 
-    def test_custom_history_limit_zero_is_not_supported(self):
-        """history_limit must be >= 1 — guarded at the MCP tool layer.
-        Internal _to_detail with limit=0 would slice to []; verify the slice
-        math doesn't blow up (defensive).
+    def test_custom_history_limit_one_returns_most_recent_entry(self):
+        """The MCP tool enforces history_limit >= 1; this verifies the
+        smallest valid limit returns exactly the most recent entry.
         """
         order = _make_order(history_count=5)
-        # The tool itself rejects limit < 1 via Field(ge=1), but the helper
-        # is permissive — verify it produces a sane result.
         detail = _to_detail(order, history_limit=1)
         assert len(detail.history) == 1
         assert detail.history_truncated is True
+        assert detail.history_total_count == 5
+
+    def test_history_limit_zero_raises(self):
+        """The helper rejects history_limit < 1 explicitly. Without the
+        guard, ``items[-0:]`` returns the FULL list (since ``-0 == 0``),
+        which would silently contradict ``history_truncated=True``.
+        """
+        order = _make_order(history_count=5)
+        with pytest.raises(ValueError, match="history_limit must be >= 1"):
+            _to_detail(order, history_limit=0)
+
+    def test_history_limit_negative_raises(self):
+        order = _make_order(history_count=5)
+        with pytest.raises(ValueError, match="history_limit must be >= 1"):
+            _to_detail(order, history_limit=-1)
+
+
+@pytest.mark.unit
+class TestPaginateHistory:
+    """_paginate_history slices an in-memory list into page+total+total_pages."""
+
+    def test_empty_list_first_page(self):
+        items, total, total_pages = _paginate_history([], page=1, per_page=10)
+        assert items == []
+        assert total == 0
+        # Empty list still reports total_pages=1 so callers can iterate
+        # ``for p in range(1, total_pages+1)`` without an off-by-one.
+        assert total_pages == 1
+
+    def test_single_full_page(self):
+        items, total, total_pages = _paginate_history(
+            list(range(5)), page=1, per_page=10
+        )
+        assert items == [0, 1, 2, 3, 4]
+        assert total == 5
+        assert total_pages == 1
+
+    def test_multiple_pages_first(self):
+        items, total, total_pages = _paginate_history(
+            list(range(25)), page=1, per_page=10
+        )
+        assert items == list(range(10))
+        assert total == 25
+        assert total_pages == 3
+
+    def test_multiple_pages_middle(self):
+        items, _, _ = _paginate_history(list(range(25)), page=2, per_page=10)
+        assert items == list(range(10, 20))
+
+    def test_multiple_pages_last_partial(self):
+        """Final page may be shorter than per_page when total is not divisible."""
+        items, total, total_pages = _paginate_history(
+            list(range(25)), page=3, per_page=10
+        )
+        assert items == [20, 21, 22, 23, 24]
+        assert total == 25
+        assert total_pages == 3
+
+    def test_page_past_end_returns_empty(self):
+        items, total, total_pages = _paginate_history(
+            list(range(5)), page=99, per_page=10
+        )
+        assert items == []
+        assert total == 5
+        assert total_pages == 1
+
+    def test_per_page_larger_than_total(self):
+        items, total, total_pages = _paginate_history(
+            list(range(3)), page=1, per_page=100
+        )
+        assert items == [0, 1, 2]
+        assert total == 3
+        assert total_pages == 1
+
+    def test_invalid_page_raises(self):
+        with pytest.raises(ValueError, match="page must be >= 1"):
+            _paginate_history([1, 2, 3], page=0, per_page=10)
+
+    def test_invalid_per_page_raises(self):
+        with pytest.raises(ValueError, match="per_page must be >= 1"):
+            _paginate_history([1, 2, 3], page=1, per_page=0)
 
 
 @pytest.mark.unit