docs(orders): handoff doc for outstanding order-linking work
This commit is contained in:
parent
0083199ed2
commit
5a8b8575b7
1 changed files with 331 additions and 0 deletions
331
docs/superpowers/plans/2026-08-29-HANDOFF-outstanding-work.md
Normal file
331
docs/superpowers/plans/2026-08-29-HANDOFF-outstanding-work.md
Normal file
|
|
@ -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.
|
||||
Loading…
Reference in a new issue