Build an MCP Server

API contract review — branch feat/orders-v2 vs main

| # | Change | Endpoint | Severity | Rule |
|---|---|---|---|---|
| 1 | `customer_id` removed from response | GET /api/v1/orders/{id} | BREAKING | removed response field (R2) |
| 2 | `status` enum gains `refunded` | GET /api/v1/orders | ADDITIVE, risky | new enum value; strict clients may throw (A4) |
| 3 | `limit` max lowered 100 → 50 | GET /api/v1/orders | BREAKING | tightened validation (R5) |
| 4 | `X-Request-Id` header documented | all | COSMETIC | docs only |

Verdict: 2 breaking changes. Mobile 2.3.x reads `customer_id` on the order screen and will render an empty name.

Recommendation: keep `customer_id` as a deprecated alias for two releases, raise `limit` back to 100 with a server-side cap, and ship `refunded` behind the existing `status` field with a changelog note. No new API version needed if both are done.

Migration note for client teams: handle unknown `status` values as `other` before 2026-12-01.

Project: fresh `npm init -y` + `npm i -D vitest typescript` (Vitest 5.1.0, TypeScript strict). Every runner output below is pasted from the actual run.

Behaviour list (simplest first): 1. returns empty for blank input · 2. returns the address trimmed and lowercased when valid · 3. returns malformed when there is no @ · 4. returns malformed when there is more than one @ · 5. returns malformed when the domain has no dot · 6. returns too_long when the address is over 254 characters

Cycle 1: returns empty for blank input

```
[TDD] cycle 1/6  step RED  behaviour: returns empty for blank input
test:    src/validateEmail.test.ts > validateEmail > returns empty for blank input
ran:     npx vitest run src/validateEmail.test.ts -t "returns empty for blank input"
result:  FAIL  Error: Cannot find module './validateEmail' imported from <project>/src/validateEmail.test.ts
reason:  right (the unit under test does not exist yet: missing function, not a typo in the test)
changed: src/validateEmail.test.ts (+8, new file)
next:    minimum code in src/validateEmail.ts
```

```
[TDD] cycle 1/6  step GREEN  behaviour: returns empty for blank input
ran:     npx vitest run src/validateEmail.test.ts -t "returns empty for blank input" --reporter=verbose; npx vitest run src/validateEmail.test.ts
result:  PASS  Tests 1 passed (1)
changed: src/validateEmail.ts (+9, new file: result types + hard-coded `{ ok: false, error: 'empty' }`)
next:    refactor
```

Refactor: no refactor (hard-coded return is forced into the general version by cycle 2).

[Cycles 2–6 follow the same shape: each RED shows a real assertion mismatch such as `Tests 1 failed | 1 skipped (2)`, each GREEN the passing count, with refactors in cycles 5 and 6.]

Done

```
[TDD] DONE  cycles 6  tests added 6  suite PASS
ran:     npm test   (vitest run)   and   npx tsc -p .
result:  Test Files  1 passed (1) | Tests  6 passed (6); tsc exit 0
pre-existing failures: none (no tests existed)
found, not fixed: the 254 boundary itself (exactly 254 chars accepted) has no test; it passes today, so a test would be green on first run. 'a@.com' and 'a@b.' are accepted because the spec only asks for a dot in the domain.
```

No-fake-green check before every GREEN: no `.skip`/`.todo`, no assertion weakened, no test-only branch, nothing mocked.

> Assumption: signed-in users read their own orders and create their own orders as `pending`; every status change after that is made by your server with the `service_role` key, never by the browser.

Audit: `public.orders` (about 1.8M rows)

Rule files applied: `security-rls-basics`, `security-rls-performance`, `security-privileges`, `schema-foreign-key-indexes`, `query-missing-indexes`, `query-composite-indexes`, `schema-data-types`, `schema-constraints`, `lock-short-transactions`, `monitor-explain-analyze`.

Findings, by rule (first 6 of 12)

| # | Severity | Rule | Finding |
|---|---|---|---|
| 1 | CRITICAL | `security-rls-basics` | `orders_insert ... with check (true)` lets anyone who can insert write an order for any `user_id`. Combined with finding 2, an anonymous visitor holding only your public anon key can create orders on behalf of any user. |
| 2 | CRITICAL | `security-privileges` | `grant select, insert, update ... to anon`. Orders belong to signed-in users, so `anon` needs no privilege at all. None of the three policies has a `to` clause, so each applies to every role, including `anon`. |
| 3 | HIGH | `security-privileges` | `authenticated` can `update` every column of its own rows: the owner can still run `update orders set status = 'paid', total_cents = 1` from the browser. |
| 4 | HIGH | `security-rls-basics` | The insert path also trusts `status`: a client can insert a row that is already `status = 'paid'`. |
| 5 | HIGH | `security-rls-performance` | All policies call `auth.uid()` bare, so it is evaluated per row. Wrap it as `(select auth.uid())` so it is evaluated once per statement. |
| 6 | HIGH | `schema-foreign-key-indexes`, `query-missing-indexes` | No index on `user_id`. It is the foreign key, the RLS filter column and the column every "my orders" query filters on. Each RLS-filtered select over 1.8M rows is a sequential scan. |

Fixing migration (file 1 of 2, excerpt)

```sql
set local lock_timeout = '5s';

revoke all on public.orders from anon;
revoke update, delete, truncate, references, trigger on public.orders from authenticated;
grant select, insert on public.orders to authenticated;

drop policy if exists orders_select on public.orders;
drop policy if exists orders_insert on public.orders;
drop policy if exists orders_update on public.orders;

create policy orders_select on public.orders
  for select to authenticated
  using ((select auth.uid()) = user_id);

create policy orders_insert on public.orders
  for insert to authenticated
  with check ((select auth.uid()) = user_id and status = 'pending');
-- No update or delete policy: denied for every role except service_role.
```

File 2 holds only `create index concurrently if not exists orders_user_id_created_at_idx on public.orders (user_id, created_at desc);` because `concurrently` cannot run inside the transaction the Supabase CLI wraps around each migration.

[The full answer continues with findings 7–12, the lock plan for 1.8M rows, the status and total_cents check constraints added `not valid` then validated, an `explain analyze` to run before and after, and one RLS test per role.]

Assumption: no n8n-MCP server is connected in this session, so I took the skill's fallback path: I wrote the workflow JSON directly from the bundled rules and checked it by review (a rule-check script plus a dry run of every expression). The real validator (`validate_workflow`) was not run, and nothing was deployed.

How the skill routed this

| Step | Sub-skill used | What it decided |
|---|---|---|
| Shape | `n8n-workflow-patterns` | Webhook processing pattern: Webhook, then Validate, then Action, then Respond |
| Node parameters | `n8n-node-configuration` | Slack post needs a channel and text. `responseMode: "responseNode"`. `options.responseCode` set on every Respond node. UUID node ids, no placeholder credentials |
| Expressions | `n8n-expression-syntax` | Webhook data lives under `.body`. Use `$('Webhook').item.json.body.*` once past the IF node. `=` prefix on every expression field |
| Error paths | `n8n-error-handling` | Slack gets `onError: "continueErrorOutput"` and its `main[1]` is wired to the 500 responder. `retryOnFail` is on. Bad input is caught upstream and gets a 400 |
| Validation | `n8n-validation-expert` | Runtime-profile checks done by review. Security warnings get fixed (header auth on the webhook) |

```
Webhook ──> Validate Input ──true──> Post to #signups ──success (main[0])──> Respond 200 OK
                        └──false──> Respond 400 Invalid Input
                                     Post to #signups ──error (main[1])────> Respond 500 Slack Failed
```

Validation result (excerpt)

```
PASS  Webhook responseMode is responseNode (Respond nodes control the reply)
PASS  Every branch (success and each error) ends at a Respond to Webhook node: no hanging branch
PASS  'Post to #signups': onError=continueErrorOutput AND main[1] wired
PASS  'Post to #signups': retryOnFail with maxTries <= 5 and waitBetweenTries <= 5000
PASS  Every {{ }} field starts with = (no 'Expression requires = prefix' error)
PASS  Webhook fields read from .body, never $json.name / $json.email / $json.plan
…
43 passed, 0 failed
```

Expression dry run against sample webhook items:

```
valid           IF -> true  (Slack)  Slack text: New signup: Ana Lima (ana@example.com) on pro
missing_plan    IF -> false (400)
bad_email       IF -> false (400)
no_body         IF -> false (400)
500 body:   {"error":"upstream_error","message":"The signup was received but could not be posted to Slack. Please retry later."}
```

Not verified without the server: the node typeVersions (Webhook 2.2, IF 2.3, Slack 2.3, Respond 1.5) and the Slack v2 parameter names; confirm by opening the nodes after import.

[The full answer also holds the six-node list, the importable workflow JSON and the expressions table with the reason behind each one.]

Phase 1: Root cause investigation
Full suite: FAIL test_expired_coupon_rejected_twice (tests/test_checkout.py:16) AssertionError: Coupon(code='OLD50', …expires_at=2020-01-01…) is not None.
In isolation the same test fails at a different line (17): AssertionError: 50.0 != 100.0. So the first lookup in a process is rejected and every later lookup is accepted; in the full suite the first lookup was done by test_expired_coupon_rejected. Cross-test pollution through module-level state.
Cache probe at the boundary: cache before [] → 1st None → cache after ['OLD50'] → 2nd Coupon(OLD50). Docstring says the cache was added in 3f2a1c9 "to cut DB reads": that is the regression point.
Trace (root-cause-tracing.md): apply_coupon → get_coupon → `if code in _CACHE: return _CACHE[code]`. get_coupon writes the coupon to _CACHE before the expiry check, and the cache-hit branch has no expiry check at all.

Phase 2: Pattern analysis
Working path: cache miss → _load → expiry check. Broken path: cache hit. Expiry depends on `now`, passed per call, so it cannot be a cached property; "don't cache expired coupons" alone would still serve a coupon cached while valid after it expires.

Phase 3: Hypothesis
"get_coupon returns cached coupons without re-checking expires_at against the caller's now; the cache was added around the lookup but the validity check stayed only on the miss path." Confirmed by the probe above.

Phase 4: Implementation
Failing test first: test_cached_coupon_rechecked_for_expiry (look up while valid, then after expiry) plus a setUp that clears the module cache. Before the fix: 5 tests, 2 failures, both for the expected reason.
Fix at the source: the cache stores the raw record; the expiry check runs on every lookup, cached or not.

    c = _CACHE.get(code)
    if c is None:
        c = _load(code)
        if c is None:
            return None
        _CACHE[code] = c
    if c.expires_at < now:
        return None
    return c

Verification: python3 -m unittest -v → Ran 5 tests … OK. The originally failing test in isolation, 3 runs: OK, OK, OK. Fix attempts: 1.

Stage 1, brainstorm. Echo: accountants want one month of invoices as CSV. listInvoices({month}) already filters by YYYY-MM; the seed data has "Acme, Inc." (comma) and Bo "Bob" Ltd (quotes), so RFC 4180 quoting is required. One question: should drafts be included? Accountants usually book only issued invoices; I'd exclude them. (User: exclude.)

Approaches
A. A hand-written toCsv plus exportMonthCsv in a new src/invoices/csv.js: zero dependencies, testable. Recommended.
B. A CSV library: needs npm install, overkill for 8 columns.
C. Put the logic in store.js: mixes formatting with storage.
Small change, so a short design doc and no builder check. Written and approved: docs/specs/2026-11-07-invoice-csv-export-design.md (goals, non-goals, decisions, my calls: CRLF line endings, invalid month throws RangeError).

Stage 2, plan: docs/plans/2026-11-07-invoice-csv-export.md
Global Constraints: no dependencies; columns number, issued_at, client, net, tax, total, currency, status; CRLF and RFC 4180 quoting; drafts excluded.
Review Focus: a comma in the client name; quotes in the client name; an empty month returns the header only; a malformed month throws RangeError; a field containing a newline.
Task 1: CSV serializer. Create src/invoices/csv.js, tests/csv.test.js. Produces toCsv(rows, columns) and INVOICE_COLUMNS. Red, green, commit.
Task 2: Month export. Produces exportMonthCsv(month). Tests: October gives 3 lines and contains "Acme, Inc."; November gives the header only (INV-003 is a draft); '2026-1' and undefined throw RangeError.
User approved and chose inline execution.

Stage 3, ledger (.spec-first/sdd/…/progress.md)
Pre-flight: Ruling: plan Task 1 Step 2 expects FAIL "module not found"; accepted as the expected red for a brand-new module.
Task 1: complete (commits 2b2d09a..bb43c6c, tests: node --test → 2 of 2 passed after red ERR_MODULE_NOT_FOUND)
Task 2: complete (commits bb43c6c..6e3c9a7, tests: node --test → 3 of 3 passed)
Final review: fresh reviewer on the most capable model: "With fixes": 0 Critical, 2 Important, 3 Minor.
Final: fixed CSV formula injection (string fields starting = + - @ TAB CR get a leading '), test red → green, suite 5/5
Final: fixed impossible months 2026-13 / 2026-00 accepted, test red → green, suite 5/5
Final: Ruling: formula guard applies to string values only; negative numeric amounts (credit notes) stay numbers.
Final: minor (deferred): no UTF-8 BOM; Excel on Windows may garble non-ASCII client names.

git log, branch feat/invoice-csv-export
77d1a3b fix: neutralize spreadsheet formulas and reject impossible months in CSV export
6e3c9a7 feat: export a month of invoices as CSV
bb43c6c feat: add RFC 4180 CSV serializer for invoices
2b2d09a docs: invoice CSV export plan
7e753cd docs: invoice CSV export design
Finish options offered: merge, PR, keep, discard. […]

Scope: app/dashboard/page.tsx and the modules it imports (components/charts, UsageCard, ProjectList, RichTextEditor, lib/api.ts).

| # | Rule | File:line | Impact | Finding |
|---|---|---|---|---|
| 1 | async-dependencies | app/dashboard/page.tsx:8-10 | CRITICAL | getProjects(user.id) and getUsage(user.id) depend only on `user` but are awaited one after the other: 3 sequential round trips before any HTML is sent, where 2 would do. Promise.all them. |
| 2 | bundle-barrel-imports | components/ProjectList.tsx:3 | CRITICAL | `import _ from 'lodash'` in a 'use client' file ships all of lodash for one sortBy. Use lodash/sortBy, or toSorted / localeCompare. |
| 3 | async-suspense-boundaries | app/dashboard/page.tsx:7-19 | HIGH | The whole page, including the static shell and RichTextEditor, waits for all three fetches. Move usage and projects into async children in <Suspense> so the shell streams first. |
| 4 | server-serialization | app/dashboard/page.tsx:16 | HIGH | The raw projects array goes to client ProjectList, which reads only id, name and status. Map to {id, name, status} on the server. |
| 5 | rerender-no-inline-components | components/ProjectList.tsx:7 | HIGH | `Row` is defined inside ProjectList, so every keystroke in the filter remounts every <li>. Interaction cost, not first load. Hoist Row. |
| 6 | rerender-derived-state-no-effect | components/UsageCard.tsx:4-5 | MEDIUM | percent is kept in state and set in useEffect: server HTML renders "0% of plan used" and the real value appears only after hydration. Compute it during render. |

Score: 62 / 100 (100 − 2×10 CRITICAL − 3×5 HIGH − 1×3 MEDIUM). This counts rule findings; it is not a measured load time.

Checked and not scored: the local charts barrel (five tiny stubs, tree-shakeable); RichTextEditor is a <textarea> today, so no dynamic import yet. Not a performance rule but it blocks the build: components/charts/index.ts re-exports names the chart files do not export, and page.tsx reads `params.id` synchronously though params is a Promise in Next 15.

What makes the first load slow, in order:
1. The server waterfall (rows 1 and 3). Parallelizing projects and usage removes one full round trip; Suspense lets the header paint after only getUser.
2. Client JS weight (row 2). A one-line import change.
3. Payload and first-paint correctness (rows 4 and 6).
Row 5 is an interaction cost; fix it after the others. Fixes were not requested, so no files were changed.

Method: dual-agent (A: design review · B: detector + browser overlay)

Design Health Score, site/index.html (Persuade surface)
| # | Heuristic | Score | Key issue |
|---|---|---|---|
| 1 | Visibility of system status | 3 | FAQ shows +/–; nav has no current-section state |
| 2 | Match system / real world | 3 | "$" pricing next to VAT and "categorise" blurs the market |
| 3 | User control and freedom | 3 | Nav links vanish under 760px with no menu |
| 4 | Consistency and standards | 2 | Primary CTA purple in nav, mint in hero; 01–03 means both "feature" and "step" |
| 5 | Error prevention | 3 | "Start free" never says whether a card is required |
| 6 | Recognition rather than recall | 3 | Labelled nav, scannable sections |
| 7 | Flexibility and efficiency | n/a | Single-pass marketing page |
| 8 | Aesthetic and minimalist design | 2 | Largest object is an empty placeholder frame; glow is decoration |
| 9 | Error recovery | 2 | Pricing CTAs link to #pricing, the section they sit in |
| 10 | Help and documentation | 3 | FAQ answers switching objections; nothing on security |
| Total | | 24/36 | Acceptable (67%) |

Design specificity verdict. Copy is authored for Ledgerly ("the 3% you need to confirm queued for Friday", reminders on day 3/7/14); the visuals are category-interchangeable dark SaaS: indigo radial glow behind a centred hero, mint pill CTA, three identical 01/02/03 cards, a "Most popular" chip, and a placeholder frame where the product should be.
Deterministic scan: `impeccable detect --json site/index.html`, 13 findings: dark-glow ×6, numbered-section-labels ×3, hero-eyebrow-chip ×1, kicker-above-heading ×1, cramped-padding ×1, overused-font ×1.

Priority issues
- [P0] Hero visual is an empty placeholder. Fix: render a mocked weekly-review screen (unmatched-transaction queue, client match, export). Command: bolder, or new-work for the hero shot.
- [P1] Dead pricing CTAs: "Start free / Start Pro / Talk to us" link to #pricing, so the conversion click silently fails. Command: harden.
- [P1] No trust line at the bank-connection ask: read-only / encryption / provider line under the hero CTAs and step 01. Command: clarify.
- [P2] Inconsistent CTA colour hierarchy plus dark-glow shadows. Command: polish.
- [P2] Generic composition (eyebrow on every section, numbered feature cards, centred glow). Command: bolder.

Persona red flags. Jordan (first-timer): "P&L", "VAT", "quarterly estimates" unexplained; "Start Pro" does nothing. Riley (stress tester): footer Privacy/Terms are "#"; "Talk to us" on a self-serve $29 plan. Casey (mobile): nav links disappear with no menu; long scroll to dead pricing buttons.

Then, "make the hero call-to-action bolder without touching the rest": per bolder.md, amplified what the system already owns (Bricolage Grotesque 800, the mint accent, the pill radius) on a new `.btn-hero` class scoped to the hero: 60px tall, display face at 19px/800, a neutral offset elevation shadow instead of the coloured glow, hover/active/focus-visible states. Nav, pricing and the secondary button untouched; no copy added. Detector after the edit: 12 findings (was 13), all outside the edit's scope. […]