fix(ai-tools): structured VALIDATION_ERROR for manage_* billing-domain tools (#2362) - #2370
Conversation
…n tools (#2362) manage_quotes (and its siblings manage_invoices, manage_contracts, manage_catalog) let malformed calls escape as raw HTTP 500s: the flat tool input schema marks every id/payload optional, so a missing quoteId was coerced via String(undefined) into the literal "undefined" and died downstream as an opaque uuid/DB error, and payload casts like `input.line as QuoteLineInput` let ZodError-free garbage straight into the services. - New shared apps/api/src/services/aiToolValidation.ts: per-action required-param presence checks (run BEFORE any coercion) and ZodError -> structured `{ error, code: "VALIDATION_ERROR" }` mapping with self-describing paths ("line.sourceType: ..."). - manage_quotes now parses input/patch/block/line payloads with the same shared Zod schemas the HTTP quote routes use (one source of truth), and add_catalog_line validates catalogItemId presence + guid before coercion — a partNumber-only call gets a clean error instead of a 500. - Tool descriptions now enumerate required fields per action; the line/input/block params document required + optional fields (sourceType and taxable were previously undiscoverable), and partNumber is explicitly documented as a stored override, NOT a lookup key. - Same missing-param guard + ZodError mapping swept across manage_invoices, manage_contracts, manage_catalog. - Unit tests per aiTools conventions: missing quoteId, missing line.sourceType/taxable, partNumber-only add_catalog_line, non-uuid catalogItemId, mismatched block content, plus sweep coverage for the sibling tools — each asserts the structured error, not a throw. Closes #2362 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Review run: /code-review (high effort, inline finder angles: line-by-line diff scan, removed-behavior audit, cross-file caller trace, reuse/simplification, CLAUDE.md conventions) over the full stacked diff. |
… update patch (#2361) (#2373) ## Summary **Stacked on #2370** (base branch = `fix/2362-manage-quotes-validation`) so the diff shows only the #2361 work. Merge #2370 first, then retarget/merge this one. `manage_quotes` exposed the full quote write surface (header, blocks, lines, deposits, send/decline/pay-link) with **no read tools** — inconsistent with siblings (invoices: `list_invoices`/`get_invoice`; contracts: `list_contracts`/`get_contract`). The model could not discover a quote or see its blocks/lines, and the only "read" workaround was `{action:"update", patch:{}}` — a write used as a read that bumped `updatedAt`. ## Changes - **`list_quotes`** — org/status filters, newest first, copying the `list_contracts` pattern. Filters are validated with the shared `listQuotesQuerySchema` (the same schema the `GET /quotes` route uses), so a bad filter returns a structured `VALIDATION_ERROR`. Returns `{ quotes, showing }`. - **`get_quote`** — full view: header (with derived `dueOnAcceptanceTotal`/`depositDueTotal`/`categoryBreakdown`), blocks, and lines, reusing the existing `getQuote` service the web UI reads. **No new SQL.** - **Sibling-list registration** (the parity contract test enforces all of these): - tool registry: `registerQuoteTools` (`aiToolsQuotes.ts`), tier 2 / `deviceArgs: []` like `get_contract`; - `toolInputSchemas` (`aiToolSchemas.ts`); - `TOOL_PERMISSIONS` (`aiGuardrails.ts`) → `quotes:read`. - `TOOL_TIERS` (`aiAgentSdkTools.ts`) is deliberately **not** touched: the billing-domain siblings (`list_contracts`, `get_contract`, `get_invoice`, `list_invoices`, `manage_quotes` itself) are all absent from it — that map only curates the in-app SDK agent's offered toolset; the MCP server path reads the registry tier directly. - **Empty-patch update**: `{action:"update", patch:{}}` now returns `{"error":"patch is empty — nothing to update. Use get_quote to read a quote.","code":"VALIDATION_ERROR"}` **before** any UPDATE runs, so a no-op can no longer dirty `updatedAt`. I chose the reject-with-VALIDATION_ERROR option (over silent short-circuit) because the empty patch was only ever used as a read workaround — rejecting steers the model to the new `get_quote`, and a silent success would hide caller bugs. ## Tests - `aiToolsQuotes.test.ts`: `list_quotes` default limit + filter forwarding + invalid-status `VALIDATION_ERROR`; `get_quote` full view + missing `quoteId` + `QUOTE_NOT_FOUND` mapping; empty-patch rejection asserting `updateQuote` is never called. - Contract sweeps green: `aiToolsRegistryParity` (schema + RBAC parity for every registered tool), `aiTools.deviceArgsCoverage.contract`, `aiTools.deviceAccessSiteScope.contract`, `aiGuardrails` (97 tests). - `tsc --noEmit --project apps/api/tsconfig.json` clean. Closes #2361 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Todd Hebebrand <todd@lanternops.io> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
manage_quotes(observed live in an MCP dogfood session) surfaced every malformed call as a bareHTTP 500 {"error":"Internal Server Error"}. Root causes:action), so a missingquoteIdsailed past validation, got coerced byString(undefined)into the literal string"undefined", and died downstream as an opaque uuid/DB error.input.line as QuoteLineInput) skipped Zod entirely, so schema violations either threw raw ZodErrors (escaping as 500s) or passed garbage into the services.add_manual_line's real schema (quoteLineInputSchema) requiressourceTypeandtaxable— neither was mentioned in the tool description, so the "obvious" payload always 500'd.add_catalog_lineranString(input.catalogItemId)unconditionally, so apartNumber-only call 500'd;partNumberread like a lookup key but is only a stored override.Changes
apps/api/src/services/aiToolValidation.ts— per-action required-param presence checks (run BEFORE any coercion) and ZodError → structured{"error": "<message with path>", "code": "VALIDATION_ERROR"}mapping, matching the existing service-error shape (e.g.QUOTE_NOT_FOUND).manage_quotes: presence-checks each action's required params, then parsesinput/patch/block/linepayloads with the same shared Zod schemas the HTTP quote routes use (one source of truth). ZodError paths are self-describing (line.sourceType: ...).input/line/blockdocument required + optional fields the way the depositpatchparam already did;catalogItemIdis marked REQUIRED foradd_catalog_lineandpartNumberis explicitly documented as a stored override, NOT a lookup key (kept as-is rather than promoted to a lookup key — smallest safe change; a SKU-search lookup can be a follow-up).manage_invoices,manage_contracts,manage_catalog.Tests
aiToolsQuotes.test.ts: missingquoteId/patch/input, missingline.sourceType/line.taxable(asserts both field paths in the error),partNumber-onlyadd_catalog_line, non-uuidcatalogItemId, mismatchedblockcontent, missinglineId— each asserts the structured error and that the service was NOT called.aiToolsBilling.manageInvoices.test.ts,aiToolsContracts.manageContracts.test.ts,aiToolsCatalog.manageCatalog.test.ts.tsc --noEmit --project apps/api/tsconfig.jsonclean.Closes #2362
🤖 Generated with Claude Code