feat(mcp): create_sales_order accepts shipping fees inline (closes #818)

[dougborg]

Summary

Closes #818 — create_sales_order now accepts shipping_fees inline so ecommerce-order workflows no longer need the create-then-modify two-step (currently every fee-bearing SO is two MCP calls + two preview/apply gates).

Wire-level reality

Katana's POST /sales_orders does NOT accept fees inline at the wire. The MCP wrapper collapses the conceptual one-step into a "primary SO POST + chained POST /sales_order_shipping_fee per fee" pipeline. This is NOT a Katana API change — it's MCP-wrapper sugar to give the agent a single logical action with one preview/apply card.

Atomicity choice

Best-effort (per pre-approved plan). SO success is durable; if a fee POST fails, the response carries:

• shipping_fee_outcomes[i].succeeded = False + error field
• A (non-BLOCK) warning telling the operator how to retry via modify_sales_order(add_shipping_fees=[...])

Rollback was considered and rejected — a failed DELETE /sales_orders/{id} cleanup would leave a worse orphan than a partial create.

Test plan

• uv run poe agent-check — lint + type-check pass
• uv run poe test — 3563 unit tests pass
• uv run poe check (full incl browser) — verifying in CI
• Real session: re-run the eBay-order skill that motivated this issue to confirm a single create_sales_order(shipping_fees=[...]) call replaces the previous two-step

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds inline shipping_fees support to the MCP create_sales_order tool, collapsing the common “create SO then modify to add fees” workflow into a single preview/apply card while respecting Katana’s wire-level limitation (fees still require separate POSTs after the SO is created).

Changes:

• Extended CreateSalesOrderRequest / SalesOrderResponse to accept and report per-fee shipping_fee_outcomes, with best-effort apply semantics.
• Implemented chained POST /sales_order_shipping_fee calls after SO creation and surfaced partial-failure warnings + next-action retry coaching.
• Updated Prefab UI rendering and added unit + browser tests covering preview, all-success apply, and partial-failure apply states.

Reviewed changes

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

Show a summary per file

File	Description
uv.lock	Bumps editable package version for katana-mcp-server.
katana_mcp_server/src/katana_mcp/tools/foundation/sales_orders.py	Adds request/response models and create-then-add-fees apply pipeline with per-fee outcomes.
katana_mcp_server/src/katana_mcp/tools/prefab_ui.py	Renders a “Shipping fees” subsection on the SO create card (preview + applied outcomes).
katana_mcp_server/src/katana_mcp/resources/help.py	Documents the new shipping_fees input and shipping_fee_outcomes output behavior.
katana_mcp_server/tests/tools/test_sales_orders.py	Adds tool-level tests for preview/apply success, partial failure, and BLOCKing invalid amounts.
katana_mcp_server/tests/test_prefab_ui.py	Adds unit tests pinning SO-create shipping-fees section rendering across states.
katana_mcp_server/tests/browser/render_test_server.py	Adds canned UI scenarios for browser rendering tests.
katana_mcp_server/tests/browser/test_other_cards_render.py	Adds browser tests to ensure the new card states render correctly.

[dougborg]

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

CI green. All findings addressed in the fixup just pushed. See thread replies for the inline-comment specifics.

✨ What Looks Good

• unwrap_as used correctly for both the SO POST and each fee POST; per-fee try/except doesn't swallow SO errors.
• to_unset / unset_dict used consistently for the wire request construction; no hand-rolled if value is not None else UNSET patterns.
• Retry coaching in next_actions and the Alert includes the literal so.id — operators can copy-paste the exact modify_sales_order call.
• Browser render tests cover all three fee states (preview / all-success / partial-failure) and unit tests assert the exact shipping_fee_outcomes shape.
• SOShippingFeeAdd / SOShippingFeeUpdate hoisted to module-top so the modify_sales_order path imports them without a forward reference.
• Help resource updated in the same commit with wire-level semantics + retry path — no drift surface.

🚫 BLOCKING (addressed in fixup)

Apply path lacked BLOCK-validation on shipping_fees amounts. An agent calling preview=False directly with a malformed amount would leave a created SO + zero applied fees on the books (no rollback on best-effort). Now validates before the SO POST and returns a BLOCK response with id=None if amounts don't parse — pinned by test_create_sales_order_apply_blocks_on_invalid_amount.

⚠️ SUGGESTIONS (addressed in fixup)

1. total_amount float drift → switched to Decimal accumulation for the fee total (matches the codebase convention for monetary sums); still passes float(total) to _format_money.
2. Missing all-fees-fail test → added test_create_sales_order_apply_all_shipping_fees_fail covering SO success + every fee failing (0/N applied, SO id preserved, retry hint emitted).
3. Asymmetric "PENDING" / "NOT_SHIPPED" fixture status → flipped the ternary to read in impl order + added an inline comment so future readers don't "fix" what isn't broken.

💬 NITPICKS (addressed in fixup)

1. is_preview or succeeded is None conflation → split into two explicit branches (if is_preview: then if succeeded is None:) with a comment marking the second as defensive (the apply pipeline always resolves succeeded before returning).
2. ShippingFeeOutcome mutability → added a one-line note to _outcomes_from_planned_fees docstring explaining the intentional omission of ConfigDict(frozen=True).

Verdict

Comment / no-block. Both Copilot inline findings + all 6 code-reviewer agent findings are addressed in the fixup. 3565 unit tests pass; agent-check clean.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[dougborg]

Round 2 fixup pushed — apply-path BLOCK render + partial-total fixes

Rebased onto current main + addressed Copilot's second-round findings.

Finding 1 (apply-path BLOCK rendered as CREATED): The new apply-path validation returned is_preview=False with id=None, but build_so_create_ui seeds state.applied=True whenever is_preview is false — so the card flipped to the CREATED rendering despite nothing being created. Fixed: return is_preview=True instead. Now state.applied=False, the card renders as a preview, and the BLOCK warnings disable the Confirm button — visually matches the actual outcome. Message reworded to "Fix the amounts and re-issue with preview=false." Test updated.

Finding 2 (misleading partial total): "Total shipping" rendered whenever any fee amount parsed but only summed the parseable subset — so a 2-of-3-parseable plan would show "Total shipping: $X · 3 fee(s)" implying the plan totaled $X. Fixed: gate on parseable_amount_count == len(outcomes). When BLOCK warnings are present, the total is skipped entirely; the BLOCK warnings still surface each unparseable row separately.

3577 unit tests pass; agent-check clean. Branch rebased onto current main, all 4 round-2 threads resolved.

[Copilot]

Pull request overview

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

[dougborg]

Round 3 fixup — Decimal end-to-end through _format_money

Copilot caught that the round-2 Decimal accumulation was being undone by float() conversions at the _format_money call sites — defeating the exact-decimal guarantee for both per-row labels and the total.

Fix: lifted from decimal import Decimal, InvalidOperation to module-top, extended _format_money(amount: float | int | Decimal | None, ...), and routed each parsed amount_dec straight to both the per-row label and the running total. Babel's format_currency accepts Decimal directly (preferred for monetary amounts), so the rendered per-row text and the running sum now share the same exact decimal value end-to-end.

3577 unit tests pass; agent-check clean. Both round-3 threads resolved.

[Copilot]

Pull request overview

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

[dougborg]

Round 4 fixup — non-finite Decimal guard + state-driven apply-outcome morph

Three Copilot findings addressed:

1. Docstring drift in apply-block test — fixed to match is_preview=True reality + explain why.

2. Decimal("NaN") / Decimal("Infinity") were accepted — Python's Decimal() constructor accepts these as valid (they only fail on arithmetic). Fixed: _validate_shipping_fee_amounts now explicitly rejects non-finite values via parsed.is_finite() with a distinct "not a finite decimal" BLOCK warning, plus a defensive InvalidOperation catch around the negativity check. New test test_create_sales_order_preview_nan_infinity_amounts_blocked pins all three cases (NaN, Infinity, -Infinity).

3. Shipping-fees section frozen at preview time → morph doesn't reflect outcome — applied the same state-driven pattern from feat(mcp): build_bom_modify_ui — per-row diff-decorated BOM modify card (closes #811) #833's BOM modify card. New helper _so_shipping_fees_apply_state precomputes outcome summary slots from shipping_fee_outcomes. build_so_create_ui seeds these into state at apply-time, and the preview iframe's on_success chain wires SetStates from $result.state.*. Render: an If("applied")-gated block surfaces a summary line + failed-fee Alert with retry coaching, which now correctly morphs after Confirm.

Acknowledged limitation: the per-row APPLIED/FAILED badges inside _render_so_shipping_fees_section still don't morph — each row is painted at Python time, and Prefab doesn't expose state-bound row arrays for arbitrary list rendering (same Alert-children limitation we hit on BOM). The summary + failed-fee details DO morph correctly, which covers the "did this work / what failed" question. A fully-correct row-level morph would need either a DataTable-style abstraction or per-index state slots; both bigger refactors than this PR's scope.

3579 unit tests pass; agent-check clean. All 3 threads resolved.

[Copilot]

Pull request overview

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

[dougborg]

Round 5 fixup — defensive NaN/Infinity guard in renderer

One Copilot finding addressed: _render_so_shipping_fees_section was treating Decimal("NaN") and Decimal("Infinity") as parseable (they parse without raising), polluting total_amount (NaN + Decimal → NaN) and risking a format_currency failure on the final total formatting.

Fix: added a defensive parsed.is_finite() check after the parse. Non-finite Decimals now fall through to the raw-string label and are EXCLUDED from both total_amount and parseable_amount_count. Comment block calls out the belt + suspenders rationale — the validator already BLOCKs these on the request side, but this guards the case where an agent bypasses preview.

3579 unit tests pass; agent-check clean. Thread resolved.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[dougborg]

Round 7 fixup — retry coaching now survives the morph

Real UX bug Copilot caught: the build-time Alert in _render_so_shipping_fees_section carried the "Retry the failed fees via modify_sales_order(...)" coaching, but the build-time Alert doesn't morph after Confirm. The state-driven Alert (added in round 4) was rendering only the failed-fee list — no recovery instructions. Operators hitting a partial failure would see what failed but not how to fix it.

Fix: _so_shipping_fees_apply_state now takes so_id and, when there are failures, appends a retry-coaching line to applied_fees_failed_summary:

Failed — Standard shipping: 422 invalid
Failed — Handling: 500 timeout

Retry the failed fee(s) via modify_sales_order(id=9001, add_shipping_fees=[...]).
Each failed row above preserves its description / amount / tax_rate_id —
copy them straight in.

build_so_create_ui passes response.get("id") so the SO id is embedded literally in the morphed text — operator can copy/paste straight into the modify call. The morph now carries both the failure detail AND the recovery instructions.

3594 unit tests pass; agent-check clean. Thread resolved.

[Copilot]

Pull request overview

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

[dougborg]

Round 8 fixup — misleading-total bug + retry-string sweep

Two real findings + one consistency sweep:

Finding 1 (misleading total on partial failure): the bottom-line "Total shipping" summed every parseable fee — including failed ones. On partial failure that meant displaying "$30 total · 3 fee(s)" when only $20 actually attached to the SO. Operators would think they invoiced more than they did.

Fix: track two Decimal totals (planned_total for preview/all-success, succeeded_total for partial-failure). Render rules:

• Preview: Total shipping: $X · N fee(s)
• All-success apply: Total shipping: $X · N fee(s) applied
• Partial failure: Total shipping applied: $Y · M of N fee(s) applied (labeled "applied" so it can't be confused with the requested total)
• All-failed: total line skipped entirely (nothing landed on the SO)

New test pins all-failed; partial-failure test extended to assert the "Total shipping applied" label + counter + absence of the misleading planned total.

Findings 2 & 3 (missing id=<so_id> in retry coaching): modify_sales_order(add_shipping_fees=[...]) is missing the required id= param — copy/paste from the docs would fail. Swept all 8 occurrences: field description, ShippingFeeOutcome.succeeded description, in-line fee_warnings message, post-fee-block code comment, _render_so_shipping_fees_section docstring, help.py, two test docstrings. All consistent now.

Rebased onto current main (incorporated #833's prefab_ui.py changes that landed in 170fba69 + the v0.96.0 release). 3647 unit tests pass; agent-check clean. All 3 threads resolved.