Extend /card-review anti-patterns with morph-correctness lessons from #858

[dougborg]

Why

PR #858 (build_so_modify_ui) took 9 rounds of review to land. Every round caught at least one real bug, and ~all of them were morph-correctness — the same family of mistakes recurring in different corners of the (preview/apply × modify/correct × header/sub-entity × executed/NOT-RUN) grid.

The lessons are valuable for #721's remaining children (#724 MO modify, #725 stock transfer modify, #726 item modify) and any future in-place-morph card. The right home is the /card-review skill's prefab-anti-patterns.md reference, introduced by #861.

Blocked on #861 landing. Once it does, paste the sections below into .claude/skills/card-review/references/prefab-anti-patterns.md and add corresponding numbered table rows in SKILL.md's "Scan against the anti-patterns" section.

Sections to add

Numbering picks up after the existing 6 in prefab-anti-patterns.md.

---

7. $result.<x> references that aren't on the envelope

Symptom. A preview→Confirm in-place morph silently does nothing — SetState targets stay at their preview seed. Bind expressions like {{ $result.extras.applied_plan_rows }} resolve to null. The card flips applied=True but no state slot actually morphs.

Detection grep:

grep -nE '\\$result\\.[a-z]' katana_mcp_server/src/katana_mcp/tools/prefab_ui.py | grep -v '\\$result\\.state\\.'

Any hit that isn't $result.state.<slot> is suspect.

Root cause. $result in the on_success Rx context resolves to the apply tool's wire-shape structured_content — a PrefabApp envelope keyed by $prefab / view / defs / state, NOT the raw ModificationResponse. Pinned by test_apply_button_morphs_card_to_applied_state.

Fix. Seed state slots at apply-build time from response.extras.*; read off $result.state.<slot> from the preview iframe's on_success chain:

# Server-side (in the modify impl):
response.extras["applied_outcome_label"] = outcome_label
# Apply-time builder seeds:
state["applied_outcome_label"] = response.get("extras", {}).get("applied_outcome_label") or "APPLIED"
# Preview-side SetState chain:
SetState("applied_outcome_label", "{{ \$result.state.applied_outcome_label }}")

Never \$result.extras.* — that path doesn't exist on the envelope.

---

8. Build-time if is_preview: gating UI that needs to morph

Symptom. After Confirm, the card-header Badge keeps reading "PREVIEW" / footer reads "BOM applied." even on FAILED outcomes / failed-row Alert never appears / per-row badges stuck on PLANNED.

Detection grep:

grep -nE 'if is_preview:' katana_mcp_server/src/katana_mcp/tools/prefab_ui.py
grep -nE 'if not is_preview:' katana_mcp_server/src/katana_mcp/tools/prefab_ui.py

Any hit gating rendered UI that has a different preview vs applied shape is suspect.

Root cause. Python-time is_preview is captured at build time and never changes. The iframe morph flips state.applied=True but doesn't re-execute the Python tree.

Fix. Use If("applied") (or If(Rx("<slot>") > 0)) for any UI whose shape depends on the apply outcome:

# WRONG — frozen at preview render
if not is_preview:
    _render_failed_rows_block(table_rows)

# RIGHT — morphs in lockstep with state.applied
with If(Rx("applied_failed_count") > 0):
    with Alert(variant="destructive"):
        AlertTitle(content="{{ applied_failed_count }} failed row(s)")
        AlertDescription(content="{{ applied_failed_summary }}")

For Badge variants (which aren't Rx-reactive), fan out parallel If/Else branches.

---

9. execute_plan fail-fast tail silently disappears on morph

Symptom. A 5-action plan fails at action 2. After Confirm, the morphed card shows 2 actions (1 succeeded + 1 failed). Actions 3-5 simply vanish — the operator thinks the plan only had 2 entries.

Root cause. execute_plan is fail-fast — response.actions ends at the failed action. The morph overwrites preview's full row list with the apply response's SHORTER list.

Fix. Server-side: synthesize status_label="NOT RUN" action dicts for plan[len(response.actions):] (the never-run tail) and merge them in BEFORE writing the rendered row dicts into extras.applied_*. Reference impl: _modify_product_bom_impl in katana_mcp_server/src/katana_mcp/tools/foundation/bom.py.

For modify cards with multiple sub-entity sections (SO modify), route each NOT-RUN action to the right section via the operation→section map. Add a "NOT RUN": "secondary" entry to the section's status-variant dict so the neutral variant renders.

---

10. Cross-impl parity on extras: modify vs correct vs delete

Symptom. The modify impl's NOT-RUN synthesis works perfectly. A failed correct_sales_order morph still hides skipped phases — because the correction impl uses a DIFFERENT path (_run_phases_until_failure) and doesn't populate the same extras.

Detection. When any modify card handles multiple tools (modify_X + correct_X + delete_X), audit each impl independently:

grep -nE 'response\\.extras\\[' katana_mcp_server/src/katana_mcp/tools/foundation/*.py

If extras["applied_*"] populates from _modify_X_impl but not from _correct_X_impl, that's a finding.

Fix. Extract the synthesis logic to a shared helper or thread the plan tail through the correction path. Reference: _run_phases_until_failure was extended (in #858 round 6) to ALSO return the unattempted ActionSpec tail, and _correct_sales_order_impl calls the same _synthesize_correction_not_run_actions helper.

---

11. NOT-RUN actions polluting field diff maps

Symptom. A correction sequence has an EXECUTED update_header and a SKIPPED later update_header. After morph, the executed status diff disappears — the card shows the SKIPPED action's empty diff as the field state.

Root cause. _index_changes_by_field treats succeeded=None as a normal planned diff and uses last-write-wins on the field map. NOT-RUN actions, synthesized with succeeded=None, write LAST and overwrite executed entries.

Fix. Filter NOT-RUN actions out of any flattening helper BEFORE the field-map population:

def _index_changes_by_field(actions, *, include_operations=None):
    for action in actions:
        if action.get("status_label") == "NOT RUN":
            continue  # synthesized; doesn't carry real diff data
        # ... existing logic

This filter also means skipped actions need their OWN rendering surface (anti-pattern #12) — they aren't shown via the field map.

---

12. Skipped (NOT RUN) actions need a dedicated surface

Symptom. A correct_sales_order close-phase header step is skipped because phase 2 failed. The morphed card shows nothing about the skipped header step — operator can't tell whether the close action ran or not.

Root cause. Anti-pattern #11's filter is correct (NOT-RUN shouldn't pollute the diff map), but it removes the ONLY rendering path the skipped action had. Sub-entity section renderers only handle sub-entity ops, so a skipped header has zero render surface.

Fix. Add an explicit state-driven Alert (or compact section) for skipped operations:

# Server-side compute count + pre-formatted summary text.
response.extras["applied_header_skipped_count"] = n_skipped
response.extras["applied_header_skipped_summary"] = (
    "Step skipped: close the sales order (NOT RUN — earlier phase failed before this step ran)."
)
# Render under If("applied") with variant="info" (neutral; not failure).
with If(Rx("applied_header_skipped_count") > 0), Alert(variant="info"):
    AlertTitle(content="{{ applied_header_skipped_count }} step(s) skipped")
    AlertDescription(content="{{ applied_header_skipped_summary }}")

Alert(variant="secondary") is NOT valid — use info for neutral skipped state.

---

13. Build-time tool-name strings in retry coaching

Symptom. Same builder handles modify_sales_order AND correct_sales_order. A correct_sales_order partial failure renders an Alert saying "Retry the failed action(s) via modify_sales_order(...)" — wrong tool.

Detection grep:

grep -nE 'modify_[a-z_]+\\(|correct_[a-z_]+\\(|delete_[a-z_]+\\(' katana_mcp_server/src/katana_mcp/tools/prefab_ui.py | grep -i retry

Fix. Derive from confirm_tool (already threaded into build_X_modify_ui):

retry_phrase = {
    "modify_sales_order": "Retry the failed action(s) via modify_sales_order",
    "correct_sales_order": "Retry the failed correction step(s) via correct_sales_order",
}.get(confirm_tool, "Retry the failed action(s)")

---

14. Footer body text via static applied_verb

Symptom. Tier-1 Badge morphs to "PARTIAL FAILURE" / "FAILED" after Confirm, but footer body reads "BOM applied." — internally contradicting.

Detection grep:

grep -nE 'applied_verb\\s*=\\s*"' katana_mcp_server/src/katana_mcp/tools/prefab_ui.py

Root cause. _render_preview_footer interpolates a Python string into Muted(content=f"{title_prefix} {applied_verb}."). The string is frozen at build time.

Fix. Pass applied_verb as a mustache template + seed state:

# Impl computes verb from outcome_label, stuffs into extras.
response.extras["applied_verb"] = {"APPLIED": "applied", "FAILED": "failed", "PARTIAL FAILURE": "partially applied"}.get(outcome_label, "applied")
# Builder seeds state.applied_verb; on_success chain SetStates from $result.state.applied_verb.
# Pass as template to footer:
_render_preview_footer(applied_verb="{{ applied_verb }}", ...)

---

SKILL.md table additions

Add 8 rows to the "Scan against the anti-patterns" table in SKILL.md with the new numbers 7-14. Severity: 7/8/9/11/12 are BLOCKING (correctness bugs that silently mislead operators); 10 is SUGGESTION (cross-impl parity check); 13/14 are SUGGESTION (single-tool wording).

Acceptance

• Sections 7-14 added to prefab-anti-patterns.md in the existing style (Symptom / Detection / Root cause / Fix with code).
• SKILL.md table updated with corresponding rows + severity labels.
• The 8 detection grep recipes work against current prefab_ui.py.