diff --git a/docs/api-mcp-layer.md b/docs/api-mcp-layer.md index 031f86d..fa05ecb 100644 --- a/docs/api-mcp-layer.md +++ b/docs/api-mcp-layer.md @@ -324,7 +324,7 @@ Anything else returns `reason: "forbidden_field"`. For inventory qty use `stock_items.adjust`. Returns `{ok: true, item, previous}` on success or `{ok: false, reason, message, field?}` on rejection. -`stock_items.set_pricing` (required: `updates`; `actor.id` is required at runtime): +`stock_items.set_pricing` (required: `updates`; `actor.id` is required at runtime and must be a 15-char user id — it is written to `stock_ledger.created_by`, a relation to `users`; `actor.name` defaults to the id): ```json { diff --git a/src/app/api/mcp/route.test.ts b/src/app/api/mcp/route.test.ts index 271f926..677dd36 100644 --- a/src/app/api/mcp/route.test.ts +++ b/src/app/api/mcp/route.test.ts @@ -1995,10 +1995,70 @@ const toolNames = body.result.tools.map((t: { name: string }) => t.name); expect(JSON.parse(body.result.content[0].text)).toEqual(serviceResult); }); - it("defaults actor.name to an empty string", async () => { + it.each([ + ["missing", { id: actor.id }], + ["empty", { id: actor.id, name: "" }], + ["non-string", { id: actor.id, name: 5 }], + ])("defaults a %s actor.name to the actor id (no bare-space ledger note)", async (_l, a) => { updateStockItemsPricing.mockResolvedValueOnce({ results: [], summary: { total: 1, succeeded: 0, failed: 0 } }); - await call({ updates: [{ id: ID1, cost_price: 1 }], actor: { id: actor.id } }); - expect(updateStockItemsPricing.mock.calls[0][0].actor).toEqual({ id: actor.id, name: "" }); + await call({ updates: [{ id: ID1, cost_price: 1 }], actor: a }); + expect(updateStockItemsPricing.mock.calls[0][0].actor).toEqual({ id: actor.id, name: actor.id }); + }); + + it.each([ + ["hermes", "hermes"], + ["14-char", "abcde12345fghi"], + ["16-char", "abcde12345fghijk"], + ["uppercase", "ABCDE12345FGHIJ"], + ["path-shaped", "../../abcde12345"], + ])("rejects a non-user-id actor.id (%s) and never calls the service", async (_l, id) => { + const res = await call({ updates: [{ id: ID1, cost_price: 1 }], actor: { id, name: "X" } }); + const body = await res.json(); + expect(body.error.message).toBe( + "actor.id must be a valid 15-character user id (it is recorded in the cost audit trail).", + ); + expect(updateStockItemsPricing).not.toHaveBeenCalled(); + }); + + it("passes a valid 15-char actor.id through", async () => { + updateStockItemsPricing.mockResolvedValueOnce({ results: [], summary: { total: 1, succeeded: 0, failed: 0 } }); + const res = await call({ updates: [{ id: ID1, cost_price: 1 }], actor: { id: "a1b2c3d4e5f6g7h", name: "Dave" } }); + expect(res.status).toBe(200); + expect(updateStockItemsPricing.mock.calls[0][0].actor.id).toBe("a1b2c3d4e5f6g7h"); + }); + + it.each([ + ["missing", {}], + ["short", { id: "abc" }], + ["uppercase", { id: "ABCDE12345FGHIJ" }], + ["path-shaped", { id: "../x" }], + ["non-string", { id: 123456789012345 }], + ])("rejects an update whose id is %s, naming updates[i].id", async (_l, row) => { + const res = await call({ updates: [{ id: ID1, cost_price: 1 }, { ...row, cost_price: 1 }], actor }); + const body = await res.json(); + expect(body.error.message).toMatch(/updates\[1\]\.id must be a valid 15-character stock_item id/); + expect(updateStockItemsPricing).not.toHaveBeenCalled(); + }); + + it.each([ + ["null", null], + ["a numeric string", "100"], + ])("reports %s price values as per-row validation_error from the real service", async (_l, value) => { + const actual = await vi.importActual( + "@/lib/server/stock-pricing-service", + ); + updateStockItemsPricing.mockImplementationOnce((input: Parameters[0]) => + actual.updateStockItemsPricing(input), + ); + const res = await call({ updates: [{ id: ID1, cost_price: value }], actor }); + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.error).toBeUndefined(); + const data = JSON.parse(body.result.content[0].text); + expect(data.summary).toEqual({ total: 1, succeeded: 0, failed: 1 }); + expect(data.results[0]).toMatchObject({ id: ID1, ok: false, reason: "validation_error" }); + expect(data.results[0].message).toContain("costPrice"); + expect(update).not.toHaveBeenCalled(); }); it.each([ @@ -2140,6 +2200,13 @@ const toolNames = body.result.tools.map((t: { name: string }) => t.name); expect(data.selling_price).toBeUndefined(); }); + it("documents current behaviour: an explicit markup of 0 with a positive cost is treated as absent, so the 80% default is applied", async () => { + await callCreate({ name: "Widget", cost_price: 100, markup: 0 }); + const data = create.mock.calls[0][0]; + expect(data.markup).toBe(80); + expect(data.selling_price).toBe(180); + }); + it("respects a caller-supplied selling price (no default override)", async () => { await callCreate({ name: "Widget", cost_price: 100, selling_price: 125 }); const data = create.mock.calls[0][0]; diff --git a/src/app/api/mcp/route.ts b/src/app/api/mcp/route.ts index 73ae201..c2be49d 100644 --- a/src/app/api/mcp/route.ts +++ b/src/app/api/mcp/route.ts @@ -2123,7 +2123,7 @@ const tools = [ }, actor: { type: "object", - description: "Who is making the change (recorded in the cost audit trail). `id` is required.", + description: "Who is making the change (recorded in the cost audit trail). `id` is required and must be the 15-char id of a WFDASH user (see users.lookup); `name` defaults to the id.", properties: { id: { type: "string" }, name: { type: "string" }, @@ -6301,6 +6301,7 @@ async function handleMcpPost(req: NextRequest) { if (toolName === "stock_items.set_pricing") { const MAX_PRICING_UPDATES = 100; + const PB_ID_RE = /^[a-z0-9]{15}$/; const ALLOWED_PRICING_KEYS = ["id", "cost_price", "selling_price", "markup"]; if (!Array.isArray(args.updates) || args.updates.length === 0) { throw new Error("updates must be a non-empty array for stock_items.set_pricing."); @@ -6316,6 +6317,13 @@ async function handleMcpPost(req: NextRequest) { "actor.id is required for stock_items.set_pricing (it is recorded in the cost audit trail).", ); } + // stock_ledger.created_by is a relation to `users`: a non-user id would + // make the ledger write fail AFTER the cost was already patched. + if (!PB_ID_RE.test(actorRaw.id)) { + throw new Error( + "actor.id must be a valid 15-character user id (it is recorded in the cost audit trail).", + ); + } const pricingUpdates = (args.updates as unknown[]).map((raw, i) => { if (!raw || typeof raw !== "object" || Array.isArray(raw)) { throw new Error(`updates[${i}] must be an object.`); @@ -6328,8 +6336,11 @@ async function handleMcpPost(req: NextRequest) { ); } } + if (typeof row.id !== "string" || !PB_ID_RE.test(row.id)) { + throw new Error(`updates[${i}].id must be a valid 15-character stock_item id.`); + } const out: { id: string; costPrice?: number; sellingPrice?: number; markup?: number } = { - id: row.id as string, + id: row.id, }; if (row.cost_price !== undefined) out.costPrice = row.cost_price as number; if (row.selling_price !== undefined) out.sellingPrice = row.selling_price as number; @@ -6344,7 +6355,10 @@ async function handleMcpPost(req: NextRequest) { updates: pricingUpdates, actor: { id: actorRaw.id, - name: typeof actorRaw.name === "string" ? actorRaw.name : "", + name: + typeof actorRaw.name === "string" && actorRaw.name.trim() + ? actorRaw.name + : actorRaw.id, }, }); return jsonRpcOk(body.id, result); diff --git a/src/lib/server/giellie-system-prompt.test.ts b/src/lib/server/giellie-system-prompt.test.ts index 2373987..1e50962 100644 --- a/src/lib/server/giellie-system-prompt.test.ts +++ b/src/lib/server/giellie-system-prompt.test.ts @@ -67,6 +67,18 @@ describe("buildSystemPrompt", () => { expect(p).toMatch(/stock_items\.update cannot change cost/i); }); + it("tells the model to pass both selling_price and markup on create, omitting rather than passing 0", () => { + expect(p).toMatch(/always pass BOTH selling_price and markup/); + expect(p).toMatch(/silently applies an 80% markup/); + expect(p).toMatch(/omit a field rather than passing 0/); + }); + + it("tells the model to read the item first when changing only markup or only cost", () => { + expect(p).toMatch(/only markup or only cost/); + expect(p).toContain("stock_items.get"); + expect(p).toMatch(/before\/after/); + }); + it("distinguishes supplier price lists from POS stock prices", () => { expect(p).toContain("price_list.add_items"); expect(p).toMatch(/ask which one/i); diff --git a/src/lib/server/giellie-system-prompt.ts b/src/lib/server/giellie-system-prompt.ts index ffb7515..d013d89 100644 --- a/src/lib/server/giellie-system-prompt.ts +++ b/src/lib/server/giellie-system-prompt.ts @@ -22,6 +22,8 @@ How to work: Stock items and pricing (POS): - Before adding a stock item (or items from a list, photo or spreadsheet), search for an existing match first (stock_items.list with search by SKU and by name) so you do not create duplicates. Collect: name, SKU (if any), unit, qty on hand / min / target, supplier, and pricing: cost price, plus markup % or a selling price. If the cost or the markup / selling price is missing, ASK for it. You may suggest the app's default markup of 80% as an option, but never apply a markup the user did not give or confirm. - The POS till reads selling_price, not markup, so always set both consistently. selling_price = cost × (1 + markup/100), rounded to 2 decimals. If the user gives cost and selling price instead, markup = (selling ÷ cost − 1) × 100, to 1 decimal. Show the working for each price. +- 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.