From 38cd015e286b7d79bf95a300e3093947d085b2cc Mon Sep 17 00:00:00 2001 From: Dave Date: Thu, 5 Nov 2026 23:53:54 +0200 Subject: [PATCH] fix(mcp): a stock quantity write needs an acting user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batch ran with an empty actor.id when the caller sent none, so the ledger row went out unattributed — against the plan's own rule that the acting user is stamped on every ledger row. The tool now requires a real 15-char user id, exactly as stock_items.set_pricing does, and declares the actor in its schema so a schema-following client can supply one. The other fixes follow from the same review: - A numeric SKU is a SKU, so it is coerced to its string form; a boolean or an object is malformed input and is refused rather than dropped silently. - The duplicate-SKU error names both spellings and the lines they are on, so the operator can see which two rows collided. - dry_run also accepts the string "true". It is the one flag that decides whether data is written at all. - The description now says the batch is rejected as a whole on a repeated SKU, and that quantities are truncated to whole units. --- src/app/api/mcp/route.test.ts | 254 ++++++++++++++++++++++++++++------ src/app/api/mcp/route.ts | 94 ++++++++++--- 2 files changed, 290 insertions(+), 58 deletions(-) diff --git a/src/app/api/mcp/route.test.ts b/src/app/api/mcp/route.test.ts index cddc749..34c9317 100644 --- a/src/app/api/mcp/route.test.ts +++ b/src/app/api/mcp/route.test.ts @@ -144,6 +144,14 @@ vi.mock("@/lib/server/price-list-service", () => ({ addPriceListItems: (...args: unknown[]) => addPriceListItemsMock(...args), })); +// Mock the bulk quantity engine for stock_items.set_quantities, so the +// route's own contract (what it validates, and what it forwards) can be +// asserted without a PocketBase behind it. +const setStockQuantitiesBulkMock = vi.fn(); +vi.mock("@/lib/server/stock-quantities-bulk-service", () => ({ + setStockQuantitiesBulk: (...args: unknown[]) => setStockQuantitiesBulkMock(...args), +})); + import { GET, POST } from "./route"; function mcpRequest(body: unknown) { @@ -4294,10 +4302,44 @@ describe("MCP structured upstream errors", () => { // stock_items.set_quantities — the batch stock-quantity write // --------------------------------------------------------------------------- describe("MCP tool stock_items.set_quantities", () => { + const ACTOR = { id: "user123456789ab", name: "Dave" }; + beforeEach(() => { process.env.AGENT_API_KEY = "t"; + setStockQuantitiesBulkMock.mockReset(); + setStockQuantitiesBulkMock.mockResolvedValue({ + ok: true, + mode: "set", + dry_run: false, + summary: { + total: 1, + applied: 1, + unchanged: 0, + unmatched: 0, + ambiguous: 0, + failed: 0, + }, + results: [], + }); }); + const call = (args: Record) => + POST( + new NextRequest("http://localhost/api/mcp", { + method: "POST", + headers: { + authorization: `Bearer ${process.env.AGENT_API_KEY}`, + "content-type": "application/json", + }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { name: "stock_items.set_quantities", arguments: args }, + }), + }), + ); + it("is declared in the tool catalogue", async () => { const res = await GET( new NextRequest("http://localhost/api/mcp", { @@ -4308,55 +4350,187 @@ describe("MCP tool stock_items.set_quantities", () => { expect(body.tools.some((t) => t.name === "stock_items.set_quantities")).toBe(true); }); - it("rejects a batch with a duplicate SKU", async () => { - const res = await POST( + it("documents the whole-batch duplicate rule and the whole-unit truncation", async () => { + const res = await GET( new NextRequest("http://localhost/api/mcp", { - method: "POST", - headers: { - authorization: `Bearer ${process.env.AGENT_API_KEY}`, - "content-type": "application/json", - }, - body: JSON.stringify({ - jsonrpc: "2.0", - id: 1, - method: "tools/call", - params: { - name: "stock_items.set_quantities", - arguments: { - mode: "set", - lines: [ - { sku: "ABC-1", qty: 1 }, - { sku: "abc 1", qty: 2 }, - ], - }, - }, - }), + headers: { authorization: `Bearer ${process.env.AGENT_API_KEY}` }, }), ); + const body = (await res.json()) as { + tools: Array<{ + name: string; + description: string; + inputSchema: { properties: Record }; + }>; + }; + const tool = body.tools.find((t) => t.name === "stock_items.set_quantities"); + expect(tool).toBeTruthy(); + // The model has to learn these from the description alone. + expect(tool!.description).toMatch(/duplicate|repeated|only once/i); + expect(tool!.description).toMatch(/whole units|truncat/i); + // The actor is part of the contract, so it is declared. + expect(tool!.inputSchema.properties.actor).toBeTruthy(); + }); + + it("forwards the batch to the engine: mode, dry_run, actor and truncated qty", async () => { + const res = await call({ + mode: "set", + dry_run: true, + lines: [ + { sku: "ABC-1", qty: 12.7 }, + { sku: "XYZ-2", qty: -4 }, + ], + actor: ACTOR, + }); + + expect(res.status).toBe(200); + expect(setStockQuantitiesBulkMock).toHaveBeenCalledTimes(1); + const arg = setStockQuantitiesBulkMock.mock.calls[0][0]; + expect(arg.mode).toBe("set"); + expect(arg.dryRun).toBe(true); + expect(arg.actor).toEqual({ id: ACTOR.id, name: "Dave" }); + expect(arg.pb).toBeTruthy(); + // Whole units: 12.7 is truncated to 12. A negative quantity is + // passed through unchanged — the till oversells on purpose. + expect(arg.lines).toEqual([ + { sku: "ABC-1", description: undefined, qty: 12 }, + { sku: "XYZ-2", description: undefined, qty: -4 }, + ]); + }); + + it("passes a negative quantity through and truncates a fraction without complaint", async () => { + await call({ + mode: "add", + lines: [{ sku: "ABC-1", qty: -3.9 }], + actor: ACTOR, + }); + expect(setStockQuantitiesBulkMock).toHaveBeenCalledTimes(1); + expect(setStockQuantitiesBulkMock.mock.calls[0][0].lines[0].qty).toBe(-3); + }); + + it("treats the string \"true\" as a dry run, so it cannot write by accident", async () => { + await call({ + mode: "set", + dry_run: "true", + lines: [{ sku: "ABC-1", qty: 1 }], + actor: ACTOR, + }); + expect(setStockQuantitiesBulkMock).toHaveBeenCalledTimes(1); + expect(setStockQuantitiesBulkMock.mock.calls[0][0].dryRun).toBe(true); + }); + + it("rejects a batch with a duplicate SKU and never calls the engine", async () => { + const res = await call({ + mode: "set", + lines: [ + { sku: "ABC-1", qty: 1 }, + { sku: "abc 1", qty: 2 }, + ], + actor: ACTOR, + }); const body = await res.json(); expect(JSON.stringify(body)).toMatch(/duplicate SKU/i); + // Both spellings and where they are, so the operator can see which + // two rows collided. + expect(body.error.message).toContain('"ABC-1"'); + expect(body.error.message).toContain('"abc 1"'); + expect(body.error.message).toMatch(/line 1/); + expect(body.error.message).toMatch(/line 2/); + // A malformed paste must not half-apply. + expect(setStockQuantitiesBulkMock).not.toHaveBeenCalled(); }); it("rejects an unknown mode", async () => { - const res = await POST( - new NextRequest("http://localhost/api/mcp", { - method: "POST", - headers: { - authorization: `Bearer ${process.env.AGENT_API_KEY}`, - "content-type": "application/json", - }, - body: JSON.stringify({ - jsonrpc: "2.0", - id: 1, - method: "tools/call", - params: { - name: "stock_items.set_quantities", - arguments: { mode: "sideways", lines: [{ sku: "ABC-1", qty: 1 }] }, - }, - }), - }), - ); + const res = await call({ + mode: "sideways", + lines: [{ sku: "ABC-1", qty: 1 }], + actor: ACTOR, + }); const body = await res.json(); expect(JSON.stringify(body)).toMatch(/mode must be/i); + expect(setStockQuantitiesBulkMock).not.toHaveBeenCalled(); + }); + + it("rejects a batch with no acting user, so no ledger row goes out unattributed", async () => { + const res = await call({ + mode: "set", + lines: [{ sku: "ABC-1", qty: 1 }], + }); + const body = await res.json(); + expect(body.error.message).toMatch(/actor\.id is required/i); + expect(setStockQuantitiesBulkMock).not.toHaveBeenCalled(); + }); + + it.each([ + ["14-char", "abcde12345fghi"], + ["16-char", "abcde12345fghijk"], + ["uppercase", "ABCDE12345FGHIJ"], + ["path-shaped", "../../abcde12345"], + ])("rejects a non-user-id actor.id (%s) and never calls the engine", async (_l, id) => { + const res = await call({ + mode: "set", + lines: [{ sku: "ABC-1", qty: 1 }], + actor: { id, name: "Dave" }, + }); + const body = await res.json(); + expect(body.error.message).toMatch(/15-character user id/i); + expect(setStockQuantitiesBulkMock).not.toHaveBeenCalled(); + }); + + it("defaults a missing actor.name to the actor id", async () => { + await call({ + mode: "set", + lines: [{ sku: "ABC-1", qty: 1 }], + actor: { id: ACTOR.id }, + }); + expect(setStockQuantitiesBulkMock.mock.calls[0][0].actor).toEqual({ + id: ACTOR.id, + name: ACTOR.id, + }); + }); + + it("accepts a numeric SKU as its string form", async () => { + await call({ + mode: "set", + lines: [{ sku: 12345, qty: 1 }], + actor: ACTOR, + }); + expect(setStockQuantitiesBulkMock).toHaveBeenCalledTimes(1); + expect(setStockQuantitiesBulkMock.mock.calls[0][0].lines[0].sku).toBe("12345"); + }); + + it.each([ + ["a boolean", true], + ["an object", { code: "ABC-1" }], + ])("rejects %s SKU instead of dropping it silently", async (_l, sku) => { + const res = await call({ + mode: "set", + lines: [{ sku, qty: 1 }], + actor: ACTOR, + }); + const body = await res.json(); + expect(body.error.message).toMatch(/sku must be a string or a number/i); + expect(setStockQuantitiesBulkMock).not.toHaveBeenCalled(); + }); + + it("accepts a numeric description as its string form", async () => { + await call({ + mode: "set", + lines: [{ description: 4321, qty: 1 }], + actor: ACTOR, + }); + expect(setStockQuantitiesBulkMock).toHaveBeenCalledTimes(1); + expect(setStockQuantitiesBulkMock.mock.calls[0][0].lines[0].description).toBe("4321"); + }); + + it("rejects a malformed description instead of dropping it silently", async () => { + const res = await call({ + mode: "set", + lines: [{ description: { text: "Widget" }, qty: 1 }], + actor: ACTOR, + }); + const body = await res.json(); + expect(body.error.message).toMatch(/description must be a string or a number/i); + expect(setStockQuantitiesBulkMock).not.toHaveBeenCalled(); }); }); diff --git a/src/app/api/mcp/route.ts b/src/app/api/mcp/route.ts index 8e7be28..00c824e 100644 --- a/src/app/api/mcp/route.ts +++ b/src/app/api/mcp/route.ts @@ -2210,6 +2210,7 @@ const tools = [ "Set or change the stock quantity on one or more POS/stock items, matched by SKU and description. " + 'Use mode "set" when the number is the new quantity on hand (the stocktake case), and mode "add" when it is a change to add to the current quantity. ' + "Each line is resolved by exact SKU first, then by a description match (flagged as such). A line that cannot be resolved is reported, never guessed. " + + "Quantities are truncated to whole units. One SKU may appear only once in a batch — a repeated SKU rejects the whole batch, so split the paste. " + "Pass dry_run true first to preview the before/after plan. Quantities may be negative — the till oversells on purpose and the stocktake corrects it. " + "Writes a stock_ledger row against the acting user and mirrors the new quantity onto the price-list catalogue.", inputSchema: { @@ -2249,6 +2250,15 @@ const tools = [ description: "Plan the change without writing anything. Returns the same result shape with the same before/after numbers.", }, + actor: { + type: "object", + description: + "Who is making the change (recorded on every stock_ledger row). `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" }, + }, + }, }, required: ["mode", "lines"], }, @@ -6535,6 +6545,7 @@ async function handleMcpPost(req: NextRequest) { } if (toolName === "stock_items.set_quantities") { + const PB_ID_RE = /^[a-z0-9]{15}$/; const mode = String(args.mode ?? ""); if (mode !== "set" && mode !== "add") { throw new Error('mode must be "set" or "add"'); @@ -6545,15 +6556,28 @@ async function handleMcpPost(req: NextRequest) { throw new Error("lines must contain between 1 and 100 entries"); } - const lines = rawLines.map((raw) => { - const l = (raw ?? {}) as { sku?: unknown; description?: unknown; qty?: unknown }; + const lines = rawLines.map((raw, i) => { + if (!raw || typeof raw !== "object" || Array.isArray(raw)) { + throw new Error(`lines[${i}] must be an object.`); + } + const l = raw as { sku?: unknown; description?: unknown; qty?: unknown }; + // A numeric SKU is a SKU — a paste out of a spreadsheet arrives + // as a number. Anything else (a boolean, an object) is + // malformed input, not a missing SKU: dropping it silently + // would turn the line into a description-only line and report + // it as unmatched, hiding the real mistake. The description + // follows the same rule, so the two text fields behave alike. + const asText = (value: unknown, field: string): string | undefined => { + if (typeof value === "string") return value; + if (typeof value === "number" && Number.isFinite(value)) return String(value); + if (value === undefined || value === null) return undefined; + throw new Error(`lines[${i}].${field} must be a string or a number.`); + }; + const sku = asText(l.sku, "sku"); + const description = asText(l.description, "description"); const qty = typeof l.qty === "number" && Number.isFinite(l.qty) ? Math.trunc(l.qty) : NaN; - return { - sku: typeof l.sku === "string" ? l.sku : undefined, - description: typeof l.description === "string" ? l.description : undefined, - qty, - }; + return { sku, description, qty }; }); if (lines.some((l) => !Number.isFinite(l.qty))) { @@ -6562,28 +6586,62 @@ async function handleMcpPost(req: NextRequest) { // One SKU is one stock row, so two lines for the same SKU cannot // both be true. That is a malformed paste, not a per-row outcome. - const seen = new Set(); - const duplicates = new Set(); - for (const l of lines) { + // The message names both spellings and where they are, so the + // operator can see which two rows collided. + const seen = new Map(); + const duplicates: string[] = []; + for (let i = 0; i < lines.length; i++) { + const l = lines[i]; const key = normaliseSku(l.sku ?? ""); if (!key) continue; - if (seen.has(key)) duplicates.add(l.sku ?? key); - seen.add(key); + const first = seen.get(key); + if (first) { + duplicates.push( + `"${first.raw}" (line ${first.line + 1}) and "${l.sku}" (line ${i + 1})`, + ); + } else { + seen.set(key, { raw: l.sku ?? key, line: i }); + } } - if (duplicates.size > 0) { - throw new Error(`duplicate SKU in one batch: ${[...duplicates].join(", ")}`); + if (duplicates.length > 0) { + throw new Error( + `duplicate SKU in one batch: ${duplicates.join("; ")}. Each SKU may appear only once.`, + ); } - const actorRaw = (args.actor ?? {}) as { id?: string; name?: string }; + // The acting user is stamped on every ledger row, so a batch + // without one is refused rather than written unattributed. + const actorRaw = (args.actor ?? {}) as { id?: unknown; name?: unknown }; + if (typeof actorRaw.id !== "string" || !actorRaw.id.trim()) { + throw new Error( + "actor.id is required for stock_items.set_quantities (it is recorded on every stock_ledger row).", + ); + } + // stock_ledger.created_by is a relation to `users`: a non-user + // id would make the ledger write fail AFTER the quantity was + // already changed. + if (!PB_ID_RE.test(actorRaw.id)) { + throw new Error( + "actor.id must be a valid 15-character user id (it is recorded on every stock_ledger row).", + ); + } + + // `dry_run` decides whether anything is written at all, so it + // accepts the string "true" too — a caller that sends "true" + // must not get a real write. + const dryRun = args.dry_run === true || args.dry_run === "true"; + const result = await setStockQuantitiesBulk({ pb, lines, mode, - dryRun: args.dry_run === true, + dryRun, actor: { - id: typeof actorRaw.id === "string" ? actorRaw.id : "", + id: actorRaw.id, name: - typeof actorRaw.name === "string" && actorRaw.name ? actorRaw.name : "Giellie", + typeof actorRaw.name === "string" && actorRaw.name.trim() + ? actorRaw.name + : actorRaw.id, }, }); return jsonRpcOk(body.id, result);