Red-Green TDD Loop

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.

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

2. Tool design

| Tool | Does | Inputs (constraints) | Returns | Read-only / destructive | Errors the model can fix |
|---|---|---|---|---|---|
| `find_customers` | Customers whose name or email contains the text, case-insensitive | `query` string 1–100 (trimmed), `limit` int 1–50 = 20, `cursor` optional | id, name, email; `next_cursor` | read-only, idempotent | no match: try a shorter part of the name or the email domain |
| `list_customer_orders` | One customer's orders, newest first | `customer_id` int ≥ 1, `status` enum optional, `limit`, `cursor` | customer + orders; `next_cursor` | read-only, idempotent | unknown customer: call `find_customers` |
| `get_order` | One order with the customer's name and email | `order_id` int ≥ 1 | order + customer (structured) | read-only, idempotent | not found: check the number or list the customer's orders |
| `refund_order` | Sets a paid order to refunded | `order_id` int ≥ 1, `confirm` int optional, must equal `order_id` | updated order, `previous_status: "paid"` | destructive, idempotent | no `confirm`: preview, ask the user; order pending or already refunded; not found |

Checklist notes: one rule for empty results (an `isError` result with a next step), applied to every tool. All SQL is parameterized. Read tools use a read-only SQLite handle; only `refund_order` opens a writable one, and its `UPDATE ... WHERE id = ? AND status = 'paid'` is a single statement. The guard on the destructive tool is server-side (the preview/confirm step), not just the `destructiveHint` annotation.

3. Scaffold

From `templates/typescript-server.md`. Installed: `@modelcontextprotocol/server` 2.4.0, `@modelcontextprotocol/client` 2.4.0, `zod` 4.6.7; Node 24.6.0.

```
build-an-mcp-server/
  data/seed.sql, data/shop.db     3 customers, 5 orders
  src/index.ts                    stdio entry point
  src/server.ts                   createServer(): the four tools
  src/shop.ts                     SQLite client module (the system being wrapped)
  test/smoke.ts                   spawns the server over stdio, calls every tool
```

5. Test (Inspector CLI, eval loop layer 2)

```
$ npx @modelcontextprotocol/inspector --cli npx tsx src/index.ts --method tools/call --tool-name get_order --tool-arg order_id=1001
{
  "content": [{ "type": "text", "text": "Order #1001 for Ada Lovelace <ada@example.com>: $49.99, paid, created 2026-10-02T09:15:00Z" }],
  "structuredContent": { "order": { "id": 1001, "customer_id": 1, "total_cents": 4999, "status": "paid", "created_at": "2026-10-02T09:15:00Z", "customer_name": "Ada Lovelace", "customer_email": "ada@example.com" } }
}
```

Built, tested, not tested

- Built: four tools over `data/shop.db`, server-side preview/confirm guard on `refund_order`, smoke test, configs for Claude Code, Codex and Cursor, README.
- Tested: starter smoke (pass), full smoke test 14 checks (pass), strict typecheck (pass), Inspector CLI call (pass), seed database unchanged after tests.
- Not tested: the Inspector web UI by hand and the model check (running the five real requests through a live Claude Code session and recording which tool it picks).

[The full answer also holds `src/shop.ts`, `src/server.ts`, `test/smoke.ts` and the three client config snippets.]

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. […]