chore: align with katana-openapi-client safety + MCP fixes (#60)

* chore: add pre-push hook blocking direct pushes to main from non-main branches

When a feature branch is created via ``git checkout -b <name> origin/main``,
git sets the new local branch's upstream to ``origin/main``. A subsequent
``git push -u origin <name>`` then resolves to the tracked upstream and pushes
straight to main — bypassing PR review entirely. Our sister repo
(katana-openapi-client commit 30f3fd86) hit exactly this in production: a
non-PR push to main triggered semantic-release and published an unintended
PyPI build before the pipeline could be cancelled.

Statuspro has no equivalent guard. Adds the same mechanical line of defense
katana ported in #434:

- ``scripts/pre-push-guard.sh`` — pre-commit-compatible pre-push hook that
  refuses pushes where ``remote_ref == refs/heads/main`` and ``local_ref !=
  refs/heads/main``. Suggests the safe form (``git push -u origin
  HEAD:refs/heads/<branch>``) in the rejection message.
- ``.pre-commit-config.yaml`` — adds ``default_install_hook_types:
  [pre-commit, pre-push]`` and registers ``block-push-to-main`` in the
  ``pre-push`` stage. Existing ``pre-commit install`` invocations now
  install both hook types.

Existing collaborators must re-run ``uv run pre-commit install`` once for
the pre-push hook to take effect; the README quick-start handles fresh
clones automatically.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(harness): adopt /open-pr push-refspec safety + worktree note

Pairs with the pre-push guard added in the previous commit. Documents the
push-refspec trap in two places that don't read CLAUDE.md mid-flow:

- ``.claude/skills/open-pr/SKILL.md`` CRITICAL block + Phase 5 — mandates
  ``git push -u origin HEAD:refs/heads/<branch>`` over the bare-branch form.
  The bare form resolves to the local branch's tracked upstream; if a
  branch was created via ``git checkout -b <name> origin/main`` the
  upstream is ``origin/main`` and the bare push targets main.
- ``CLAUDE.md`` Known Pitfalls — same explanation, plus a worktree note
  pointing out that pre-commit hooks (including the new pre-push guard)
  aren't shared across worktrees and require ``pre-commit install`` per
  worktree.

Lifted from katana-openapi-client's harness retro (PR #441) where the same
trap bit twice during PR development.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(mcp): mock time.perf_counter in observability timing tests

Both ``test_observe_service_timing_accuracy`` and
``test_observe_tool_timing_accuracy`` were measuring real time after
``await asyncio.sleep(...)`` and asserting the duration fell within
loose-lower-bound / tight-upper-bound brackets:

- tool variant: ``50 <= duration_ms < 200`` after a 100ms sleep
- service variant: ``50 <= duration_ms < 500`` after a 50ms sleep

The tool variant's ``< 200`` upper bound is the same flake pattern that
broke katana-openapi-client CI on Python 3.14 (235.7ms < 100ms in their
analogous service test, fixed in their #450 sweep). asyncio scheduler
+ GIL contention under CI runner load routinely pushes the measured
value well above tight upper bounds.

Both tests now mock ``time.perf_counter`` (the module-level binding the
decorator imports as ``time.perf_counter`` in ``statuspro_mcp.logging``)
with ``side_effect=[start, end]``. The computed duration is exact and
deterministic — no real sleep, no scheduler variance, exact-equality
assertion. The tests' name claims "timing accuracy" and now actually
tests that: the decorator computes ``(end - start) * 1000`` correctly.
Real asyncio scheduling latency was never the right thing to assert.

Test runtime drops from ~150ms (sleep-bound) to ~1ms (deterministic).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(mcp): use ShowToast for Cancel buttons (drop SendMessage round-trip)

Four Cancel buttons in Prefab preview UIs were sending fake user messages
back through the LLM ("Cancel the status change" / "Cancel the comment" /
"Cancel the due date change" / "Cancel the bulk update"). The model would
acknowledge in chat for what is functionally a "do nothing" action,
cluttering the conversation with a synthetic user message + LLM response.

Replaced with ``ShowToast`` (client-side only, no server trip per the
prefab_ui actions docstring): the user gets visible "Cancelled" feedback
in the iframe overlay, no chat message is appended, no LLM round-trip
happens, no tool is invoked.

Sites:
- ``build_status_change_preview_ui`` → "Status change cancelled"
- ``build_comment_preview_ui`` → "Comment cancelled"
- ``build_due_date_change_preview_ui`` → "Due date change cancelled"
- ``build_bulk_status_change_preview_ui`` → "Bulk update cancelled"

The two remaining ``SendMessage`` sites in ``prefab_ui.py`` (lines 232,
266) are intentional action triggers ("Add a comment to order...", etc.)
that prompt the LLM to follow up — those stay as is.

Ported from katana-openapi-client #440.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(mcp): coerce LLM-mistyped list inputs back into Python lists

LLMs occasionally serialize list-typed tool arguments as a single string
instead of a JSON array. Two shapes are observed in the wild:

1. CSV: ``order_ids='20486,20487,20488'``
2. JSON-stringified: ``order_ids='[20486, 20487, 20488]'``

Both make pydantic raise ``Input should be a valid list [type=list_type,
input_type=str]`` and the tool call aborts. The recovery is mechanical
and lossless — split on commas (or parse as JSON), strip whitespace,
hand pydantic a real list. So we do it.

Adds:

- ``statuspro_mcp/tools/list_coercion.py`` — a single ``coerce_str_list_input``
  BeforeValidator that handles both shapes. Lists pass through unchanged;
  non-string non-list inputs fall through to pydantic's normal type error
  so genuinely malformed input still surfaces loudly. Six type aliases
  (``CoercedStrList`` / ``CoercedIntList`` / ``CoercedStrIntList`` and
  their ``Opt`` variants) collapse the per-field
  ``Annotated[list[X] | None, BeforeValidator(coerce_str_list_input)]``
  boilerplate at call sites.
- 19 unit tests covering passthrough, CSV, JSON-array string, whitespace,
  empty inputs, non-string inputs, mixed-type lists, and the
  ``min_length`` interaction.

Applied to seven LLM-facing list parameters in ``orders.py``:

- ``list_orders``: tags, tags_any, financial_status, fulfillment_status
- ``get_orders_batch``: order_ids
- ``lookup_orders_batch``: order_numbers
- ``bulk_update_order_status``: order_ids

Internal/response-side list fields don't need it — pydantic-on-pydantic
round-trips already use real lists.

Ported from katana-openapi-client #428 (mechanical port of the alias-based
collapse already merged there).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(mcp): tighten coerce_str_list_input per /simplify pass

Two micro-simplifications surfaced by a /simplify review of the freshly-
landed list_coercion module:

- ``json.JSONDecodeError`` is a subclass of ``ValueError`` (verified at
  runtime). Drop the redundant tuple in the except clause; catch
  ``ValueError`` alone.
- The CSV fallback comprehension called ``item.strip()`` twice per item
  (once in the filter, once in the projection). Use a walrus assignment
  so the strip happens once: ``[stripped for item in s.split(",") if
  (stripped := item.strip())]``.

Behavior identical; all 19 unit tests still pass. Diverges from the
katana-openapi-client upstream by these two lines — re-sync if/when
katana picks up the same cleanup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: dougborg

.claude/skills/open-pr/SKILL.md

@@ -24,6 +24,7 @@ Ship a feature branch end-to-end: validate, self-review, push, create PR, wait f
 - **Use polling scripts for CI and review state** — never check review comments with `gh pr view --json`. That endpoint only returns top-level PR comments, not inline review comments attached to code lines. Use `poll-review.sh` which calls the correct API (`gh api repos/.../pulls/.../comments`).
 - **Never merge with unaddressed review comments** — every comment gets fixed, deferred with a tracked issue, or discussed. CI green does not override review feedback.
 - **No `--no-verify`** — never bypass commit hooks, type checkers, or linters. If a check fails, fix the cause.
+- **Push with the safe refspec, never the bare branch name** — use `git push -u origin HEAD:refs/heads/<branch-name>`, **not** `git push -u origin <branch-name>`. The bare form resolves the destination via the local branch's *upstream*. When a feature branch is created with `git checkout -b <name> origin/main`, its upstream is `origin/main` — and `git push -u origin <name>` then pushes straight to **`main`**, not to a new remote `<name>` ref. The pre-push guard at `scripts/pre-push-guard.sh` exists for this reason but does not fire from worktrees that haven't installed the repo hooks via `uv run pre-commit install`. Same rule applies to subsequent pushes (`git push --force-with-lease` after a rebase).
 
 ## STANDARD PATH
 
@@ -140,10 +141,11 @@ Note: This phase is optional and relies on manual review or the `/simplify` skil
 
 ## Phase 5: Push and create PR
 
-1. Push:
+1. Push with the explicit-destination refspec (see CRITICAL — bare-branch form
+   can push to `main` when the local branch's upstream is `origin/main`):
 
    ```bash
-   git push -u origin <branch>
+   git push -u origin HEAD:refs/heads/<branch>
    ```
 
 2. Create PR with HEREDOC body:

.pre-commit-config.yaml

@@ -7,6 +7,11 @@
 #
 # See issue #134 for background on why we use local hooks.
 
+# Make `pre-commit install` set up the pre-push hook too. Without this, the
+# pre-push hook would silently not run for contributors who only ran the bare
+# `pre-commit install` from the README. See block-push-to-main below.
+default_install_hook_types: [pre-commit, pre-push]
+
 repos:
   - repo: local
     hooks:
@@ -55,3 +60,17 @@ repos:
         language: system
         pass_filenames: false
         always_run: true
+
+      # Refuse pushes that would land non-main commits on the `main` ref.
+      # See CLAUDE.md "Known Pitfalls" — git's tracked-upstream resolution can
+      # silently route `git push -u origin <branch>` to main when the local
+      # branch was created from origin/main. Mechanical guardrail, ported
+      # from katana-openapi-client (#434) after a real release-pipeline
+      # incident there.
+      - id: block-push-to-main
+        name: block direct push to main from a non-main branch
+        entry: scripts/pre-push-guard.sh
+        language: system
+        stages: [pre-push]
+        pass_filenames: false
+        always_run: true

CLAUDE.md

@@ -6,10 +6,15 @@ Guidance for Claude Code working with this repository.
 
 ```bash
 uv sync --all-extras         # Install dependencies
-uv run pre-commit install    # Setup hooks
+uv run pre-commit install    # Setup hooks (incl. pre-push guard)
 cp .env.example .env         # Add STATUSPRO_API_KEY
 ```
 
+> **Worktree note**: pre-commit hooks aren't shared across git worktrees. Fresh
+> worktrees only have whatever hooks were installed at clone time — re-run
+> `uv run pre-commit install` after creating a new worktree, otherwise the pre-push
+> guard won't fire.
+
 ## Essential Commands
 
 | Command                  | Time   | When to Use              |
@@ -92,6 +97,13 @@ Common mistakes to avoid:
 - **None-to-UNSET conversion** — When building attrs API request models from optional
   fields, use `to_unset(value)` from `statuspro_public_api_client.domain.converters`
   instead of `value if value is not None else UNSET`.
+- **Push-refspec trap** — `git checkout -b <name> origin/main` sets the new branch's
+  upstream to `origin/main`. A subsequent `git push -u origin <name>` then resolves to
+  the tracked upstream and pushes straight to **`main`**, bypassing PR review. Always
+  use the explicit-destination form: `git push -u origin HEAD:refs/heads/<name>`. The
+  pre-push guard at `scripts/pre-push-guard.sh` catches this mistake — but only fires if
+  `pre-commit install` has run in the current worktree (hooks aren't shared across
+  worktrees). Bypassing the guard with `--no-verify` is forbidden by project policy.
 
 ## Using the LSP tool
 

scripts/pre-push-guard.sh

@@ -0,0 +1,81 @@
+#!/usr/bin/env bash
+# Pre-push guard: refuse pushes that land non-main commits on the `main` ref.
+#
+# Why this exists: when a feature branch is created via
+#   git checkout -b <name> origin/main
+# git sets the new local branch's upstream to origin/main. A subsequent
+#   git push -u origin <name>
+# resolves <name> to the tracked upstream and pushes straight to main. This
+# pattern caused a real release-pipeline incident in our sister repo
+# (katana-openapi-client commit 30f3fd86) before the guard was added there.
+#
+# pre-commit invokes this with the remote name as $1, the URL as $2, and
+# `<local-ref> <local-sha> <remote-ref> <remote-sha>` lines on stdin.
+#
+# Bypass via --no-verify is forbidden by project policy (see CLAUDE.md). The
+# right form for first-time feature-branch pushes is:
+#   git push -u origin HEAD:refs/heads/<branch-name>
+
+set -euo pipefail
+
+EXIT_CODE=0
+
+while read -r local_ref _local_sha remote_ref _remote_sha; do
+    # Skip deletions (local_ref is empty when a remote ref is being deleted).
+    [ -z "$local_ref" ] && continue
+
+    # Only guard pushes that target main.
+    case "$remote_ref" in
+        refs/heads/main) ;;
+        *) continue ;;
+    esac
+
+    # The local ref-name pushed to main must itself be `main`. Refuse anything else.
+    case "$local_ref" in
+        refs/heads/main)
+            # ok — main → main is the legitimate path for admins/release automation
+            ;;
+        refs/heads/*)
+            # Branch other than main → main: the common upstream-tracking mistake.
+            # Suggest the canonical safe form with the actual branch name.
+            local_short="${local_ref#refs/heads/}"
+            cat >&2 <<EOF
+ERROR: refusing to push '$local_short' to remote 'main'.
+
+This is almost always git's tracked-upstream resolution biting you — the
+local branch was probably created via 'git checkout -b $local_short origin/main',
+so its upstream is origin/main and a bare 'git push -u origin $local_short' targets main.
+
+The fix: use an explicit destination ref so the remote branch name matches:
+
+    git push -u origin HEAD:refs/heads/$local_short
+
+If you genuinely need to push to main from a non-main branch (rare), do it
+through a PR. Bypassing this hook with --no-verify is forbidden by project policy
+— see CLAUDE.md 'Known Pitfalls'.
+EOF
+            EXIT_CODE=1
+            ;;
+        *)
+            # Detached HEAD push, tag-to-main push, or some other unusual ref shape.
+            # Don't try to construct a branch name from a ref we don't understand —
+            # print a generic safe form instead.
+            cat >&2 <<EOF
+ERROR: refusing to push '$local_ref' to remote 'main'.
+
+The local ref isn't a normal branch (got: $local_ref). Pushes to main from
+anything other than the local 'main' branch are blocked.
+
+If you intended to push your current work to a new feature branch:
+
+    git push -u origin HEAD:refs/heads/<branch-name>
+
+Then open a PR. Bypassing this hook with --no-verify is forbidden by project
+policy — see CLAUDE.md 'Known Pitfalls'.
+EOF
+            EXIT_CODE=1
+            ;;
+    esac
+done
+
+exit "$EXIT_CODE"

statuspro_mcp_server/src/statuspro_mcp/resources/help.py

@@ -47,6 +47,20 @@
 update_order_status(…, confirm=True)  # apply
 ```
 
+## Input handling
+
+List-typed parameters (`tags`, `tags_any`, `financial_status`,
+`fulfillment_status`, `order_ids`, `order_numbers`) accept three input
+shapes — pass whichever is most natural for the call:
+
+- A real list: `["20486", "20487"]`
+- A JSON-stringified array: `'["20486", "20487"]'`
+- A comma-separated string: `"20486,20487"`
+
+The CSV and JSON-string forms are normalized to a list before validation.
+Empty / whitespace-only strings yield `[]`. Anything else (e.g. a bare
+integer for a `list[int]` field) raises a normal pydantic type error.
+
 ## Rate limits
 
 StatusPro documents rate limits per-endpoint in its OpenAPI description (not

statuspro_mcp_server/src/statuspro_mcp/tools/list_coercion.py

@@ -0,0 +1,87 @@
+"""Coerce LLM-mistyped list inputs back into Python lists.
+
+LLMs occasionally send list-typed tool arguments as a single string instead
+of a JSON array. Two shapes are observed in the wild:
+
+1. **Comma-separated values**: ``"20486,20487,20488"``
+2. **JSON-stringified array**: ``'["20486", "20487"]'``
+
+When this happens, pydantic raises ``Input should be a valid list
+[type=list_type, input_type=str]``, the tool call fails, and the user has
+to retry. The recovery is mechanical and lossless — split on commas (or
+parse as JSON), strip whitespace, hand pydantic a real list. So we do it.
+
+Usage on an LLM-facing tool parameter — prefer the prebuilt aliases::
+
+    from pydantic import Field
+    from statuspro_mcp.tools.list_coercion import CoercedStrListOpt
+
+    async def list_orders(
+        ...,
+        tags: Annotated[CoercedStrListOpt, Field(description="...")] = None,
+    ) -> ...:
+
+Required (non-Optional) variants and the heterogeneous ``str | int`` shape
+have aliases too — see below. Use the raw ``coerce_str_list_input``
+validator directly only for one-off types that don't fit the aliases.
+
+Apply only to **LLM-facing** tool parameters and request-model fields.
+Internal/response-side list fields don't need it — pydantic-on-pydantic
+round-trips already use real lists.
+
+Ported from katana-openapi-client (#428).
+"""
+
+from __future__ import annotations
+
+import json
+from typing import Annotated, Any
+
+from pydantic import BeforeValidator
+
+
+def coerce_str_list_input(value: Any) -> Any:
+    """Best-effort recovery of LLM-mistyped list arguments.
+
+    - List input → returned unchanged.
+    - String input → parsed as JSON if it looks like an array, otherwise
+      split on commas with whitespace stripped. Empty strings yield ``[]``.
+    - Anything else → returned unchanged so pydantic raises its normal
+      type error (don't mask genuinely malformed input).
+    """
+    if not isinstance(value, str):
+        return value
+
+    s = value.strip()
+    if not s:
+        return []
+
+    # JSON-stringified array: '[...]' — trust it if it parses to a list.
+    # ``json.JSONDecodeError`` is a subclass of ``ValueError``; catching the
+    # parent covers both.
+    if s.startswith("["):
+        try:
+            parsed = json.loads(s)
+        except ValueError:
+            pass
+        else:
+            if isinstance(parsed, list):
+                return parsed
+
+    # Fall back to CSV: split, strip, drop empty fragments.
+    return [stripped for item in s.split(",") if (stripped := item.strip())]
+
+
+# Type aliases — collapse the per-field
+# ``Annotated[list[X] | None, BeforeValidator(coerce_str_list_input)]`` boilerplate
+# into a single readable token at the call site. ``Opt`` suffix marks the
+# Optional/None-default variant; bare names are required (non-Optional).
+
+CoercedStrList = Annotated[list[str], BeforeValidator(coerce_str_list_input)]
+CoercedStrListOpt = Annotated[list[str] | None, BeforeValidator(coerce_str_list_input)]
+CoercedIntList = Annotated[list[int], BeforeValidator(coerce_str_list_input)]
+CoercedIntListOpt = Annotated[list[int] | None, BeforeValidator(coerce_str_list_input)]
+CoercedStrIntList = Annotated[list[str | int], BeforeValidator(coerce_str_list_input)]
+CoercedStrIntListOpt = Annotated[
+    list[str | int] | None, BeforeValidator(coerce_str_list_input)
+]

statuspro_mcp_server/src/statuspro_mcp/tools/orders.py

@@ -38,6 +38,11 @@
 from pydantic import Field
 
 from statuspro_mcp.services import get_services
+from statuspro_mcp.tools.list_coercion import (
+    CoercedIntList,
+    CoercedStrList,
+    CoercedStrListOpt,
+)
 from statuspro_mcp.tools.prefab_ui import (
     build_bulk_status_change_preview_ui,
     build_comment_preview_ui,
@@ -377,13 +382,19 @@ async def list_orders(
             str | None, Field(description="Filter to one status code")
         ] = None,
         tags: Annotated[
-            list[str] | None, Field(description="Must match all tags")
+            CoercedStrListOpt, Field(description="Must match all tags")
         ] = None,
         tags_any: Annotated[
-            list[str] | None, Field(description="Match any of these tags")
+            CoercedStrListOpt, Field(description="Match any of these tags")
+        ] = None,
+        financial_status: Annotated[
+            CoercedStrListOpt,
+            Field(description="Filter by financial status (e.g. paid, pending)"),
+        ] = None,
+        fulfillment_status: Annotated[
+            CoercedStrListOpt,
+            Field(description="Filter by fulfillment status (e.g. fulfilled, partial)"),
         ] = None,
-        financial_status: list[str] | None = None,
-        fulfillment_status: list[str] | None = None,
         exclude_cancelled: Annotated[
             bool | None, Field(description="Exclude cancelled orders")
         ] = None,
@@ -584,7 +595,7 @@ async def get_order(
     async def get_orders_batch(
         context: Context,
         order_ids: Annotated[
-            list[int],
+            CoercedIntList,
             Field(
                 description="Order ids to fetch (1-50 items).",
                 min_length=1,
@@ -635,7 +646,7 @@ async def get_orders_batch(
     async def lookup_orders_batch(
         context: Context,
         order_numbers: Annotated[
-            list[str],
+            CoercedStrList,
             Field(
                 description=(
                     "Order numbers to resolve (1-50 items). Both '20486' "
@@ -1029,7 +1040,7 @@ async def update_order_due_date(
     async def bulk_update_order_status(
         context: Context,
         order_ids: Annotated[
-            list[int],
+            CoercedIntList,
             Field(description="Order ids (1-50 items)", min_length=1, max_length=50),
         ],
         status_code: str,

statuspro_mcp_server/src/statuspro_mcp/tools/prefab_ui.py

@@ -24,6 +24,7 @@
 from datetime import UTC, datetime
 from typing import Any, Literal
 
+from prefab_ui.actions import ShowToast
 from prefab_ui.actions.mcp import CallTool, SendMessage
 from prefab_ui.app import PrefabApp
 from prefab_ui.components import (
@@ -377,7 +378,7 @@ def build_status_change_preview_ui(
             Button(
                 label="Cancel",
                 variant="outline",
-                on_click=SendMessage("Cancel the status change"),
+                on_click=ShowToast(message="Status change cancelled", variant="info"),
             )
     return app
 
@@ -436,7 +437,7 @@ def build_comment_preview_ui(preview: dict[str, Any]) -> PrefabApp:
             Button(
                 label="Cancel",
                 variant="outline",
-                on_click=SendMessage("Cancel the comment"),
+                on_click=ShowToast(message="Comment cancelled", variant="info"),
             )
     return app
 
@@ -491,7 +492,7 @@ def build_due_date_change_preview_ui(preview: dict[str, Any]) -> PrefabApp:
             Button(
                 label="Cancel",
                 variant="outline",
-                on_click=SendMessage("Cancel the due date change"),
+                on_click=ShowToast(message="Due date change cancelled", variant="info"),
             )
     return app
 
@@ -566,7 +567,7 @@ def build_bulk_status_change_preview_ui(preview: dict[str, Any]) -> PrefabApp:
             Button(
                 label="Cancel",
                 variant="outline",
-                on_click=SendMessage("Cancel the bulk update"),
+                on_click=ShowToast(message="Bulk update cancelled", variant="info"),
             )
     return app