fix(mcp): address Copilot review on #949

- _same_bin_row_warnings also flags both-None rows — unassigned →
  unassigned moves nothing, same no-op as an identical bin id on both
  sides.
- The modify card's Location header line renders resolved names
  ('Main warehouse (id=1)') instead of a bare id, labelled 'Location'.
  Names resolve from the typed cache into extras['resolved_locations']
  on every plan shape (the line renders on header-only cards too), and
  a location move resolves both the old and new location.
- idempotentHint stays True for consistency with the PO/SO/MO modify
  siblings that also carry add_rows; the repo-wide audit is #950.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: dougborg

katana_mcp_server/src/katana_mcp/tools/foundation/bin_transfers.py

@@ -372,18 +372,21 @@ def _build_nested_rows(
 
 
 def _same_bin_row_warnings(rows: list[BinTransferRowInput]) -> list[str]:
-    """Warn about rows whose source and target bin are the same (no-op move)."""
+    """Warn about rows whose source and target bin are the same (no-op move).
+
+    Both-``None`` counts too — unassigned → unassigned moves nothing, just
+    like an identical bin id on both sides.
+    """
     offenders = [
         str(idx)
         for idx, row in enumerate(rows, start=1)
-        if row.source_bin_location_id is not None
-        and row.source_bin_location_id == row.target_bin_location_id
+        if row.source_bin_location_id == row.target_bin_location_id
     ]
     if not offenders:
         return []
     return [
-        f"Row(s) {', '.join(offenders)} have the same source and target bin — "
-        "the move would be a no-op for those rows."
+        f"Row(s) {', '.join(offenders)} have the same source and target bin "
+        "(or neither set) — the move would be a no-op for those rows."
     ]
 
 
@@ -1179,6 +1182,41 @@ async def _resolve_bins_for_card(
     return {b.id: b.bin_name for b in bins}
 
 
+async def _resolve_location_names_for_card(
+    services: Any,
+    existing: BinTransfer | None,
+    request: ModifyBinTransferRequest,
+) -> dict[int, str]:
+    """Resolve ``{location_id: name}`` for the modify card's Location line.
+
+    Covers the transfer's current location and, on a location move, the
+    header patch's new one — so the card renders names ("Main warehouse"),
+    not bare ids. Cache misses just drop out of the map; the card falls
+    back to the raw id.
+    """
+    from katana_public_api_client.models_pydantic._generated import CachedLocation
+
+    location_ids: set[int] = set()
+    if existing is not None:
+        location_ids.add(int(existing.location_id))
+    if request.update_header is not None and request.update_header.location_id:
+        location_ids.add(int(request.update_header.location_id))
+    if not location_ids:
+        return {}
+
+    resolved: dict[int, str] = {}
+    for location_id in location_ids:
+        name, _ = await resolve_entity_name(
+            services.typed_cache.catalog,
+            CachedLocation,
+            location_id,
+            entity_label="Location",
+        )
+        if name:
+            resolved[location_id] = name
+    return resolved
+
+
 async def _modify_bin_transfer_impl(
     request: ModifyBinTransferRequest, context: Context
 ) -> ModificationResponse:
@@ -1274,7 +1312,7 @@ async def _modify_bin_transfer_impl(
         request.add_rows or request.update_rows or request.delete_row_ids
     )
     if has_row_crud:
-        resolved_variants, resolved_bins = await asyncio.gather(
+        resolved_variants, resolved_bins, resolved_locations = await asyncio.gather(
             _resolve_bin_row_variants(
                 services, _collect_bin_row_variant_ids(existing, request)
             ),
@@ -1287,9 +1325,15 @@ async def _modify_bin_transfer_impl(
                     else None
                 ),
             ),
+            _resolve_location_names_for_card(services, existing, request),
         )
     else:
         resolved_variants, resolved_bins = {}, {}
+        # The Location header line renders on every card shape, so its
+        # name resolution isn't gated on row CRUD.
+        resolved_locations = await _resolve_location_names_for_card(
+            services, existing, request
+        )
 
     response = await run_modify_plan(
         request=request,
@@ -1306,6 +1350,7 @@ async def _modify_bin_transfer_impl(
         extras={
             "resolved_variants": resolved_variants,
             "resolved_bins": resolved_bins,
+            "resolved_locations": resolved_locations,
         },
         cache_merge=CacheMerge(
             cache=services.typed_cache,

katana_mcp_server/src/katana_mcp/tools/prefab_ui.py

@@ -8109,7 +8109,7 @@ def build_stock_transfer_modify_ui(
 # renders as the "Status" line.
 _BT_HEADER_FIELDS: tuple[tuple[str, str], ...] = (
     ("bin_transfer_number", "Transfer No"),
-    ("location_id", "Location ID"),
+    ("location_id", "Location"),
     ("created_date", "Created Date"),
     ("departed_at", "Departed At"),
     ("arrived_at", "Arrived At"),
@@ -8118,10 +8118,23 @@ def build_stock_transfer_modify_ui(
 )
 
 
+def _bt_location_display(value: Any, names: dict[int, str]) -> Any:
+    """Render a location id as ``Name (id=N)`` when resolvable, else as-is."""
+    if value is None:
+        return None
+    try:
+        key = int(value)
+    except (TypeError, ValueError):
+        return value
+    name = names.get(key)
+    return f"{name} (id={key})" if name else value
+
+
 def _render_bin_transfer_entity_view(
     bt: dict[str, Any],
     *,
     changes: dict[str, FieldChangeView] | None = None,
+    location_names: dict[int, str] | None = None,
 ) -> list[str]:
     """Render the bin-transfer entity view (header scalar diffs + warnings),
     returning the block-warning list for the caller to gate the Confirm button.
@@ -8130,17 +8143,30 @@ def _render_bin_transfer_entity_view(
     field is changing, else its plain value when present. Bin transfers have a
     GET-by-id endpoint, so unlike stock transfers the plain branch fires for
     unchanged header fields — the prior snapshot provides real context around
-    the diff lines.
+    the diff lines. The Location line resolves ids to names via
+    ``location_names`` (``extras["resolved_locations"]``) so the operator can
+    confirm without cross-referencing Katana; misses fall back to the raw id.
 
     Must be called inside ``with CardContent(), Column(gap=3):``.
     """
     changes = changes or {}
+    names = location_names or {}
     for field, label in _BT_HEADER_FIELDS:
         change = changes.get(field)
+        value = bt.get(field)
+        if field == "location_id":
+            if change is not None:
+                change = change.model_copy(
+                    update={
+                        "before": _bt_location_display(change.before, names),
+                        "after": _bt_location_display(change.after, names),
+                    }
+                )
+            value = _bt_location_display(value, names)
         if change is not None and change.kind != "unchanged":
             _render_field_diff_line(label, change=change)
-        elif bt.get(field) is not None:
-            _render_field_diff_line(label, value=bt.get(field))
+        elif value is not None:
+            _render_field_diff_line(label, value=value)
 
     _render_failed_changes_block(changes, field_label_overrides=dict(_BT_HEADER_FIELDS))
     return _render_warnings_block(bt.get("warnings"))
@@ -8243,10 +8269,13 @@ def build_bin_transfer_modify_ui(
         actions, include_operations=frozenset({"update_header", "update_status"})
     )
 
+    extras = response.get("extras") or {}
     bin_row_rows, bin_row_summary = _bin_modify_row_rows(
-        raw_prior_state, actions, extras=response.get("extras") or {}
+        raw_prior_state, actions, extras=extras
     )
     show_row_table = bool(bin_row_summary)
+    # Same wire shape as resolved_bins ({id: name}) — reuse the coercer.
+    location_names = _coerce_resolved_bins(extras.get("resolved_locations"))
 
     number_change = changes_by_field.get("bin_transfer_number")
     header_number = (
@@ -8301,7 +8330,7 @@ def build_bin_transfer_modify_ui(
             if response.get("message"):
                 Muted(content=response["message"])
             block_warnings = _render_bin_transfer_entity_view(
-                entity, changes=changes_by_field
+                entity, changes=changes_by_field, location_names=location_names
             )
             if show_row_table:
                 Separator()

katana_mcp_server/tests/browser/render_test_server.py

@@ -1429,6 +1429,7 @@ def _bin_transfer_modify_response(*, is_preview: bool, succeeded: bool | None) -
                 403: {"sku": "WASHER", "display_name": "M5 washer"},
             },
             "resolved_bins": {7: "A-01", 8: "B-02", 9: "C-03"},
+            "resolved_locations": {1: "Main warehouse"},
         },
         "warnings": [],
         "next_actions": [],

katana_mcp_server/tests/browser/test_modification_render.py

@@ -216,6 +216,8 @@ def test_bin_transfer_modify_preview_renders_header_and_row_table(
         # Real prior — header diff reads before → after, not (prior unknown).
         assert frame.locator("text=Transfer No: BT-001 → BT-002").count() >= 1
         assert frame.locator("text=Status: CREATED → IN_TRANSIT").count() >= 1
+        # Location renders the resolved name, not the bare id.
+        assert frame.locator("text=Location: Main warehouse (id=1)").count() >= 1
         # Row diff table with summary line.
         assert frame.locator("text=Line items:").count() >= 1
         assert frame.locator("text=+1 added, ~1 updated, -1 deleted").count() >= 1

katana_mcp_server/tests/tools/test_bin_transfers.py

@@ -177,7 +177,9 @@ async def test_create_bin_transfer_confirm_success():
 
 @pytest.mark.asyncio
 async def test_create_bin_transfer_same_bin_rows_warn():
-    """Rows with source == target bin raise an operator-facing warning."""
+    """Rows with source == target bin — including both unset (unassigned →
+    unassigned) — raise an operator-facing warning; rows that actually move
+    stock don't."""
     context, _ = create_mock_context()
 
     request = CreateBinTransferRequest(
@@ -189,15 +191,21 @@ async def test_create_bin_transfer_same_bin_rows_warn():
                 source_bin_location_id=7,
                 target_bin_location_id=7,
             ),
-            BinTransferRowInput(variant_id=101, quantity=1),
+            BinTransferRowInput(variant_id=101, quantity=1),  # both None: no-op
+            BinTransferRowInput(
+                variant_id=102,
+                quantity=1,
+                source_bin_location_id=7,
+                target_bin_location_id=9,
+            ),
         ],
         preview=True,
     )
     result = await _create_bin_transfer_impl(request, context)
 
     same_bin = [w for w in result.warnings if "same source and target bin" in w]
     assert len(same_bin) == 1
-    assert "Row(s) 1" in same_bin[0]
+    assert "Row(s) 1, 2" in same_bin[0]
 
 
 @pytest.mark.asyncio
@@ -543,9 +551,11 @@ async def test_modify_bt_plan_canonical_order_status_last():
         "delete_row",
         "update_status",
     ]
-    # Row CRUD plans thread the resolved lookups for the card's row table.
+    # Row CRUD plans thread the resolved lookups for the card's row table,
+    # and every plan shape threads location names for the header line.
     assert "resolved_variants" in response.extras
     assert "resolved_bins" in response.extras
+    assert "resolved_locations" in response.extras
 
 
 @pytest.mark.asyncio