David/WFDASH-WEBAPP
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(mcp): a stock quantity write needs an acting user

Browse source 
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.
This commit is contained in:
Dave 2026-11-05 23:53:54 +02:00
parent 2c415d82f1
commit 38cd015e28
2 changed files with 290 additions and 58 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

254
src/app/api/mcp/route.test.ts
View file

@ -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<string, unknown>) =>
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<string, unknown> };
}>;
};
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();
});
});

94
src/app/api/mcp/route.ts
View file

@ -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<string>();
const duplicates = new Set<string>();
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<string, { raw: string; line: number }>();
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);