refactor(mcp): extract BOM table-merge helpers into shared bom_table module

[dougborg]

Closes #850.

Summary

• New module katana_mcp/tools/foundation/bom_table.py (pure domain, no Prefab imports) holds the BOM table-merge math.
• foundation/bom.py no longer imports underscore helpers from prefab_ui — the UI/impl coupling Copilot flagged on feat(mcp): build_bom_modify_ui — per-row diff-decorated BOM modify card (closes #811) #833 is gone.
• prefab_ui.build_bom_modify_ui imports the same helpers from the new module; rendering glue stays in prefab_ui.py.

What moved

• _merge_bom_rows_for_modify_card and its row-shapers (_bom_existing_row_from_snapshot, _bom_synth_orphan_row, _bom_add_action_to_row, _apply_bom_update_to_row, _apply_bom_delete_to_row, _bom_change_lookup)
• _prepare_bom_table_rows
• _format_bom_quantity / _format_bom_quantity_diff
• _BOM_ROW_STATUS_VARIANTS + _BomMergedRowKind
• _summarize_apply_outcome and _derive_status_label (dict version)

What stayed in prefab_ui.py

• build_bom_modify_ui + all rendering glue (public render contract).
• _bom_row_summary — purely presentational summary line.
• _BOM_MODIFY_COLUMNS + _resolve_bom_table_rows — UI-bound wiring.

Judgment calls

• _summarize_apply_outcome: Cross-entity-generic, but only directly called by foundation/bom and prefab_ui.build_po_modify_ui. Housed in bom_table.py because that breaks the bom→prefab_ui underscore import without adding a second new module. If a third caller appears later, hoist to a dedicated shared-modify-helpers module — pragmatic call now to keep the diff focused.
• _derive_status_label (dict version): Used by the BOM merge AND by _action_to_row in prefab_ui. Moved with the BOM merge so the new module is self-contained; prefab_ui imports it back. The ActionResult-typed version in _modification.py is untouched.

Test plan

• uv run poe agent-check and uv run poe test (3647 passed, 7 skipped) both clean.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR refactors the BOM modify-card “table merge” logic out of the UI layer (prefab_ui.py) into a new shared, non-UI domain module (foundation/bom_table.py) so both the rendering path and the tool-implementation path can reuse the same row-shaping helpers without foundation/bom.py importing UI internals.

Changes:

• Added katana_mcp.tools.foundation.bom_table to host BOM table-merge + status/outcome bucketing helpers with no Prefab/UI imports.
• Updated prefab_ui.py to import the extracted helpers and removed the in-file implementations.
• Updated foundation/bom.py (apply path) and test_prefab_ui.py to reference the new module (including logger name for the warning test).

Reviewed changes

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

File	Description
katana_mcp_server/src/katana_mcp/tools/foundation/bom_table.py	New shared, non-UI module containing BOM row-merge + formatting + status/outcome helpers.
katana_mcp_server/src/katana_mcp/tools/prefab_ui.py	Removes BOM merge helpers from UI module; imports them from foundation.bom_table and keeps only rendering glue.
katana_mcp_server/src/katana_mcp/tools/foundation/bom.py	Switches apply-time helper imports from prefab_ui to foundation.bom_table to eliminate UI/impl coupling.
katana_mcp_server/tests/test_prefab_ui.py	Updates caplog expectations to the new logger namespace emitted by the extracted module.

[dougborg]

Review Summary

Clean structural refactor that achieves its stated goal: BOM table-merge math is now in a self-contained domain module with no Prefab imports, and foundation/bom.py no longer reaches into prefab_ui for underscore helpers. All 258 tests survive unchanged. The behavior-fidelity check passes — every moved function is a verbatim copy. No generated files were touched.

Three issues found, none blocking.

---

BLOCKING (0)

None.

---

SUGGESTIONS (2)

1. katana_mcp_server/tests/test_prefab_ui.py:2264 and :3369-3372 — stale test imports of moved symbols still go through prefab_ui

These two call-sites import _summarize_apply_outcome, _merge_bom_rows_for_modify_card, _prepare_bom_table_rows from katana_mcp.tools.prefab_ui rather than from their new canonical home katana_mcp.tools.foundation.bom_table. They pass today only because prefab_ui.py incidentally re-exports all four symbols at module level (lines 111-115). If those top-level imports are ever narrowed to local/function scope (a routine refactoring step), the two test sites break silently without any indication the test was testing the wrong module.

The top-level test import at line 18 (from katana_mcp.tools.prefab_ui import _merge_bom_rows_for_modify_card) is used throughout TestMergeBomRowsForModifyCard — that class should be importing from bom_table given that the function now lives there.

Fix: update lines 2264 and 3369-3372 to from katana_mcp.tools.foundation.bom_table import .... Also update the top-level module import at line 18 to remove _merge_bom_rows_for_modify_card from the prefab_ui import block and import it from bom_table instead (either at the top of the file or at the test-class level).

2. katana_mcp_server/src/katana_mcp/tools/foundation/bom_table.py:82 — _BomMergedRowKind is defined but never referenced as a type annotation

The Literal type alias is moved over faithfully from prefab_ui.py but is unused within the new module: the row-shaper functions never annotate their kind writes with it (they use plain string literals). If ruff's F841 / PYI018 passes over this it may produce noise in future lint runs, and a reader scanning the module will reasonably wonder if it was meant to annotate something.

Fix: either annotate the "kind" dict values (e.g. declare a helper return type that includes kind: _BomMergedRowKind), or remove the dead alias and add it back when it is actually enforcing a type boundary.

---

NITPICKS (1)

3. katana_mcp_server/src/katana_mcp/tools/foundation/bom_table.py:110-142 — _summarize_apply_outcome is cross-entity generic but housed in a BOM-specific module

The PR notes call this out and make a pragmatic deferred-work call. That reasoning is sound for now. To prevent the deferred work from disappearing, recommend filing a GitHub issue (or adding a # TODO(#NNN) comment inline) so the "hoist to shared-modify-helpers if a third caller appears" intent is tracked rather than staying only in the PR description.

---

What Looks Good

• The new module has zero Prefab imports — verified with grep -E 'from prefab_ui|PrefabApp|Badge\b'. Goal achieved.
• All seven moved functions are verbatim copies of their originals — no dropped None-guards, no missing or defaults, no behavioral drift on any path.
• The caplog logger-name assertion in TestMergeBomRowsForModifyCard.test_unknown_operation_is_logged_and_dropped was correctly updated to katana_mcp.tools.foundation.bom_table (both the at_level guard and the rec.name filter).
• The two _derive_status_label variants remain distinct: the dict-typed one is in bom_table, the ActionResult-typed one stays in _modification.py — no collapse.
• foundation/bom.py lazy-import correctly re-targets bom_table with no residual prefab_ui underscore imports.
• Test count is unchanged (258 → 258); no tests were silently deleted in the rename.

[dougborg]

Addressed all three review findings in d3f9957:

Suggestion 1 — stale test imports of moved symbols (test_prefab_ui.py:18, 2264, 3369-3372): all four call-sites now import _merge_bom_rows_for_modify_card, _prepare_bom_table_rows, and _summarize_apply_outcome directly from katana_mcp.tools.foundation.bom_table. The top-level import block at line 18 was split so _merge_bom_rows_for_modify_card (used by TestMergeBomRowsForModifyCard) reads from its canonical home; the two local imports inside test bodies were retargeted likewise. Tests no longer depend on the prefab_ui incidental re-export.

Suggestion 2 — unused _BomMergedRowKind Literal alias (bom_table.py:82): the alias is now exercised as a type annotation on the kind local in each of the five row-shaper helpers (_bom_existing_row_from_snapshot, _bom_synth_orphan_row, _bom_add_action_to_row, _apply_bom_update_to_row, _apply_bom_delete_to_row). Pyright now enforces that any future kind write outside the existing|added|updated|deleted vocabulary is a type error — the alias carries the documentary intent the review flagged AND enforces the boundary.

Nitpick 3 — _summarize_apply_outcome cross-entity in BOM-named module: filed #859 to track the eventual hoist into a dedicated foundation/modify_outcome.py when a third caller appears (SO/MO modify cards or a generic modify-result renderer). Inline TODO(#859) comment added to the _summarize_apply_outcome docstring so the deferred work isn't only captured in this PR's description.

Verification: uv run poe agent-check and uv run poe test (3647 passed, 7 skipped) both clean.