fix(mcp): drop broken live-tick SetState; pin behavior with production-shape harness stub (#645)

Copilot review on PR #634 caught that the live-tick design
(SetState("plan_actions", Rx("$result.actions")) in the Confirm
button's on_success chain) was a no-op in production. The browser
harness was passing only because the stub modify_manufacturing_order
returned ToolResult(content="ok", structured_content=response_dict) —
putting the raw ModificationResponse into structured_content, which
is exactly the shape $result.actions needed.

Real modification tools use make_tool_result(response, ui=...) which
puts the response JSON in content and a PrefabApp wire envelope (no
actions field) in structured_content. $result in the on_success Rx
context resolves to that envelope, so $result.actions never resolves.

This commit:

- Drops the broken SetState("plan_actions", RESULT.actions) from the
  Confirm-button apply chain. Apply path now morphs the card via the
  existing applied=True flag (rows stay PLANNED; footer/badge flip to
  applied state). Same UX the codebase had before #634's live-tick
  attempt — no functional regression.
- Updates the browser harness stub to use make_tool_result exactly
  like real tools, pinning the production wire shape so future live-
  tick attempts will verify against an honest stub.
- Renames test_apply_button_pushes_result_actions_into_state to
  test_apply_button_does_not_set_state_plan_actions and asserts the
  no-op SetState is NOT in the chain — prevents regression.
- Updates the browser click-through test to assert the actual
  applied-state morph (Confirm disabled, "Applied" pill visible)
  rather than rows transitioning.
- In-code comment in build_modification_preview_ui documents why the
  live-tick was dropped and points at the follow-up issue.

Live-tick UX tracked as a separate enhancement in #645 — it requires
identifying the right Rx path (or restructuring the apply path) before
it can ship. The browser harness is now in place to verify any future
attempt.

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

Author: dougborg

katana_mcp_server/src/katana_mcp/tools/prefab_ui.py

@@ -1444,16 +1444,17 @@ def build_modification_preview_ui(
         if legacy is not None:
             actions = [legacy]
 
-    apply_action = _build_apply_action_direct(
-        confirm_tool,
-        confirm_request,
-        # Live-tick: replace the planned rows with the apply RESULT's actions
-        # before the generic ``applied=True`` flag flips. Each row's
-        # ``status_label`` updates from PLANNED to its terminal value
-        # (APPLIED / APPLIED (verified) / FAILED) and the DataTable
-        # re-renders rows in place — no iframe morph.
-        extra_on_success=[SetState(_PLAN_ACTIONS_KEY, RESULT.actions)],
-    )
+    apply_action = _build_apply_action_direct(confirm_tool, confirm_request)
+    # NOTE: A live-tick design (rows ticking PLANNED -> APPLIED in place via
+    # ``SetState("plan_actions", RESULT.actions)``) was attempted in #634 but
+    # turned out to be broken: ``$result`` in the on_success Rx context
+    # resolves to the apply tool's ``structured_content`` (a PrefabApp wire
+    # envelope from ``make_tool_result``), not to the raw
+    # ``ModificationResponse``. ``$result.actions`` therefore doesn't
+    # resolve and the SetState was a no-op in production. Caught by Copilot
+    # review; verified by the browser harness when its stub was switched to
+    # match production shape. Tracked as a follow-up — until then the
+    # apply path morphs the card via the existing ``applied=True`` flag.
     entity_type_raw = str(response.get("entity_type") or "entity")
     entity_type_label = _humanize_snake_case(entity_type_raw)
     verb_label = _verb_label(confirm_tool)

katana_mcp_server/tests/browser/render_test_server.py

@@ -17,7 +17,11 @@
 
 from fastmcp import FastMCP
 from fastmcp.tools import ToolResult
-from katana_mcp.tools._modification import ActionResult, FieldChange
+from katana_mcp.tools._modification import (
+    ActionResult,
+    FieldChange,
+    ModificationResponse,
+)
 from katana_mcp.tools.prefab_ui import (
     build_batch_recipe_update_ui,
     build_inventory_check_ui,
@@ -26,6 +30,7 @@
     build_search_results_ui,
     build_verification_ui,
 )
+from katana_mcp.tools.tool_result_utils import make_tool_result
 from prefab_ui.app import PrefabApp
 from pydantic import BaseModel
 
@@ -391,7 +396,14 @@ async def modify_manufacturing_order(
     Sub-payload args mirror :class:`ModifyManufacturingOrderRequest` so
     FastMCP's signature validator accepts the full Confirm-button payload.
     All values are ignored — the stub always returns the same canned
-    envelope, since we just need ``RESULT.actions`` to land in state.
+    envelope.
+
+    Uses ``make_tool_result`` (same helper real modification tools use) so
+    the wire shape matches production exactly: ``content`` carries the
+    response JSON, ``structured_content`` carries the apply result card's
+    Prefab envelope. This pins the contract that ``RESULT.actions`` in
+    the on_success Rx expression resolves the same way against the stub
+    as it does against the real tool.
     """
     del (
         id,
@@ -406,13 +418,14 @@ async def modify_manufacturing_order(
         update_productions,
         delete_production_ids,
     )  # unused — canned response
-    response = _twelve_action_response(
+    response_dict = _twelve_action_response(
         is_preview=preview, succeeded=None if preview else True
     )
-    return ToolResult(
-        content="ok",
-        structured_content=response,
+    response = ModificationResponse.model_validate(response_dict)
+    ui = build_modification_result_ui(
+        response_dict, tool_name="modify_manufacturing_order"
     )
+    return make_tool_result(response, ui=ui)
 
 
 if __name__ == "__main__":

katana_mcp_server/tests/browser/test_modification_render.py

@@ -71,39 +71,40 @@ def test_modify_twelve_actions_applied_renders(self, render_scenario):
         # Every action shows APPLIED (verified).
         assert frame.locator("td").filter(has_text="APPLIED").count() == 12
 
-    def test_apply_button_ticks_rows_live(self, render_scenario):
-        """Click-through live-tick: Confirm fires the apply call, the
-        on_success chain pushes RESULT.actions into state.plan_actions,
-        and rows transition from PLANNED to APPLIED in place.
+    def test_apply_button_morphs_card_to_applied_state(self, render_scenario):
+        """Click-through: Confirm fires the apply call, the on_success
+        chain flips ``state.applied=True``, and the footer/badge row
+        morphs to its applied state.
+
+        The pinned-here behavior is the conservative apply UX — the row
+        cells stay PLANNED because ``$result.actions`` doesn't resolve
+        in the on_success Rx context (see in-code comment in
+        ``build_modification_preview_ui``). A live-tick UX where rows
+        transition PLANNED → APPLIED in place is tracked as a follow-up
+        once the right Rx path is identified.
 
         The stub ``modify_manufacturing_order`` tool in
-        ``render_test_server.py`` returns a canned applied response.
-
-        Waits on the observable post-state condition (12 APPLIED cells
-        present) rather than a fixed timeout — fast on warm CI, robust
-        on slow CI, deterministic-fail if the live-tick path breaks.
+        ``render_test_server.py`` matches production wire shape via
+        ``make_tool_result``, so this test fails the same way production
+        would if the apply path regresses.
         """
         frame = render_scenario("modify_mo_12_actions_preview")
 
-        # Pre-state: 12 PLANNED, 0 APPLIED.
+        # Pre-state: 12 PLANNED, 0 APPLIED, "Applying..." badge absent,
+        # "Applied" footer pill absent.
         assert frame.locator("td").filter(has_text="PLANNED").count() == 12
-        assert frame.locator("td").filter(has_text="APPLIED").count() == 0
+        assert frame.locator("text=Applied").count() == 0
 
         # Fire the Confirm button.
         frame.locator("button").filter(has_text="Confirm").first.click()
 
-        # Wait for the live-tick to land — first APPLIED cell to become
-        # visible. ``wait_for`` polls with a 30s default timeout, so this
-        # completes ~immediately when the apply round-trip succeeds and
+        # Wait for the applied-state morph: the "Applied" pill in the
+        # button row appears once on_success fires. ``wait_for`` polls
+        # with a 30s timeout — completes fast when the apply succeeds,
         # fails deterministically if the on_success chain is broken.
-        frame.locator("td").filter(has_text="APPLIED").first.wait_for(
-            state="visible", timeout=30000
-        )
+        frame.locator("text=Applied").first.wait_for(state="visible", timeout=30000)
 
-        # Post-state: 0 PLANNED, 12 APPLIED.
-        assert frame.locator("td").filter(has_text="PLANNED").count() == 0, (
-            "Live-tick failed — Confirm click did not push RESULT.actions "
-            "into state.plan_actions, so rows stayed PLANNED. Check the "
-            "on_success chain in _build_apply_action_direct."
-        )
-        assert frame.locator("td").filter(has_text="APPLIED").count() == 12
+        # The Confirm button is now disabled (state.applied gates it via
+        # the ``locked`` Rx in ``_render_apply_button_row``).
+        confirm = frame.locator("button").filter(has_text="Confirm").first
+        assert confirm.is_disabled()

katana_mcp_server/tests/test_prefab_ui.py

@@ -2002,10 +2002,17 @@ def test_twelve_action_mixed_plan_renders_single_state_bound_table(self):
             assert r["target_label"].startswith("#")
             assert r["summary"] == "deleted"
 
-    def test_apply_button_pushes_result_actions_into_state(self):
-        """The Confirm button's CallTool on_success chain must SetState
-        ``plan_actions`` to ``$result.actions`` so each row's status_label
-        ticks live from PLANNED → APPLIED/FAILED on apply.
+    def test_apply_button_does_not_set_state_plan_actions(self):
+        """Pin: the live-tick design (``SetState("plan_actions", RESULT.actions)``)
+        was attempted in #634 but turned out to be broken — ``$result`` in
+        the on_success Rx context resolves to the apply tool's
+        ``structured_content`` (a PrefabApp wire envelope), not to the raw
+        ``ModificationResponse``. The SetState was a no-op in production.
+
+        Until the right Rx path is identified (tracked as a follow-up), the
+        on_success chain MUST NOT include a SetState targeting plan_actions
+        — a no-op SetState is misleading. The apply path morphs the card via
+        the existing ``applied=True`` flag instead.
         """
         response = _modification_preview_response(
             actions=[
@@ -2027,9 +2034,6 @@ def test_apply_button_pushes_result_actions_into_state(self):
         )
         envelope = app.to_json()
 
-        # Find the CallTool action and inspect its on_success chain.
-        # toolCall actions are nested under Button.on_click (not Component
-        # children), so search the full envelope tree.
         def find_action(o: object, action_name: str) -> dict[str, Any] | None:
             if isinstance(o, dict):
                 if o.get("action") == action_name:
@@ -2047,9 +2051,7 @@ def find_action(o: object, action_name: str) -> dict[str, Any] | None:
 
         call_tool = find_action(envelope, "toolCall")
         assert call_tool is not None
-        # The wire field is camelCase via Pydantic alias="onSuccess".
         on_success = call_tool.get("onSuccess") or call_tool.get("on_success") or []
-        # Must include a setState targeting plan_actions with $result.actions.
         plan_action_set = next(
             (
                 a
@@ -2060,15 +2062,10 @@ def find_action(o: object, action_name: str) -> dict[str, Any] | None:
             ),
             None,
         )
-        assert plan_action_set is not None, (
-            f"on_success must SetState('plan_actions', RESULT.actions) for "
-            f"live-tick; got {on_success!r}"
-        )
-        # The value should be an Rx reference resolving to $result.actions.
-        # Rx serializes via str() to "{{ key }}".
-        value = plan_action_set.get("value")
-        assert isinstance(value, str) and "$result.actions" in value, (
-            f"plan_actions value must reference $result.actions; got {value!r}"
+        assert plan_action_set is None, (
+            f"on_success must NOT SetState('plan_actions', ...) — it would "
+            f"be a no-op until the live-tick Rx path is fixed. Found: "
+            f"{plan_action_set!r}"
         )