diff --git a/.agents/skills/wfdash-giellie/SKILL.md b/.agents/skills/wfdash-giellie/SKILL.md index 89eeb98..127166a 100644 --- a/.agents/skills/wfdash-giellie/SKILL.md +++ b/.agents/skills/wfdash-giellie/SKILL.md @@ -290,11 +290,20 @@ REST mirror at `/api/v1/quote_items` (GET list + POST create) and `/api/v1/quote ### 8. Price-list catalogue (add items from a parsed supplier/quote document) +> **⚠️ OPT-IN ONLY (changed 2026-10-15).** `price_list.add_items` writes to the +> `price_list` catalogue, which **the POS never reads** — `listActiveStockCatalog()` +> queries `stock_items` only. A price that exists only here is invisible at the +> till. It is also now classified `confirm` in `giellie-tool-policy.ts`, so it asks +> every time regardless of the chat's trust setting. **Only call it when the +> operator explicitly asked for the price list / catalogue / supplier price list.** +> For "load prices", "add these items", "here's a supplier list", or anything +> ambiguous, use `stock_items.create` — that is the default. + | Tool | Purpose | |------|---------| -| `price_list.add_items` | Bulk-upsert parsed line items into the `price_list` collection. The agent reads the source document (supplier PDF, quote PDF, pasted email) with its native vision, extracts the per-line fields, and passes them in. The tool handles dedupe by `(sku, supplier)` and computes `sku_normalised` + `search_text` automatically. **You do the OCR, we do the commit.** | +| `price_list.add_items` | Bulk-upsert parsed line items into the `price_list` collection. The agent reads the source document (supplier PDF, quote PDF, pasted email) with its native vision, extracts the per-line fields, and passes them in. The tool handles dedupe by `(sku, supplier)` and computes `sku_normalised` + `search_text` automatically. **You do the OCR, we do the commit.** Pass the supplier **name**, never a PocketBase record id — `price_list.supplier` is free text. | -**When to use it:** the operator pastes a supplier's price list PDF, a quote from a customer, or a supplier's emailed price sheet into the chat and asks you to add the items to the catalogue. Read the document with your vision capability, extract the per-line fields, and call `price_list.add_items` with the structured array. +**When to use it:** the operator explicitly asks you to add items to the **catalogue / price list** — not merely to load prices onto the counter. Read the document with your vision capability, extract the per-line fields, and call `price_list.add_items` with the structured array. If they asked to load prices for something they sell, that is `stock_items.create` (§ POS/stock items), not this. **Input shape:** diff --git a/docs/api-mcp-layer.md b/docs/api-mcp-layer.md index fa05ecb..1ba5e65 100644 --- a/docs/api-mcp-layer.md +++ b/docs/api-mcp-layer.md @@ -531,7 +531,7 @@ Grouped by resource family. The full description is in the table column; the des | `order_items.update_status` | Update an order_item's `item_status` and/or `drv_status` fields. Use this when the operator needs to flip a single row through the workflow without bulk-pricing or per-line invoice reconciliation (`order_items.update` is the patch tool f... | | `order_items.confirm_collected` | Confirm collection of one or more customer order_items WITHOUT requiring a driver assignment. Mirrors the agent-side role that the stock-side `stock_purchase_items.mark_collected` already plays: marks `item_status` → `collected` and `drv... | | `order_items.create` | Add a new line item to an existing order. Mirrors the REST POST /api/v1/orders/{id}/items endpoint. `selling_price` auto-derives from `cost_price * (1 + markup / 100)` when omitted. Per-row failures never abort the whole call (returns `c... | -| `order_items.update` | Patch fields on an order_item for invoice reconciliation. Whitelist: cost_price, selling_price, quantity, sku, unit, category, markup, markup_percent, notes, qty_processed, qty_assigned, qty_collected, qty_verified, qty_stocked, qty_pick... | +| `order_items.update` | Patch fields on an order_item for invoice reconciliation and catalogue relabelling. Whitelist: cost_price, selling_price, quantity, sku, unit, category, description, supplier, markup, markup_percent, pricing_source, pricing_notes, matche... | | `order_items.list` | List order_items (the line items under each order). Useful for finding pending/PB-ready items, getting one supplier's items, etc. Pass PB filter syntax in `filter`. | | `order_items.get` | Fetch a single order_item by id. Pass `expand` to inline `order`, `purchase_order`, `category`, `supplier`. | @@ -655,7 +655,7 @@ Grouped by resource family. The full description is in the table column; the des | --- | --- | | `stock_items.list` | List stock_items. Supports substring search on name/sku/description, low-stock filter (`current_qty <= min_qty AND is_active = true`), pagination, and `expand` (defaults to `supplier`). | | `stock_items.get` | Fetch a single stock_item by id. The response includes the item (expand=supplier) and a paginated recent-ledger slice (`ledger.page`, `ledger.items`, etc.). Pass `ledger=false` to skip the ledger fetch. | -| `stock_items.create` | Create a new stock_item. `name` is required. When `current_qty > 0` an opening-balance ledger row is written so the running total and the ledger sum stay in sync (reason=`adjustment`, reference_type=`manual`). | +| `stock_items.create` | Create a new stock_item (a POS/till item). This is the DEFAULT place to put a product and its prices — use it unless the user explicitly asked for the price list / catalogue. The till reads `selling_price` from this collection and nothin... | | `stock_items.adjust` | Record a manual stock adjustment. Inserts a `stock_ledger` row and updates `current_qty` on the parent item. `change_qty` is signed (positive = in, negative = out); zero is rejected. `reason` must be one of STOCK_REASON_VALUES. | | `stock_items.update` | Patch catalogue fields on a master stock_item: sku, description, notes, unit, selling_price, markup. For inventory qty use stock_items.adjust. Returns {ok: true, item, previous} on success or {ok: false, reason, message, field?} on rejec... | | `stock_items.set_pricing` | Set cost price / selling price / markup on existing stock_items in one batch (max 100 per call). The POS till reads `selling_price` (NOT markup), so set both consistently: selling = cost × (1 + markup/100). Cost changes are audited in st... | @@ -695,7 +695,7 @@ Grouped by resource family. The full description is in the table column; the des | Tool | Description | | --- | --- | -| `price_list.add_items` | Bulk-upsert parsed supplier-price-list line items into the `price_list` catalogue. The agent reads the source document (a supplier PDF, a quote PDF, or a pasted supplier email) with its native vision capability, extracts the per-line fie... | +| `price_list.add_items` | ⚠️ DO NOT USE BY DEFAULT. This writes to the `price_list` catalogue, which the POS does NOT read — nothing here ever reaches the till. Only call this when the user has explicitly asked for the price list / catalogue / supplier price list... | ### Users diff --git a/src/app/api/mcp/route.test.ts b/src/app/api/mcp/route.test.ts index 677dd36..fd7431f 100644 --- a/src/app/api/mcp/route.test.ts +++ b/src/app/api/mcp/route.test.ts @@ -4074,6 +4074,92 @@ const toolNames = body.result.tools.map((t: { name: string }) => t.name); }); }); +// --------------------------------------------------------------------------- +// order_items.update — description/whitelist drift guard +// +// The dispatcher filters `patch` against ORDER_ITEM_PATCHABLE_FIELDS +// (src/lib/server/order-items-service.ts), but the tool's *prose* whitelist +// and its inputSchema are hand-written, so they can silently drift from the +// constant that actually enforces the rule. That drift already bit once: +// `description` was enforced but undocumented, which reads to an agent as +// "I cannot set the description on a line" — the exact belief that blocked +// relabelling MAS-CONSIGNMENT-FAST against the supplier price list. +// +// NOTE: the prose whitelist must stay a plain string literal. It is read by +// scripts/mcp-doc-gen.py with a naive source-text parse, so a `${...}` +// interpolation lands verbatim in docs/api-mcp-layer.md. These tests are the +// drift protection instead of interpolation. +// --------------------------------------------------------------------------- +describe("order_items.update whitelist drift guard", () => { + const EXPECTED_WHITELIST = [ + "cost_price", + "selling_price", + "quantity", + "sku", + "unit", + "category", + "description", + "supplier", + "markup", + "markup_percent", + "pricing_source", + "pricing_notes", + "matched_price_list_id", + "user_edited", + "notes", + "qty_processed", + "qty_assigned", + "qty_collected", + "qty_verified", + "qty_stocked", + "qty_picked", + "qty_loaded", + "qty_delivered", + "qty_invoiced", + "qty_cancelled", + "supplier_invoice_number", + "customer_invoice_number", + "collection_date", + "collected_date", + "verified_date", + "delivered_date", + "invoiced_date", + ]; + + async function orderItemsUpdateTool() { + const res = await POST(mcpRequest({ method: "tools/list", id: 1 })); + const body = await res.json(); + return body.result.tools.find( + (t: { name: string }) => t.name === "order_items.update", + ); + } + + it("prose whitelist lists every enforced field and nothing stale", async () => { + const tool = await orderItemsUpdateTool(); + expect(tool).toBeTruthy(); + const match = tool.description.match(/Whitelist: ([^.]+)\./); + expect(match).toBeTruthy(); + const documented = match![1] + .split(",") + .map((s: string) => s.trim()) + .filter(Boolean); + expect(documented.sort()).toEqual([...EXPECTED_WHITELIST].sort()); + }); + + it("inputSchema declares every enforced field as a patch property", async () => { + const tool = await orderItemsUpdateTool(); + const declared = Object.keys(tool.inputSchema.properties.patch.properties).sort(); + expect(declared).toEqual([...EXPECTED_WHITELIST].sort()); + }); + + it("keeps `description` and `sku` documented for price-list relabelling", async () => { + const tool = await orderItemsUpdateTool(); + expect(tool.inputSchema.properties.patch.properties.description).toBeTruthy(); + expect(tool.inputSchema.properties.patch.properties.sku).toBeTruthy(); + expect(tool.description).toMatch(/CODE - DESCRIPTION/); + }); +}); + // --------------------------------------------------------------------------- // Structured upstream errors (McpUpstreamError → JSON-RPC -32010/-32011) // --------------------------------------------------------------------------- diff --git a/src/app/api/mcp/route.ts b/src/app/api/mcp/route.ts index c2be49d..c7d7108 100644 --- a/src/app/api/mcp/route.ts +++ b/src/app/api/mcp/route.ts @@ -1045,7 +1045,7 @@ const tools = [ { name: "order_items.update", description: - "Patch fields on an order_item for invoice reconciliation. Whitelist: cost_price, selling_price, quantity, sku, unit, category, markup, markup_percent, notes, qty_processed, qty_assigned, qty_collected, qty_verified, qty_stocked, qty_picked, qty_loaded, qty_delivered, qty_invoiced, qty_cancelled, supplier_invoice_number, customer_invoice_number, collection_date, collected_date, verified_date, delivered_date, invoiced_date. Use this to compare what the supplier invoiced (supplier_invoice_number + qty_invoiced + cost_price) against what was ordered (quantity) and what was collected (qty_collected), and patch the differences. `category` is a PB relation id (resolve it from `categories.list` first, not free text). `unit` is useful when the supplier ships in a different pack/measure than the PO. Status-only updates go through `order_items.update_status` instead.", + "Patch fields on an order_item for invoice reconciliation and catalogue relabelling. Whitelist: cost_price, selling_price, quantity, sku, unit, category, description, supplier, markup, markup_percent, pricing_source, pricing_notes, matched_price_list_id, user_edited, notes, qty_processed, qty_assigned, qty_collected, qty_verified, qty_stocked, qty_picked, qty_loaded, qty_delivered, qty_invoiced, qty_cancelled, supplier_invoice_number, customer_invoice_number, collection_date, collected_date, verified_date, delivered_date, invoiced_date. Use this to compare what the supplier invoiced (supplier_invoice_number + qty_invoiced + cost_price) against what was ordered (quantity) and what was collected (qty_collected), and patch the differences. `description` + `sku` are how you relabel a line against a supplier price list (e.g. rewriting a free-text description to \"CODE - DESCRIPTION\" and setting sku to the bare CODE) — both are whitelisted. `category` and `supplier` are PB relation ids (resolve them from `categories.list` / `suppliers.list` first, not free text). `unit` is useful when the supplier ships in a different pack/measure than the PO. Status-only updates go through `order_items.update_status` instead.", inputSchema: { type: "object", required: ["id", "patch"], @@ -1059,11 +1059,17 @@ const tools = [ cost_price: { type: "number" }, selling_price: { type: "number" }, quantity: { type: "number" }, - sku: { type: "string", description: "Free-text supplier / catalogue SKU. Tried-and-true for one-letter typos; use `order_items.update_status` to flip status instead." }, + description: { type: "string", description: "Line description. Free text — callers relabelling against a price list conventionally set this to `CODE - DESCRIPTION`." }, + sku: { type: "string", description: "Free-text supplier / catalogue SKU. Tried-and-true for one-letter typos; also the machine-readable code when relabelling against a price list. Use `order_items.update_status` to flip status instead." }, unit: { type: "string", description: "Pack/measure unit (EA, BOX, M, L, KG). Useful when supplier invoice unit differs from the original PO." }, category: { type: "string", description: "PB relation id to the `categories` collection. Resolve from `categories.list` first." }, + supplier: { type: "string", description: "PB relation id to the `suppliers` collection. Resolve from `suppliers.list` first." }, markup: { type: "number" }, markup_percent: { type: "number" }, + pricing_source: { type: "string", description: "How this line was priced (e.g. manual | ai | catalogue)." }, + pricing_notes: { type: "string", description: "Free-text pricing audit note. Stamp agent identity + date when changing pricing fields." }, + matched_price_list_id: { type: "string", description: "15-char PB id of the `price_list` row this line was sourced from." }, + user_edited: { type: "boolean", description: "Marks the line as hand-edited so bulk re-pricing skips it." }, notes: { type: "string" }, qty_processed: { type: "number" }, qty_assigned: { type: "number" }, @@ -2020,7 +2026,7 @@ const tools = [ { name: "stock_items.create", description: - "Create a new stock_item. `name` is required. When `current_qty > 0` an opening-balance ledger row is written so the running total and the ledger sum stay in sync (reason=`adjustment`, reference_type=`manual`).", + "Create a new stock_item (a POS/till item). This is the DEFAULT place to put a product and its prices — use it unless the user explicitly asked for the price list / catalogue. The till reads `selling_price` from this collection and nothing else; a price that exists only in `price_list` is invisible at the counter. Pass `supplier` as the supplier's PocketBase record id (resolve it with `suppliers.list`); it is a relation, not free text. `name` is required. When `current_qty > 0` an opening-balance ledger row is written so the running total and the ledger sum stay in sync (reason=`adjustment`, reference_type=`manual`).", inputSchema: { type: "object", required: ["name"], @@ -3045,7 +3051,7 @@ const tools = [ { name: "price_list.add_items", description: - "Bulk-upsert parsed supplier-price-list line items into the `price_list` catalogue. The agent reads the source document (a supplier PDF, a quote PDF, or a pasted supplier email) with its native vision capability, extracts the per-line fields, and passes them in. The tool handles deduplication by `(sku, supplier)`: if a row already exists for that SKU from that supplier, the existing row's `cost_price`, `description`, `selling_price`, `unit`, `category`, `subcategory`, `brand`, `markup`, `stock_qty`, `search_text`, and `sku_normalised` are patched in place; otherwise a new row is inserted. `cost_price` OVERWRITES — the latest price wins. The supplier is stored as free text on `price_list.supplier` (the live schema has no FK), so the supplier name is taken verbatim. Items with missing `sku`, missing `supplier_name`, non-positive `cost_price`, or a PB write failure are returned in `skipped[]` with a per-item reason so the agent can report them back to the operator. Max 500 items per call — chunk larger lists. Optionally pass `source_file_id` (a `raw_price_list` row id, when the operator uploaded the document via `/pricelist` first) and `source_label` (free text, e.g. `\"Q-AFRISAN-2026-08\"`) so the new rows can be traced back to the source document.", + "⚠️ DO NOT USE BY DEFAULT. This writes to the `price_list` catalogue, which the POS does NOT read — nothing here ever reaches the till. Only call this when the user has explicitly asked for the price list / catalogue / supplier price list. For anything they sell across the counter, use `stock_items.create` instead. Bulk-upsert parsed supplier-price-list line items into the `price_list` catalogue. The agent reads the source document (a supplier PDF, a quote PDF, or a pasted supplier email) with its native vision capability, extracts the per-line fields, and passes them in. The tool handles deduplication by `(sku, supplier)`: if a row already exists for that SKU from that supplier, the existing row's `cost_price`, `description`, `selling_price`, `unit`, `category`, `subcategory`, `brand`, `markup`, `stock_qty`, `search_text`, and `sku_normalised` are patched in place; otherwise a new row is inserted. `cost_price` OVERWRITES — the latest price wins. The supplier is stored as free text on `price_list.supplier` (the live schema has no FK), so the supplier name is taken verbatim — pass the NAME, never a PocketBase record id. Items with missing `sku`, missing `supplier_name`, non-positive `cost_price`, or a PB write failure are returned in `skipped[]` with a per-item reason so the agent can report them back to the operator. Max 500 items per call — chunk larger lists. Optionally pass `source_file_id` (a `raw_price_list` row id, when the operator uploaded the document via `/pricelist` first) and `source_label` (free text, e.g. `\"Q-AFRISAN-2026-08\"`) so the new rows can be traced back to the source document.", inputSchema: { type: "object", required: ["items"], diff --git a/src/lib/server/giellie-system-prompt.test.ts b/src/lib/server/giellie-system-prompt.test.ts index 1e50962..46f563d 100644 --- a/src/lib/server/giellie-system-prompt.test.ts +++ b/src/lib/server/giellie-system-prompt.test.ts @@ -79,9 +79,18 @@ describe("buildSystemPrompt", () => { expect(p).toMatch(/before\/after/); }); - it("distinguishes supplier price lists from POS stock prices", () => { + // Changed 2026-10-15. Previously the prompt said "if the request is + // ambiguous, ask which one they mean" — which left the choice to the + // model, and prices kept landing in price_list instead of the POS. The + // rule is now POS-by-default with the catalogue opt-in. + it("makes the POS the default and the price_list catalogue opt-in", () => { expect(p).toContain("price_list.add_items"); - expect(p).toMatch(/ask which one/i); + expect(p).toMatch(/NEVER write to the price_list catalogue/i); + expect(p).toMatch(/unless the user has explicitly asked/i); + expect(p).toMatch(/meaning the POS \(stock_items\)/i); + // The old "ask which one" guidance is gone — asking is now the + // fallback, not the primary instruction. + expect(p).not.toMatch(/ask which one/i); }); }); }); diff --git a/src/lib/server/giellie-system-prompt.ts b/src/lib/server/giellie-system-prompt.ts index d013d89..4c61744 100644 --- a/src/lib/server/giellie-system-prompt.ts +++ b/src/lib/server/giellie-system-prompt.ts @@ -25,7 +25,7 @@ Stock items and pricing (POS): - When calling stock_items.create always pass BOTH selling_price and markup explicitly (the backend silently applies an 80% markup if they are omitted), and omit a field rather than passing 0. - When you change only markup or only cost on an existing item, read the item first (stock_items.get) so you can recompute the other figures and show the before/after. - Set prices on new items with stock_items.create (cost_price / selling_price / markup) and on existing items with stock_items.set_pricing (one batched call for a whole list, max 100 per call). Never use stock_items.adjust or purchase orders to "record" a price, and stock_items.update cannot change cost. -- Supplier price lists (price_list.add_items, the price_list catalogue) are different from POS stock prices (stock_items). If the request is ambiguous, ask which one they mean. +- NEVER write to the price_list catalogue (price_list.add_items) unless the user has explicitly asked for the price list / catalogue / supplier price list in this conversation. Treat "load prices", "add these items", "here is a supplier list" and anything ambiguous as meaning the POS (stock_items) — that is the default. Writing prices into price_list when the user meant the till is the most common mistake in this system, because the POS does not read price_list at all: those prices never reach the counter. If you are unsure, ask once before writing anything. Style: plain, concise South African business English. Lead with the answer. Use tables for lists of records. Include order / quote / PO numbers so the user can find things. Amounts as "R12,345.67".`; } diff --git a/src/lib/server/giellie-tool-policy.test.ts b/src/lib/server/giellie-tool-policy.test.ts index ef414fe..50906e8 100644 --- a/src/lib/server/giellie-tool-policy.test.ts +++ b/src/lib/server/giellie-tool-policy.test.ts @@ -60,7 +60,24 @@ describe("classifyTool", () => { it("treats ordinary mutations as write", () => { expect(classifyTool("orders.update")).toBe("write"); expect(classifyTool("quotes.create")).toBe("write"); - expect(classifyTool("price_list.add_items")).toBe("write"); + }); + + // Changed 2026-10-15: the price_list catalogue is opt-in only. It was + // `write` (auto-approved on a "trusted" chat), which is how prices kept + // landing there instead of in the POS. It now sits in CONFIRM_TOOLS, so it + // asks every time regardless of trust. + it("treats price_list.add_items as confirm — the catalogue is opt-in only", () => { + expect(classifyTool("price_list.add_items")).toBe("confirm"); + const call = classifyToolCall("price_list.add_items", { + items: [{ sku: "ABC", description: "Thing", cost_price: 10, supplier_name: "Acme" }], + }); + expect(call.cls).toBe("confirm"); + expect(call.reason).toMatch(/price_list catalogue/i); + expect(call.reason).not.toMatch(/email/i); + }); + + it("still classifies price_list.add_items as confirm even with no args to scan", () => { + expect(classifyToolCall("price_list.add_items", {}).cls).toBe("confirm"); }); it("treats stock_items.set_pricing as write (needs approval; not read, confirm or denied)", () => { diff --git a/src/lib/server/giellie-tool-policy.ts b/src/lib/server/giellie-tool-policy.ts index 6af6137..776acfd 100644 --- a/src/lib/server/giellie-tool-policy.ts +++ b/src/lib/server/giellie-tool-policy.ts @@ -35,6 +35,11 @@ export const DENIED_TOOLS: ReadonlySet = new Set([ * Ignores the trust toggle. */ export const CONFIRM_TOOLS: ReadonlySet = new Set([ + // Writes to the `price_list` catalogue, which the POS never reads — prices + // there are invisible at the till. Opt-in only (added 2026-10-15): the + // operator must ask for the price list explicitly. `confirm` ignores the + // "trusted chat" auto-approve, so this can never fire silently. + "price_list.add_items", "quotes.send_email", "quotes.retry_email", "purchase_orders.send_email", @@ -125,6 +130,12 @@ function hasContactKey(value: unknown, depth: number): boolean { } const CANCEL_REASON = "Cancels or merges records — always asks"; +const CATALOGUE_REASON = "Writes to the price_list catalogue, which the POS never reads — always asks"; + +/** Per-tool reason for the `confirm` class, for tools that are neither email nor cancel. */ +const CONFIRM_REASONS: Record = { + "price_list.add_items": CATALOGUE_REASON, +}; /** * Args-aware classification. Starts from the tool's class; a plain write whose @@ -140,7 +151,9 @@ export function classifyToolCall( if (base === "confirm") { return { cls: "confirm", - reason: /\.(cancel|combine)$/.test(name) ? CANCEL_REASON : "Sends an email", + reason: + CONFIRM_REASONS[name] ?? + (/\.(cancel|combine)$/.test(name) ? CANCEL_REASON : "Sends an email"), }; } if (base === "write" && hasContactKey(input, 0)) {