feat(mcp): build_bom_modify_ui — per-row diff-decorated BOM modify card (closes #811)

[dougborg]

Summary

Closes #811 — sixth modify-style card under the #721 umbrella. Models on the just-shipped PO modify card (#722) but uses the table-as-entity-view variant: BOM has no scalar parent fields to diff, so the card renders the BOM table itself with per-row add/update/delete decoration instead of header-field diff lines.

Before / After

Before (the #811 symptom): a 30-row manage_product_bom(add_bom_rows=[...]) preview showed 30 identical rows reading Add Bom Row | — | 3 field(s) set | PLANNED — no ingredient SKUs, no quantities, no row identity. The user had to Confirm 30 add operations blind.

After: the card renders the merged BOM table — existing rows unchanged, added rows prefixed + with ingredient SKU + quantity, updated rows show old → new decoration, deleted rows render with the deleted kind. Per-row status pills (PLANNED / APPLIED / APPLIED_VERIFIED / FAILED) replace the opaque internal ActionResult model surfacing.

Implementation

1. Impl side — _modify_product_bom_impl resolves ingredient SKU + display_name for every variant_id touched by add / update / delete sub-payloads, threading the lookup onto a new ModificationResponse.extras field (entity-agnostic dict — useful for the other 5 modify cards needing entity-specific sidecar data).

2. Builder — build_bom_modify_ui alongside build_po_modify_ui. Helper _merge_bom_rows_for_modify_card projects prior_state.rows + actions + resolved_ingredients into a unified row list with kind and per-row status_label.

3. Dispatch — _modification.to_tool_result gains an entity_type == "product_bom" branch mirroring the PO branch.

4. Tests — 32 new tests added; 3543 → 3575 total. Browser scenarios pin the cards end-to-end through the iframe.

Test plan

• uv run poe agent-check — lint + type-check pass
• uv run poe test — 3575 unit tests pass
• uv run poe check (full incl browser) — verifying in CI
• Real session: re-run a multi-row manage_product_bom preview to confirm the merged BOM table renders correctly with each kind

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds a new modify-style UI card for manage_product_bom that renders the BOM itself as a diff-decorated table (per-row add/update/delete), and threads BOM-specific sidecar data through the modification response so the UI can render ingredient identity (SKU/display name) for planned rows—especially adds.

Changes:

• Introduce ModificationResponse.extras and plumb extras through run_modify_plan() to carry entity-specific renderer data.
• Implement build_bom_modify_ui (and _merge_bom_rows_for_modify_card) to render a merged BOM table with per-row kind/status decoration.
• Add unit + browser-render tests covering BOM merge behavior, UI output contracts, and end-to-end iframe rendering scenarios.

Reviewed changes

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

Show a summary per file

File	Description
uv.lock	Bumps katana-mcp-server version to 0.93.1.
katana_mcp_server/src/katana_mcp/tools/_modification.py	Adds ModificationResponse.extras and dispatches entity_type=="product_bom" to build_bom_modify_ui.
katana_mcp_server/src/katana_mcp/tools/_modification_dispatch.py	Extends run_modify_plan(..., extras=...) and persists extras on both preview and apply responses.
katana_mcp_server/src/katana_mcp/tools/foundation/bom.py	Collects ingredient IDs touched by the plan, resolves SKU/display name once, and threads resolved_ingredients via extras.
katana_mcp_server/src/katana_mcp/tools/prefab_ui.py	Implements BOM modify card builder + row-merge helpers and renders a DataTable bound to state.plan_rows.
katana_mcp_server/tests/tools/test_bom.py	Pins that _modify_product_bom_impl carries extras.resolved_ingredients for renderer use.
katana_mcp_server/tests/test_prefab_ui.py	Adds extensive unit tests for BOM row merging and BOM modify UI rendering.
katana_mcp_server/tests/browser/render_test_server.py	Adds BOM modify preview/applied scenarios for the browser render harness.
katana_mcp_server/tests/browser/test_modification_render.py	Adds Playwright assertions validating BOM card renders all kinds and per-row statuses.

[dougborg]

Self-review (6D via code-reviewer agent)

CI green. No production-blocking issues. The merge logic is correct on all realistic inputs.

✨ What looks good

• Explicit anti-pattern guard test (test_card_does_not_leak_internal_action_labels) pinning the feedback-user-centric-card-content regression — exactly the right way to prevent the original symptom from returning.
• The +/-/~/ 2-char gutter trick for visual diff decoration without per-row styling is a clean Prefab workaround.
• resolved_ingredients string-key coercion in build_bom_modify_ui correctly anticipates the pydantic model_dump() round-trip stringing dict[int, ...] keys.
• Good helper reuse from the shared PO modify pool (_render_preview_footer, _summarize_apply_outcome, _init_modify_card_state, _render_warnings_block, _build_apply_action, _build_cancel_action).
• Coverage shape mirrors PO modify card density (32 new tests including partial-failure, all-adds-to-empty-BOM, unresolved-ingredient degradation, browser scenarios for preview + applied).

⚠️ Suggestions (worth addressing)

1. _merge_bom_rows_for_modify_card silently drops unknown operation strings — currently exhaustive (add/update/delete) but a future reorder_bom_rows op would vanish from the rendered card with no warning. Add an else: with an explicit comment, or a log line.
2. Missing unit test: update_bom_row / delete_bom_row targeting an orphan row id (not in snapshot). The orphan path is exercised in the browser fixture but not pinned at unit level. Add TestMergeBomRowsForModifyCard cases for both.
3. Missing unit test: all-deletes plan — "clear the recipe" is a real user scenario. The symmetric inverse of test_empty_prior_with_only_adds_renders_added_rows.
4. ModificationResponse.extras: dict[str, Any] deserves a typed accessor — the int(key) coercion currently leaks into the renderer. Fine for one entity; before 2-3 more children add their own extras keys, encapsulate via a typed classmethod like bom_resolved_ingredients(self) -> dict[int, dict[str, str | None]]. Filing this as a follow-up issue rather than blocking this PR — none of the 5 remaining #721 children are in flight yet.

💬 Nitpicks

• _bom_row_summary uses r["kind"] direct-access; everything else in the file uses .get(). The contract guarantees "kind" is always present (post-merge), so this works — but breaks the defensive style.

Verdict

Comment / no-block. I'll address suggestions 1, 2, 3 + the nitpick as a small fixup on this PR; suggestion 4 goes to a follow-up issue.

[dougborg]

Fixup pushed — all review findings addressed

Followed up on the deferred items from the self-review + both Copilot inline comments:

Copilot inline (both resolved in-thread):

• Docstring drift on quantity_label for deleted rows → rewritten to describe the actual behavior (snapshot quantity preserved; deletion signaled by gutter + kind).
• _BOM_MODIFY_COLUMNS comment → rewritten to accurately describe the rendering (Status column is plain text, no Badge component used in the table at all; kind discrimination via SKU-column gutter).

Self-review SUGGESTIONS:

1. Unknown-op silent drop → added explicit log warning on unmatched action operations (covers future ops like reorder_bom_rows and known ops with missing target_id).
2. Missing orphan update/delete tests → added test_update_row_with_orphan_target_id_synthesizes_placeholder + test_delete_row_with_orphan_target_id_synthesizes_placeholder.
3. Missing all-deletes test → added test_all_deletes_plan_renders_every_row_as_deleted (the "clear the recipe" scenario).
4. Typed extras accessor → filed as refactor(mcp): replace ModificationResponse.extras dict with typed per-entity accessors #834 (deferred — no design(mcp): modify-card redesign umbrella — diff-decorated entity views per object type (#551 follow-up) #721 children in flight yet to drive the typed-accessor design).

Self-review NITPICK:

• r["kind"] → .get("kind") in _bom_row_summary for consistency with the rest of the file.

Bonus: added test_unknown_operation_is_logged_and_dropped to pin the new warning behavior.

3579 unit tests pass, agent-check clean.

[Copilot]

Pull request overview

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

[dougborg]

Round 2 fixup pushed — Copilot's second-pass findings addressed

Copilot picked up two real findings on the first fixup that I missed:

1. BOM modify card header was falling back to the bare placeholder because _modify_product_bom_impl was building existing_snapshot from only rows + total_count — product_name, variant_sku, uom, katana_url were all None. The card renders "BOM for variant {id}" instead of the rich identity. Fixed: now calls _resolve_parent_for_card (same path _get_product_bom_impl uses) and populates every identity field on the snapshot.

2. Test docstring drift on test_manage_bom_response_carries_resolved_ingredients_for_renderer — claimed existing rows read SKU/display_name "via a flat resolved_ingredients lookup for uniformity"; in reality the renderer reads them directly from snapshot rows. Fixed: rewrote the docstring to accurately describe the contract (existing/deleted → snapshot dict, added/swapped → resolved_ingredients).

3579 unit tests pass, agent-check clean. Both threads resolved.

[Copilot]

Pull request overview

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

[dougborg]

Round 3 fixup pushed — rebase + apply-state morph bug fix

Rebased onto current main + addressed Copilot's third-round finding.

The bug (Copilot caught it): After clicking Confirm in the iframe, the DataTable's Status column would stay on PLANNED forever, even though the card-header/footer correctly flipped to APPLIED. Because state.plan_rows is seeded once from the preview render's table_rows and the Confirm on_success chain only sets state.result / state.applied — nothing repointed plan_rows at the resolved-status rows. The bottom "X failed" Alert (gated by is_preview=False) was also unreachable from the iframe's morphed state.

The fix: server-side precomputation per Copilot's suggestion.

• _modify_product_bom_impl runs the same _merge_bom_rows_for_modify_card + _prepare_bom_table_rows pipeline against the resolved actions on the apply path and stuffs the result into response.extras["applied_plan_rows"].
• build_bom_modify_ui adds SetState("plan_rows", "{{ $result.extras.applied_plan_rows }}") to the Confirm on_success chain via _build_apply_action(extra_on_success=[...]). When the apply response lands in $result, the DataTable rebinds to the resolved-status rows (per-row APPLIED / FAILED prefix glyph + badge in the Status column).
• New test: test_manage_bom_apply_response_carries_applied_plan_rows pins the contract.

3592 unit tests pass; agent-check clean. Branch rebased onto current main, all 3 round-3 threads resolved.

[Copilot]

Pull request overview

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

[dougborg]

Round 4 fixup — apply-state morph hardening

Copilot caught two more morph-related gaps the round-3 fix didn't cover. The DataTable rows now repointed via applied_plan_rows, but the Tier-1 state Badge and the consolidated failed-row Alert were still frozen at preview-time defaults.

Finding 1 (badge stuck on APPLIED on failure): Fixed. Server now packs applied_outcome_label / applied_outcome_variant into extras; builder fans out the badge across an If(Rx("applied_outcome_variant") == "destructive") chain (Badge.variant isn't reactive). After morph the badge correctly shows APPLIED / PARTIAL FAILURE / FAILED.

Finding 2 (failed-row Alert unreachable from morph): Fixed. Server pre-formats applied_failed_summary (one Failed — <sku>: <error> line per failed row) + applied_failed_count into extras. Builder renders the Alert under If(Rx("applied_failed_count") > 0) with state-bound title/description. The Python-time if not is_preview guard + the _render_bom_failed_rows_block helper (which built a list of AlertDescription children — not expressible from state) are removed.

Tests: extended TestBuildBOMModifyUI._applied to populate the same applied_* extras the impl produces in production, so the partial-failure assertions still pin "PARTIAL FAILURE" + the error string in the rendered tree. 3592 unit tests pass; agent-check clean.

[Copilot]

Pull request overview

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

[dougborg]

Round 5 fixup — $result.state.* references + scoped try/except

Critical Copilot finding caught the day before merge: the round-4 SetState chain used {{ $result.extras.* }} references that don't actually work — $result in the on_success Rx context is the apply tool's wire-shape PrefabApp envelope ({$prefab, view, defs, state}), NOT the raw ModificationResponse. Same documented limitation that prevents result.katana_url from working in the morph path.

Fix: flipped all 5 SetStates to {{ $result.state.* }}. At apply-build time the builder seeds state.plan_rows, state.applied_outcome_label, etc. into the apply card's own state from the response's extras, so reading off $result.state gives the apply outcome data. Added an inline comment explaining the rationale so the next person doesn't repeat the mistake.

Also: split the snapshot-building try/except so a parent-resolution failure (typed cache miss) no longer discards already-fetched BOM rows. Now the row-fetch and parent-resolution failure modes degrade independently — rows lost → snapshot=None + "could not fetch" warning; parent lost → snapshot keeps rows + header fields fall back to None (placeholder title, table + revert reference intact).

3593 unit tests pass; agent-check clean. Both threads resolved.

[Copilot]

Pull request overview

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

[dougborg]

Round 6 fixup — comment-drift cleanup + helper-extraction follow-up filed

Pure documentation/architecture cleanup this round — no behavior changes.

Comment drift (3 sites): three remaining $result.extras.* references in the round-4/5 comments hadn't caught up with the actual $result.state.* code. Rewrote each to describe the actual three-hop data flow: impl writes to response.extras → apply-time builder seeds state from extras → preview-side SetState reads $result.state.*. Added a "never $result.extras — doesn't exist on the envelope" guard rail so future readers don't repeat my round-4 mistake.

Test docstring drift: the test_manage_bom_response_carries_resolved_ingredients_for_renderer docstring claimed _collect_ingredient_ids resolves "every existing row's ingredient" including untouched ones. Actual contract: only every add's target + swap-shaped update's new ingredient + update/delete TARGET row's existing ingredient (from snapshot). Untouched-row ingredients aren't resolved — they're already inline on the snapshot. Test assertions were correct; only the prose was off.

Helper-extraction coupling: filed as #850. The "BOM actions → merged row list / outcome summary" computation is domain logic that currently lives in prefab_ui.py as private (underscore) helpers, which _modify_product_bom_impl then imports — a UI/impl boundary violation. Doing the extraction in this PR would push the diff past review-able scope (the helper set is bigger than it looks); the follow-up will move them to a new non-UI module. Added an inline note in bom.py pointing at #850 so the coupling is visible.

3593 unit tests pass; agent-check clean. All 5 round-6 threads resolved.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[dougborg]

Round 8 fixup — two real morph/apply correctness bugs

Finding 1 (fail-fast hides remaining actions): execute_plan is fail-fast — response.actions ends at the failed action; later planned entries are never attempted and were silently absent from the morphed DataTable. So a 5-action plan that failed at action 2 looked like a 2-action plan after morph; the user had no way to see what was still pending.

Fix: _modify_product_bom_impl synthesizes status_label="NOT RUN" action dicts for plan[len(response.actions):] (the never-run tail) and merges them into the rendered actions list. Added "NOT RUN": "secondary" to _BOM_ROW_STATUS_VARIANTS. The failed-row summary appends a "K planned action(s) NOT RUN — re-issue manage_product_bom..." line so the operator knows the recovery path. New test test_manage_bom_apply_synthesizes_not_run_for_fail_fast_tail pins the contract — 3-add plan, first fails, all 3 rows render (1 FAILED, 2 NOT RUN).

Finding 2 (footer verb frozen on morph): _render_preview_footer body text was f"{title_prefix} {applied_verb}." with applied_verb computed at Python build time. The Tier-1 Badge morphed to PARTIAL FAILURE/FAILED correctly, but the footer kept reading "BOM applied." — visually contradicting the badge.

Fix: the impl now packs extras["applied_verb"] ("applied" / "failed" / "partially applied") derived from outcome_label. Apply-time builder seeds state.applied_verb; preview iframe SetStates from $result.state.applied_verb. build_bom_modify_ui passes applied_verb="{{ applied_verb }}" (mustache template) to the footer so it morphs in lockstep with the Tier-1 badge.

3594 unit tests pass; agent-check clean. Both threads resolved.

[Copilot]

Pull request overview

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