From 5a8b8575b7b59f44251bfab30a261739ede0a5d0 Mon Sep 17 00:00:00 2001 From: Dave Date: Sat, 29 Aug 2026 23:53:30 +0200 Subject: [PATCH] docs(orders): handoff doc for outstanding order-linking work --- .../2026-08-29-HANDOFF-outstanding-work.md | 331 ++++++++++++++++++ 1 file changed, 331 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-29-HANDOFF-outstanding-work.md diff --git a/docs/superpowers/plans/2026-08-29-HANDOFF-outstanding-work.md b/docs/superpowers/plans/2026-08-29-HANDOFF-outstanding-work.md new file mode 100644 index 0000000..ee36fdd --- /dev/null +++ b/docs/superpowers/plans/2026-08-29-HANDOFF-outstanding-work.md @@ -0,0 +1,331 @@ +# Handoff: outstanding work on `feat/order-quote-auto-linking` + +**Written:** 2026-08-29 +**Branch:** `feat/order-quote-auto-linking` (branched from `main`) +**HEAD at handoff:** `0083199` + +This document is for an implementing agent picking up mid-feature. It states +what is genuinely done (verified, not assumed), what is outstanding, and — +importantly — **where the original plan documents have gone stale**, because +several review cycles changed things the plans still describe the old way. + +Read this file before reading the plans. Where they disagree, this file wins. + +--- + +## Two plans are in flight + +| Plan | Status | +|---|---| +| `docs/superpowers/plans/2026-08-29-order-quote-linking.md` | Tasks 1–6 done, **7–13 outstanding** | +| `docs/superpowers/plans/2026-08-29-order-delivery-note-match.md` | **Not started**, all 6 tasks outstanding | + +The delivery-note plan depends on the quote plan's Task 6 (already merged), so +they can proceed in parallel from here. Its own pre-flight section explains +the dependency. + +--- + +## Verified current state + +Confirmed by running the commands, not by trusting prior reports: + +- **13 feature commits** on the branch since the plan landed. Working tree clean. +- `src/lib/server/order-quote-link-service.ts` — **481 lines**, exports + `QuoteMatchMethod`, `QuoteMatchCandidate`, `SuggestQuoteResult`, + `suggestQuoteForOrder`, `PairedLine`, `PairOrderLinesResult`, `pairOrderLines`. +- **55 tests passing** across `order-quote-link-service.test.ts` + + `order-quote-match.test.ts`. +- Full suite: **15 pre-existing failures across 9 files** — all predate this + work. Do not fix them, but do confirm you add none. Baseline files: + `pocketbase-admin.test.ts` (2), `po-agent-workflow.test.ts` (2), MCP + `route.test.ts` (2), `orders/route.test.ts` (2), + `quotes/[id]/submit/route.test.ts` (3), `sidebar.test.tsx` (1), + `delivery-notes-service.test.ts` (1), `orders-service.test.ts` (1 — + `splitOrderForContinuation > moves pending items...`), `invoicing/page.test.tsx` (1). + **Diff failing test NAMES, not counts** — this was how a real regression + would otherwise have hidden behind a coincidentally-equal count. + +### What works end-to-end today + +An order created through `createOrder` (the n8n email-intake path, via MCP +`orders.create` or `POST /api/v1/orders/create`) now automatically gets its +source quote detected and written to `potential_quote`, and the existing amber +"Suggested" pill appears on `/orders` with no human action. Linking a quote via +`setOrderQuote` (REST, MCP, or UI) now also persists confident order-line → +quote-line pairs onto `order_items.quote_item`. + +### What does NOT work yet + +- The **historical backlog is untouched** — only orders created from now on are + matched. That's Task 7. +- **No UI reads the line pairs.** The buyer notes and per-line attachments that + motivated this whole feature still don't show. That's Tasks 9–13. + +--- + +## ⚠️ Where the plans are STALE — read before implementing + +Six review cycles changed things the plan text still describes the old way. + +### 1. `SuggestQuoteResult.window` is `"initial"`, not `"90d"` + +The plan's Task 7 sample code and prose refer to a `"90d"` label. Review +changed it so retuning `FUZZY_WINDOW_DAYS` can't make the label lie. Actual +current shape (`order-quote-link-service.ts:54-67`): + +```ts +export interface SuggestQuoteResult { + orderId: string; + best: QuoteMatchCandidate | null; + alternatives: QuoteMatchCandidate[]; + method: QuoteMatchMethod; + window: "exact" | "initial" | "widened" | "none"; + ageGapDays: number | null; + skipped?: "already_linked"; +} +``` + +`ageGapDays` **does** exist and is populated — Task 7's histogram can read it +directly, no recomputation needed. + +### 2. The test mock harness was refactored + +`order-quote-link-service.test.ts` now has a `makePb(overrides)` that merges +per-test overrides over sensible defaults, and the helper is `mockAdminPb`, +**not** `useP`. The plan's Task 4/7 test snippets show the old style. Read the +current file and follow it. + +### 3. `vitest.config.ts` has no `clearMocks` — this bit us three times + +Mock call history is **not** reset between tests; the global `beforeEach` only +calls `clearStore()`. Three separate tests were found passing on history +leaked from tests 150 lines earlier, guarding nothing. + +**Always `vi.mocked(fn).mockClear()` at the top of any test asserting +`toHaveBeenCalledWith`.** And prove your test isn't vacuous: delete the call +site, watch the test fail, restore. Do not skip this — it caught real holes +every time it was run. + +Do **not** globally set `clearMocks: true` as a drive-by fix; it changes +behaviour across 2,278 tests and needs its own change with its own verification. + +### 4. Quote-item fetches go through a chunked helper + +Do not hand-build `quote = "a" || quote = "b" || ...` OR-chains. PB rejects +filters past ~3.5KB (measured: 107 ids fail, 106 pass — see +`orders-service.ts:499-505`). Use `fetchQuoteItemsForQuotesAdmin(pb, ids, fields)` +from `quotes-service.ts`, which does IN-syntax with an 80-id chunked fallback. +It takes an explicit `fields` param — **pass what you need**, because a +narrowed default projection silently degrades matching to near-zero +confidence with no error. + +### 5. Field projections are load-bearing and must stay tested + +`MATCHER_QUOTE_ITEM_FIELDS`, `PAIR_ORDER_ITEM_FIELDS`, `PAIR_QUOTE_ITEM_FIELDS` +exist as named constants because dropping a field from one silently disables a +safety guard while the suite stays green. If you add a query, assert its +projection in a test. + +### 6. `createOrder`'s `after()` is guarded — don't add a second one + +`after()` **throws synchronously** outside a request scope (Next 16.2.6, error +`E468`, `node_modules/next/dist/server/after/after.js`). `createOrder` wraps +the `after()` call itself and falls back to fire-and-forget. There is a single +`runQuoteMatch` closure — **add new post-intake work inside it**, in its own +`try/catch`, rather than calling `after()` again. + +--- + +# OUTSTANDING WORK + +## A. Task 6 follow-up fixes (do first — small, and they close a real hole) + +Task 6 is merged (`21bc577`) and passed spec review, but its code review found +two issues that were never applied. Both in `src/lib/server/orders-service.ts` +and its test file. + +**A1. The fallback path has zero coverage, and one test is vacuous.** + +The test mocks `next/server` so `after` never throws — meaning the `catch` + +fire-and-forget fallback (~`orders-service.ts:2238-2249`) is never executed by +the suite. Worse, the reviewer deleted the inner `try/catch` from +`runQuoteMatch` entirely and **both existing tests still passed**, because the +mock's `void cb()` detaches the promise regardless of what production code does. + +Add: + +```ts +it("falls back to an inline match when after() is unavailable", async () => { + const { after } = await import("next/server"); + const { suggestQuoteForOrder } = await import("./order-quote-link-service"); + vi.mocked(suggestQuoteForOrder).mockClear(); + vi.mocked(after).mockImplementationOnce(() => { + throw new Error("`after` was called outside a request scope."); + }); + await pbMock.collection("customers").create({ id: "cust-x", name: "Acme" }); + await pbMock.collection("companies").create({ id: "comp-x", name: "Acme (Pty) Ltd" }); + + const r = await createOrder({ + customer_id: "cust-x", + company_id: "comp-x", + items: [{ description: "M16 Bolt", quantity: 50 }], + }); + + expect(r.order.id).toBeDefined(); + expect(suggestQuoteForOrder).toHaveBeenCalledWith(r.order.id); +}); +``` + +Also add a `console.error` spy assertion to the existing +`"never fails order creation when the matcher throws"` test so it verifies the +swallow rather than the mock's detachment. + +**A2. The `catch` around `after()` logs nothing.** + +If `after()` becomes unavailable (a Next upgrade, or `waitUntil` missing on the +deploy target — a second distinct throw path in `after-context.js` that the bare +catch also swallows), every order silently degrades to fire-and-forget with +**zero trace anywhere**. Nothing breaks visibly; you just lose the latency +guarantee permanently and invisibly. + +```ts +} catch (err) { + console.error(`[createOrder] after() unavailable, running quote match inline for ${order.id}:`, err); + void runQuoteMatch(); +} +``` + +**A3 (optional).** The comment explaining the guard names neither the error +code nor the Next version, so a future reader hitting an upgrade must redo the +source dive. Add `// Next 16.2.6, error code E468 — node_modules/next/dist/server/after/after.js`. +Its stated justification also cites a smoke-script caller that doesn't exist — +all three real callers are in request scope. Keep the fallback (`createOrder` is +an exported service function reachable from multiple layers per this repo's +architecture rules), but fix the comment to say that. + +Commit: `fix(orders): cover the after() fallback path and log when it engages` + +--- + +## B. Tasks 7–13 of the order↔quote plan + +Full step-by-step detail is in +`docs/superpowers/plans/2026-08-29-order-quote-linking.md` — **read it there**, +subject to the staleness corrections above. Summary of what remains: + +### Task 7 — Backlog sweep +`POST /api/v1/orders/quote-match/sweep` + `sweepUnmatchedOrders` and +`bucketAgeGaps` in the link service. Processes orders where +`quote_matching_attempted = false && quote = ""` in batches, returns +`{ processed, suggested, noMatch, failed, remaining, ageGaps }`. + +The `ageGaps` histogram is not decoration — it's how the 90-day +`FUZZY_WINDOW_DAYS` default gets retuned from real data instead of staying an +estimate. Read `ageGapDays` straight off each `SuggestQuoteResult`. + +Verify `quote = ""` is genuinely how this codebase queries an empty relation +(grep `orders-service.ts`) rather than assuming PB's syntax. + +### Task 8 — MCP tool `orders.quote_match_sweep` +Hermes/n8n drops the `AGENT_API_KEY` on direct REST calls, so the sweep must +exist as an MCP tool. Follow the `orders.set_quote` pattern in +`src/app/api/mcp/route.ts` — tool definition alongside it, handler forwarding +to the REST route. + +### ⛔ CHECKPOINT — human runs a real sweep before Phase 2 + +Run one batch against real data, read the `ageGaps` histogram, and decide +whether 90 days is right before building UI on top. Expect the amber pill to +appear across many historical orders at once on the first full sweep — that's +information, not damage, but it will look noisy for a day. + +```bash +curl -s -X POST http://localhost:4001/api/v1/orders/quote-match/sweep \ + -H "authorization: Bearer $AGENT_API_KEY" \ + -H 'content-type: application/json' -d '{"limit":10}' +``` + +(Dev server runs on **port 4001**, not 3000.) + +### Task 9 — Extract `AttachmentList` to `src/components/orders/attachment-list.tsx` +Currently local to `process/page.tsx`. Pure move so both order pages can share it. + +### Task 10 — Serve the resolved pairing to the UI +Add `linePairs` (keyed by `order_item_id`, carrying `quote_item_id`, +`confidence`, `tier`) to `matched-attachments/route.ts`. Additive — the MCP +`orders.list_attachments` consumer must keep working. Plus +`POST /api/v1/orders/[id]/quote-line` to confirm a doubtful pair. + +### Task 11 — `src/components/orders/quote-line-context.tsx` +Shared presentation of a paired quote line: description, buyer note, +attachments, plus a "possible match — confirm?" state and an explicit +"no matching line" state. + +### Task 12 — Process page uses server pairing +**Delete `findMatchedQuoteItem`** (`process/page.tsx:3159`) — the exact/substring +guesser that is the actual root cause of vanishing buyer notes. Replace its +three call sites with `linePairs` lookups. + +### Task 13 — Order detail table + migrate off `pb-helpers` +Row indicators (note icon, paperclip + count) expanding to show quote context. +Then migrate the four `pb-helpers` matcher callers still live at +`src/app/(app)/orders/[id]/page.tsx:59-62` (`findMatchingQuotesForOrder`, +`applyOrderQuoteMatch`, `clearOrderQuoteMatch`, `setOrderQuoteManually`) onto +`/api/v1/*`, then delete the stale matcher block from `pb-helpers.ts`. + +**Note the API shape change:** the old `result.best.quote.quote_number` becomes +`result.best.quoteNumber`, and `result.best.quote.id` becomes +`result.best.quoteId`. + +`AGENTS.md` requires this migration when touching that page — it is not optional +cleanup. + +--- + +## C. The delivery-note repeat-order feature (all 6 tasks) + +`docs/superpowers/plans/2026-08-29-order-delivery-note-match.md`, spec at +`docs/superpowers/specs/2026-08-29-order-delivery-note-match-design.md`. + +Detects when a new order's items exactly match a past delivery note's items +(same **company** — a different person at the company can place the repeat +order), and flags it with a blue pill showing the DN number. + +Two things to know before starting: + +- **It needs a PB schema migration** (Task 1 of that plan) — five new `orders` + fields. All are relation/text/number/bool, **not `select`**, so no manual PB + admin step is required. Confirm with the user before running any migration + against the live database. +- **The pill reads a denormalized text field, not an expanded relation.** + `orders-service.ts:253` documents that PB issues a separate query per + expanded relation, which is why the existing quote pill shows no number. + Since this pill must show the DN number, the plan denormalizes + `delivery_note_number` onto the order at write time. Do not "simplify" this + into an `expand` — it would reintroduce a per-page-load cost the codebase + deliberately avoids. + +Its matching is **exact**, not fuzzy — either every line pairs on normalized +description and quantity with nothing left over on either side, or nothing is +flagged. There is no "possible" tier. That's deliberate and different from the +quote matcher. + +--- + +## Working agreement that produced good results here + +Every task so far ran: implement → spec-compliance review → code-quality review +→ fix findings → re-verify. That caught, among others: + +- a PB filter-length bug that would have broken exactly the high-volume + customers the matcher matters most for +- a cross-line collision that would have shown two different parts the same + estimator's note — the precise failure the feature exists to prevent +- three tests that passed while guarding nothing +- `after()` throwing outside request scope, which would have made the safety + code itself crash order creation + +The pattern that found most of these was **mutation testing**: break the thing +deliberately, confirm a test fails, restore. Please keep doing that rather than +trusting that a green suite means a guarded one.