feat(mcp): build_so_modify_ui — diff-decorated SO modify card

[dougborg]

Closes #723

Summary

• Adds build_so_modify_ui as the per-entity SO modify/delete/correct card for the design(mcp): modify-card redesign umbrella — diff-decorated entity views per object type (#551 follow-up) #721 modify-card redesign umbrella.
• Reuses the entity-view diff helpers shared with PO modify (_render_field_diff_line / _render_party_diff_line / _render_failed_changes_block); follows BOM modify's state-driven apply-outcome morph pattern for SO's parallel sub-entity outcomes (rows / addresses / fulfillments / shipping fees).
• Sub-entity actions render with +/~/- kind gutter + per-action APPLIED/FAILED Badge on the applied path; failed actions swap to ✗ for layout-stable failure signal. The consolidated sub-entity failed-action Alert at the bottom carries the textual error + a retry-coaching tail referencing the SO id.
• Header scalar diffs driven by _SO_HEADER_FIELD_SPEC (table-driven instead of branchy code).
• Dispatched via _modification.to_tool_result on entity_type == "sales_order".

Test plan

• uv run poe agent-check — clean (ruff format/lint + ty).
• uv run poe test — 3669 passed.
• uv run poe test-browser — 35 passed (including the new so_modify_partial_failure_applied scenario).
• 22 new unit tests in TestBuildSOModifyUI mirror PO modify's coverage shape (preview, applied-success, applied-partial-failure, applied-all-failed) plus SO-specific cases (sub-entity grouping, address-update unknown-prior, customer party diff, failed sub-entity ✗ gutter + Badge).

Design notes

SO has multiple parallel-outcome sub-entities, so I followed the BOM modify card's morph pattern: build-time render paints per-action rows once, then the post-Confirm in-place morph reads from state.* (seeded from $result.state.* on success) to surface failures the build-time tree couldn't predict. Reading $result.<field> directly is broken because $result resolves to the apply tool's PrefabApp envelope, not the raw ModificationResponse — same envelope-wrap limitation documented on build_bom_modify_ui and pinned by test_apply_button_morphs_card_to_applied_state.

Sub-entity action grouping is keyed off the wire operation enum via _SO_SUBENTITY_GROUPS so future schema additions land in a sensible bucket without renderer churn.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds the sales-order-specific modify/delete/correct Prefab UI card, extending the modify-card redesign to SO workflows and wiring it into modification result dispatch.

Changes:

• Introduces build_so_modify_ui with header diffs, sub-entity sections, failed-action summaries, and apply-state state slots.
• Dispatches entity_type == "sales_order" through the new SO modify UI builder.
• Adds unit and browser coverage for SO modify preview/applied/partial-failure rendering.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
katana_mcp_server/src/katana_mcp/tools/prefab_ui.py	Adds SO modify UI builder and helper functions.
katana_mcp_server/src/katana_mcp/tools/_modification.py	Routes sales-order modification responses to the new UI.
katana_mcp_server/tests/test_prefab_ui.py	Adds direct builder tests for SO modify card behavior.
katana_mcp_server/tests/browser/test_modification_render.py	Adds browser render coverage for SO partial-failure applied state.
katana_mcp_server/tests/browser/render_test_server.py	Adds canned SO partial-failure render scenario.

[dougborg]

Review Summary

The SO modify card implements the full diff-decorated morph pattern correctly for the standalone-applied path and the preview render. The $result.state.* envelope discipline is clean, all generated files are untouched, the footer applied_verb is a mustache template (correct), and the per-section sub-entity grouping covers all 12 wire operations. One Tier-1 state-badge morph gap is architectural and shared with PO (pre-existing), but two issues need fixing before merge.

---

BLOCKING (2)

1. katana_mcp_server/tests/test_prefab_ui.py — test_address_update_renders_unknown_prior_in_summary docstring contradicts the implementation

The docstring says "MUST show (prior unknown) → new" but address_style=True in _format_so_diff_pairs deliberately strips the arrow and renders field: new (bare-after form). The test assertions only check that the values appear — they pass because of the bare form, not because of the (prior unknown) arrow. The docstring is actively misleading about the rendering contract.

Why: a future dev reading this test will believe the rendered output includes the arrow, add an assertion for (prior unknown), and be confused when it fails. Worse, a refactor that changes address_style=True to the arrow form (thinking they're keeping the documented contract) would regress silently.

Fix: Rewrite the docstring to say "address updates collapse to the bare field: new form (no arrow) because Katana has no per-address GET — address_style=True skips the is_unknown_prior path." Add a negative assertion: assert '(prior unknown)' not in rendered to pin that the arrow form does NOT appear.

---

2. katana_mcp_server/tests/browser/test_modification_render.py — SO modify browser test only exercises standalone-applied, not the preview→Confirm morph path

test_so_modify_partial_failure_applied_renders calls render_scenario("so_modify_partial_failure_applied") which directly renders the apply response with is_preview=False. This is a different code path than the iframe morph (preview card → Confirm click → on_success SetState chain → morph). The state-driven Alert (If(Rx("applied_subentity_failed_count") > 0)) is the ONLY component that actually morphs on the SO card, and it is not exercised by the current browser test.

Why: if applied_subentity_failed_count is mis-seeded (e.g. a typo in the SetState slot name) the standalone-applied test still passes because the standalone path seeds state directly in Python; the morph test would catch it. The existing test_apply_button_morphs_card_to_applied_state (BOM) sets the precedent — SO deserves the same click-through coverage.

Fix: Add a browser test scenario that renders the SO modify preview card, mocks the apply call to return _so_modify_partial_failure_response(), clicks Confirm, and asserts the morph outcome (PARTIAL FAILURE badge, failed-action Alert body, retry coaching text). Follow the existing test_apply_button_morphs_card_to_applied_state pattern.

---

SUGGESTIONS (2)

3. katana_mcp_server/src/katana_mcp/tools/prefab_ui.py — Tier-1 state badge frozen at build time on in-place morph path

_render_preview_header takes Python-time applied_state_label / applied_state_variant strings. On the in-place morph path (preview → Confirm), the header badge always reads APPLIED/green even when the outcome is PARTIAL FAILURE or FAILED, because the label was baked into the tree before the Confirm click. The applied_outcome_label / applied_outcome_variant state slots and their SetState chain are seeded but nothing binds them to the header badge. The per-field ✗ glyphs and the sub-entity failed-action Alert provide partial failure signals, but the prominent Tier-1 badge doesn't morph. This is the same limitation as PO modify (#722). File a tracking issue referencing both cards rather than deferring silently.

Fix: File a GitHub issue (Workstream: Cards, P2-soon) noting that _render_preview_header needs a state-driven badge branch (Badge(label=Rx("applied_outcome_label"), variant=Rx("applied_outcome_variant"))) to complete the morph contract.

---

4. katana_mcp_server/src/katana_mcp/tools/prefab_ui.py — _so_subentity_failed_summary walks all actions including header actions

_so_subentity_failed_summary filters for succeeded is False across the full actions list. A failed update_header action would appear here as "Failed — update_header #42: …" in the sub-entity Alert, even though header failures are already surfaced by _render_failed_changes_block. The user would see the error in two places.

Fix: Filter to sub-entity operations only. Cheapest form: _SO_SUBENTITY_OPS = frozenset(op for _, _, ops in _SO_SUBENTITY_GROUPS for op in ops) then gate on str(a.get('operation') or '').lower() in _SO_SUBENTITY_OPS.

---

NITPICKS (1)

5. katana_mcp_server/src/katana_mcp/tools/prefab_ui.py — _format_so_add_fulfillment first-truthy fallback conflates status with date

status = (_so_change_new(changes, 'status') or _so_change_new(changes, 'picked_date') or 'new fulfillment') — if status is absent but picked_date is present, the line reads status 2026-05-08T14:30:00Z (label says "status", value is a timestamp). Minor UX quirk; low priority.

---

What Looks Good

• All $result.* references are correctly $result.state.* — the envelope-wrap trap from #835 review is cleanly dodged.
• _normalize_so_prior_state correctly handles the order_no → order_number and additional_info → notes wire-shape mismatch.
• applied_verb="{{ applied_verb }}" passed as mustache to _render_preview_footer — footer body morphs correctly in lockstep with the outcome Badge.
• Sub-entity section bucketing (_SO_SUBENTITY_GROUPS) covers all 12 wire operations; fallback in _format_so_action_summary handles unknown future ops gracefully.
• _SO_HEADER_FIELD_SPEC table-driven scalar diffs are clean and easy to extend.
• 22 unit tests cover preview + applied-success + applied-partial-failure + applied-all-failed + per-sub-entity-type + morph-slot seeding. The failed Badge variant test walks the JSON envelope (not string-matching) — correct approach.
• No generated files touched, no # noqa, no # type: ignore.

🤖 Generated with Claude Code

[dougborg]

Review feedback addressed in 3fb1e870 (squashed into c349484 via autosquash). Per-finding response:

BLOCKING (1) — test_address_update_renders_unknown_prior_in_summary docstring contradicts implementation: Fixed. Renamed to test_address_update_collapses_to_bare_after_form, rewrote the docstring to describe the actual bare-after contract (no arrow, no (prior unknown)), and added two negative assertions — "(prior unknown)" not in rendered and "→" not in rendered — so the bare-after form is pinned. A future refactor that flips address_style=True to the arrow form would now regress this test.

BLOCKING (2) — SO modify browser test doesn't exercise the morph path: Fixed. Added test_so_modify_partial_failure_morphs_preview_to_applied following the test_apply_button_morphs_card_to_applied_state BOM/MO pattern:

• Refactored _so_modify_partial_failure_response() to take is_preview and emit the corresponding pre-apply / post-apply shapes (same actions, different succeeded values).
• Added a so_modify_partial_failure_preview scenario.
• Added a stub modify_sales_order MCP tool to render_test_server.py (mirrors ModifySalesOrderRequest signature so the Confirm-button payload validates; canned response uses make_tool_result so the $result.state.* Rx context matches production).
• The test renders the preview, asserts the failed-action Alert is hidden pre-Confirm, clicks Confirm, then waits for the morph-driven 404 Not Found text + modify_sales_order(id=42 retry-coaching tail to appear. A slot-name typo in either side of the SetState chain would now fail this test.

SUGGESTION (3) — Tier-1 state badge frozen at build time: Filed as #860 (P2-soon, Cards). Replied inline.

SUGGESTION (4) — _so_subentity_failed_summary over-walks actions: Fixed. Replied inline.

NITPICK (5) — _format_so_add_fulfillment mislabeled fallback: Fixed. Field-specific labels — status, picked (was the misleading "status" overload), and tracking each render with their own prefix. No more status 2026-05-08T14:30:00Z when only picked_date is supplied.

CI gates locally clean: uv run poe agent-check, full uv run poe test (3669 passed), full uv run poe test-browser (36 passed including the new morph test).

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.