David/WFDASH-WEBAPP
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
WFDASH-WEBAPP/docs/superpowers/plans/2026-08-29-HANDOFF-outstanding-work.md

17 KiB
Raw Permalink Blame History

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):

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:

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.

} 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 — DONE 2026-08-29. Sweep run, window confirmed at 90 days.

Outcome: FUZZY_WINDOW_DAYS stays at 90. No change needed. Phase 2 is unblocked.

Full backlog swept: 5,096 orders processed, 2,483 suggested, 2,613 no-match, 0 failed — remaining: 0. Amber pills are now live across the historical backlog (approved by the owner).

Read this before you trust the sweep's ageGaps output. The histogram reported d0_30: 100%, which is an artifact, not a finding:

  • 92% of matched backlog orders have a gap of exactly 0 days. Their created timestamps come from the legacy bulk migration — order and quote were inserted at the same instant — so the gap measures migration timing, not quote-to-order lead time.
  • quoteToOrderAgeDays itself is correct (verified: returns 33 for a real 33-day PB-format pair). Do not go looking for a bug in it.
  • The real signal, from non-migrated orders: gaps run 1–33 days, p50 23, p90 24, max 33. Zero beyond 90 days. That's what justifies keeping 90 — nearly 3× headroom against the worst observed real gap.

Consequence worth knowing: for migrated records the date window constrains nothing. If an order and every candidate quote share a migration timestamp, all of them fall inside any window. So those ~2,400 backlog suggestions were scored with the window effectively disabled — only company / customer / description / price carried them. Backlog amber pills deserve more operator skepticism than new-order ones, whose timestamps are real and where the window does its job.

Two follow-ups, neither blocking Phase 2:

  1. Have the sweep report gap-0 matches in their own bucket so this histogram can't mislead the next reader the way it nearly did here.
  2. Check whether the migration preserved a real business date (there is a migration_meta JSON field on orders) that the matcher could prefer over created for migrated rows.

Separate open question — do not conflate it with the window. Match rate is 48.7%, well under the owner's ~85% expectation. The misses are not old quotes, so widening the window will not help. Candidates: those orders genuinely had no quote; the MATCH_CONFIDENCE_THRESHOLD of 0.4 is too strict; or description drift exceeds what the Jaccard scorer tolerates. Diagnose deliberately, don't tune blind.

Operational note for anyone re-running the sweep: limit: 200 killed the dev server outright (process gone, not a timeout). limit: 50 is stable at ~14s per batch. The route advertises up to 500 with maxDuration = 300; that ceiling is optimistic, because each order fans out into a candidate scan of up to 200 quotes plus their line items, twice when the window widens. Consider lowering the documented max.

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":50}'

(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.