From c943118300007efd3393253b59c511ee7f76e24e Mon Sep 17 00:00:00 2001 From: Dave Date: Fri, 6 Nov 2026 01:26:55 +0200 Subject: [PATCH] fix(giellie): a bad stock line must not take the approval card down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The card is fed the model's raw tool input, so a malformed line (a null, or a qty that is not a number) reached the renderer unchecked and null.sku threw during render — taking the card, and its Approve/Reject buttons, with it. Lines are now read the way the MCP route reads them: an unreadable line is reported in place, the good lines beside it still render, and the operator is told that approving rejects the whole batch. The headings stay put as well. The table now rides inside the card's own scroller instead of a second scroll region, the header row is sticky on an opaque background, and the set/add note rides with it — so every row of a 40-line stocktake is read with its labels and its mode in view. --- src/app/api/mcp/route.test.ts | 15 ++ src/components/giellie/approval-card.test.tsx | 134 +++++++++++++----- src/components/giellie/approval-card.tsx | 100 +++++++++---- 3 files changed, 192 insertions(+), 57 deletions(-) diff --git a/src/app/api/mcp/route.test.ts b/src/app/api/mcp/route.test.ts index 34c9317..6798db0 100644 --- a/src/app/api/mcp/route.test.ts +++ b/src/app/api/mcp/route.test.ts @@ -4372,6 +4372,21 @@ describe("MCP tool stock_items.set_quantities", () => { expect(tool!.inputSchema.properties.actor).toBeTruthy(); }); + it("documents that a row reported as applied on a dry run only means it would be applied", async () => { + const res = await GET( + new NextRequest("http://localhost/api/mcp", { + headers: { authorization: `Bearer ${process.env.AGENT_API_KEY}` }, + }), + ); + const body = (await res.json()) as { tools: Array<{ name: string; description: string }> }; + const tool = body.tools.find((t) => t.name === "stock_items.set_quantities"); + expect(tool).toBeTruthy(); + // On a dry run nothing is written, so "applied" means "would be + // applied". A reader of the description alone must not get the old + // ambiguity. + expect(tool!.description).toMatch(/dry run.*would be applied/i); + }); + it("forwards the batch to the engine: mode, dry_run, actor and truncated qty", async () => { const res = await call({ mode: "set", diff --git a/src/components/giellie/approval-card.test.tsx b/src/components/giellie/approval-card.test.tsx index 8946233..d10c21d 100644 --- a/src/components/giellie/approval-card.test.tsx +++ b/src/components/giellie/approval-card.test.tsx @@ -97,27 +97,29 @@ describe("ApprovalCard", () => { describe("the stock quantity tool renders a table", () => { + const stockPending = (input: Record): PendingAction => ({ + tool_uses: [ + { + id: "w1", + name: "stock_items.set_quantities", + input, + confirmAlways: false, + }, + ], + completed: [], + order: ["w1"], + }); + it("lists the sku, description and qty", () => { - render( + const { container } = render( , @@ -126,24 +128,15 @@ describe("the stock quantity tool renders a table", () => { expect(screen.getByText("Bolt M12")).toBeTruthy(); expect(screen.getByText("50")).toBeTruthy(); expect(screen.getByText(/new quantity on hand/i)).toBeTruthy(); - expect(document.body.textContent).not.toMatch(/"lines"/); + // The card renders a table — scoped to this card, not the whole page. + expect(screen.getByRole("table")).toBeTruthy(); + expect(container.textContent).not.toMatch(/"lines"/); }); it("says when the quantities are being added instead of set", () => { render( , @@ -152,7 +145,7 @@ describe("the stock quantity tool renders a table", () => { }); it("leaves other tools as JSON", () => { - render( + const { container } = render( { busy={false} />, ); - expect(document.body.textContent).toMatch(/"status"/); + expect(container.textContent).toMatch(/"status"/); + // No table for another tool … + expect(screen.queryByRole("table")).toBeNull(); + // … and the JSON keeps the card's own styling. + const pre = container.querySelector("pre"); + expect(pre).toBeTruthy(); + expect(pre!.className).toContain("max-h-48"); + expect(pre!.className).toContain("overflow-auto"); + expect(pre!.className).toContain("whitespace-pre-wrap"); + expect(pre!.className).toContain("break-words"); + expect(pre!.className).toContain("text-muted-foreground"); + }); + + it("keeps the headings in view for a 40-line batch, in the card's own scroller", () => { + const lines = Array.from({ length: 40 }, (_, i) => ({ + sku: `SKU-${i}`, + description: `Item ${i}`, + qty: i, + })); + render(); + const table = screen.getByRole("table"); + expect(table.querySelectorAll("tbody tr")).toHaveLength(40); + // The headings are pinned, so no row of the batch is read without them. + const head = table.querySelector("thead"); + expect(head!.className).toContain("sticky"); + expect(head!.className).toContain("bg-background"); + // The card's list is the only scroller — no second scroll region. + const list = screen.getByTestId("approval-list"); + expect(list.className).toContain("overflow-y-auto"); + expect(table.closest(".overflow-y-auto")).toBe(list); + }); + + it("shows a dash for a line with no sku or description", () => { + render( + , + ); + const cells = screen.getByRole("table").querySelectorAll("tbody td"); + expect(cells).toHaveLength(3); + expect(cells[0].textContent).toBe("—"); + expect(cells[1].textContent).toBe("—"); + expect(cells[2].textContent).toBe("7"); + }); + + it("shows a zero quantity as zero, not as a blank", () => { + render( + , + ); + const cells = screen.getByRole("table").querySelectorAll("tbody td"); + expect(cells[2].textContent).toBe("0"); + }); + + it("survives a malformed line and names it, without hiding the good lines", () => { + render( + , + ); + // The card did not fall over: the buttons are still on screen, so the + // operator is never stranded by a line they cannot see. + expect(screen.getByRole("button", { name: /approve/i })).toBeTruthy(); + expect(screen.getByRole("button", { name: /reject/i })).toBeTruthy(); + // The bad line is reported, and it does not read as a partial write. + expect(screen.getByText(/line 1 could not be read/i)).toBeTruthy(); + expect(screen.getByText(/reject the whole batch/i)).toBeTruthy(); + // The good line next to it still renders. + expect(screen.getByText("ABC-123")).toBeTruthy(); }); }); diff --git a/src/components/giellie/approval-card.tsx b/src/components/giellie/approval-card.tsx index 300afce..a62c965 100644 --- a/src/components/giellie/approval-card.tsx +++ b/src/components/giellie/approval-card.tsx @@ -7,6 +7,36 @@ import type { PendingAction } from "@/lib/giellie-types"; type StockQuantityLine = { sku?: string; description?: string; qty?: number }; +/** + * Read one pasted line defensively. The card is fed the model's raw + * tool input — nothing validates it before it reaches here — and the + * Approve/Reject buttons live in this card, so one malformed line must + * never take the card down. The rules mirror the MCP route's own + * validation, so a line the card calls unreadable is a line the route + * would reject. + */ +function readLine(raw: unknown): { line: StockQuantityLine } | { problem: string } { + if (!raw || typeof raw !== "object" || Array.isArray(raw)) { + return { problem: "not a line object" }; + } + const l = raw as { sku?: unknown; description?: unknown; qty?: unknown }; + const asText = (value: unknown, field: string): { value?: string; problem?: string } => { + if (value === undefined || value === null) return { value: undefined }; + if (typeof value === "string") return { value }; + // A spreadsheet paste sends a numeric SKU; the route reads it as text too. + if (typeof value === "number" && Number.isFinite(value)) return { value: String(value) }; + return { problem: `${field} must be text` }; + }; + const sku = asText(l.sku, "sku"); + if (sku.problem) return { problem: sku.problem }; + const description = asText(l.description, "description"); + if (description.problem) return { problem: description.problem }; + if (typeof l.qty !== "number" || !Number.isFinite(l.qty)) { + return { problem: "qty must be a number" }; + } + return { line: { sku: sku.value, description: description.value, qty: l.qty } }; +} + /** * The stock quantity tool is the one tool whose input is a list the * operator has to read line by line. JSON is not reviewable at that @@ -16,35 +46,57 @@ export function renderToolInput(toolName: string, input: unknown): ReactNode { if (toolName === "stock_items.set_quantities" && input && typeof input === "object") { const payload = input as { mode?: unknown; lines?: unknown }; const mode = typeof payload.mode === "string" ? payload.mode : ""; - const lines = Array.isArray(payload.lines) ? (payload.lines as StockQuantityLine[]) : []; - if (lines.length > 0) { + const rawLines: unknown[] = Array.isArray(payload.lines) ? payload.lines : []; + if (rawLines.length > 0) { + // One scroller only: the card's own list scrolls (that is what + // its pinned header was built for), so the table rides inside it + // and its headings stay put for every row of a 40-line batch. return ( -
-

- {mode === "add" - ? "Adding these quantities to the current stock." - : "Setting these as the new quantity on hand."} -

-
- - - - - - - - - - {lines.map((line, i) => ( - +
+
SKUDescriptionQty
+ + + + + + + + + + + + {rawLines.map((raw, i) => { + const read = readLine(raw); + if ("problem" in read) { + // Reported, never dropped: one bad line must not hide + // the rest, and it must not read as a partial write. + return ( + + + + ); + } + const line = read.line; + return ( + - ))} - -
+ {mode === "add" + ? "Adding these quantities to the current stock." + : "Setting these as the new quantity on hand."} +
SKUDescriptionQty
+ Line {i + 1} could not be read ({read.problem}) — approving will reject the whole + batch. +
{line.sku ?? "—"} {line.description ?? "—"} {line.qty ?? "—"}
-
+ ); + })} + +
); }