From e6f5a25e11adffccda1f363fb1a00a5d226b88cd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 29 Jun 2026 23:33:05 +0000 Subject: [PATCH 1/2] test(vendors): regression tests + AT_RISK_THRESHOLD + live-risk sort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the merged vendor spend-control feature (#119), closing gaps found in self-review: - tests/mcp-vendors.test.ts — hermetic regression (mocked getDb) pinning the at_risk-before-limit fix: filter → sort by live risk → slice, and live risk recompute when the stored risk_score is stale/null. Runs everywhere. - tests/routes/vendors.spec.ts — real-Neon route spec (DATABASE_URL-gated, per the no-mocks rule) covering POST upsert, PATCH metadata persistence (the bug Codex flagged), at_risk filtering, and /summary. - AT_RISK_THRESHOLD constant replaces the duplicated `>= 50` magic number across the route, MCP tools, and cron sweep. - GET /api/vendors now sorts by LIVE risk, not the stale stored risk_score column (the MCP path already did after the #119 fixes; the HTTP route didn't). - Document POST's FULL-representation (clobbering) upsert semantics so partial re-POSTs don't silently wipe spend data — use PATCH for partial updates. https://claude.ai/code/session_015mkdG1VYH3AdqLe4E3i9H6 --- src/lib/cron.ts | 4 +- src/lib/vendor-risk.ts | 4 ++ src/routes/mcp.ts | 6 +- src/routes/vendors.ts | 16 +++-- tests/mcp-vendors.test.ts | 135 +++++++++++++++++++++++++++++++++++ tests/routes/vendors.spec.ts | 125 ++++++++++++++++++++++++++++++++ 6 files changed, 280 insertions(+), 10 deletions(-) create mode 100644 tests/mcp-vendors.test.ts create mode 100644 tests/routes/vendors.spec.ts diff --git a/src/lib/cron.ts b/src/lib/cron.ts index dfafe44..fd51307 100644 --- a/src/lib/cron.ts +++ b/src/lib/cron.ts @@ -9,7 +9,7 @@ import { generatePaymentPlan, savePaymentPlan } from './payment-planner'; import { reconcileNotionDisputes } from './dispute-sync'; import { enqueueJob, processQueue, type ScrapeJobType } from './job-dispatcher'; import { decayStaleRouxIntents } from './intent-decay'; -import { computeVendorRisk, vendorRiskInputFromRow } from './vendor-risk'; +import { computeVendorRisk, vendorRiskInputFromRow, AT_RISK_THRESHOLD } from './vendor-risk'; /** * Cron sync orchestrator. @@ -267,7 +267,7 @@ async function sweepVendorRisk( const { score } = computeVendorRisk(vendorRiskInputFromRow(r)); ids.push(r.id as string); scores.push(score); - if (score >= 50) atRisk++; + if (score >= AT_RISK_THRESHOLD) atRisk++; } await sql` UPDATE cc_vendors SET risk_score = bulk.score, updated_at = NOW() diff --git a/src/lib/vendor-risk.ts b/src/lib/vendor-risk.ts index a37040d..102706b 100644 --- a/src/lib/vendor-risk.ts +++ b/src/lib/vendor-risk.ts @@ -13,6 +13,10 @@ */ import { urgencyLevel, type UrgencyLevel } from './urgency'; +// Score at/above which a vendor is "at risk" (high/critical). Mirrors the +// urgencyLevel 'high' boundary; centralised so routes, MCP, and cron agree. +export const AT_RISK_THRESHOLD = 50; + export type VendorPaymentStatus = 'active' | 'failed' | 'limited' | 'unknown'; export type VendorStatus = 'active' | 'paused' | 'cancelled' | 'zombie'; diff --git a/src/routes/mcp.ts b/src/routes/mcp.ts index 3c6c38e..22d4a68 100644 --- a/src/routes/mcp.ts +++ b/src/routes/mcp.ts @@ -14,7 +14,7 @@ const TRIAGE_TOOL_NAMES = new Set([ ]); import { getDb, typedRows } from '../lib/db'; import type { NeonQueryFunction } from '@neondatabase/serverless'; -import { computeVendorRisk, vendorRiskInputFromRow, numOrNull } from '../lib/vendor-risk'; +import { computeVendorRisk, vendorRiskInputFromRow, numOrNull, AT_RISK_THRESHOLD } from '../lib/vendor-risk'; import { listJobs, getJobStatus, retryJob, getDeadLetters, enqueueJob } from '../lib/job-dispatcher'; import type { ScrapeJobType, ScrapeJobStatus } from '../lib/job-dispatcher'; import { evidenceClient, ledgerClient, govClient } from '../lib/integrations'; @@ -1163,7 +1163,7 @@ async function executeTool(env: Env, sql: NeonQueryFunction, toolN const risk = computeVendorRisk(vendorRiskInputFromRow(r)); return { ...r, risk_score: risk.score, risk_level: risk.level, risk_reasons: risk.reasons }; }); - if (atRisk) vendors = vendors.filter((v) => (v.risk_score as number) >= 50); + if (atRisk) vendors = vendors.filter((v) => (v.risk_score as number) >= AT_RISK_THRESHOLD); vendors.sort((a, b) => (b.risk_score as number) - (a.risk_score as number)); const limited = vendors.slice(0, limit); return { count: limited.length, vendors: limited }; @@ -1183,7 +1183,7 @@ async function executeTool(env: Env, sql: NeonQueryFunction, toolN if (r.billing_cycle === 'monthly') monthlyCommitted += numOrNull(r.expected_amount) ?? 0; const risk = computeVendorRisk(vendorRiskInputFromRow(r)); byLevel[risk.level] = (byLevel[risk.level] || 0) + 1; - if (risk.score >= 50) atRisk.push({ vendor_name: r.vendor_name, category: r.category, score: risk.score, level: risk.level, reasons: risk.reasons }); + if (risk.score >= AT_RISK_THRESHOLD) atRisk.push({ vendor_name: r.vendor_name, category: r.category, score: risk.score, level: risk.level, reasons: risk.reasons }); } atRisk.sort((a, b) => b.score - a.score); return { diff --git a/src/routes/vendors.ts b/src/routes/vendors.ts index 954a6f4..d61153c 100644 --- a/src/routes/vendors.ts +++ b/src/routes/vendors.ts @@ -1,7 +1,7 @@ import { Hono } from 'hono'; import type { Env } from '../index'; import { getDb } from '../lib/db'; -import { computeVendorRisk, vendorRiskInputFromRow, numOrNull } from '../lib/vendor-risk'; +import { computeVendorRisk, vendorRiskInputFromRow, numOrNull, AT_RISK_THRESHOLD } from '../lib/vendor-risk'; import { createVendorSchema, updateVendorSchema, vendorQuerySchema } from '../lib/validators'; export const vendorRoutes = new Hono<{ Bindings: Env }>(); @@ -27,8 +27,11 @@ vendorRoutes.get('/', async (c) => { AND (${status}::text IS NULL OR status = ${status}) ORDER BY risk_score DESC NULLS LAST, next_bill_date ASC NULLS LAST, vendor_name ASC `; - const vendors = rows.map((r) => ({ ...r, risk: computeVendorRisk(vendorRiskInputFromRow(r)) })); - const filtered = atRisk ? vendors.filter((v) => v.risk.score >= 50) : vendors; + const vendors = rows + .map((r) => ({ ...r, risk: computeVendorRisk(vendorRiskInputFromRow(r)) })) + // Sort by *live* risk, not the stored (possibly stale) risk_score column. + .sort((a, b) => b.risk.score - a.risk.score); + const filtered = atRisk ? vendors.filter((v) => v.risk.score >= AT_RISK_THRESHOLD) : vendors; return c.json({ count: filtered.length, vendors: filtered }); }); @@ -62,7 +65,7 @@ vendorRoutes.get('/summary', async (c) => { const risk = computeVendorRisk(vendorRiskInputFromRow(r)); byLevel[risk.level] = (byLevel[risk.level] || 0) + 1; - if (risk.score >= 50) { + if (risk.score >= AT_RISK_THRESHOLD) { atRisk.push({ id: r.id, vendor_name: r.vendor_name, category: cat, payment_status: r.payment_status, score: risk.score, level: risk.level, reasons: risk.reasons }); } if (r.status === 'zombie') { @@ -104,7 +107,10 @@ vendorRoutes.get('/:id', async (c) => { return c.json({ ...row, risk: computeVendorRisk(vendorRiskInputFromRow(row)) }); }); -// Create or upsert a vendor (keyed on vendor_name). POST is a full representation. +// Create or upsert a vendor (keyed on vendor_name). POST is a FULL representation: +// on conflict every column is overwritten from this payload, and omitted fields +// fall back to their defaults (e.g. mtd_spend→0, payment_status→'unknown'). Use +// PATCH for partial updates so existing spend/status data isn't clobbered. vendorRoutes.post('/', async (c) => { const raw = await c.req.json(); const result = createVendorSchema.safeParse(raw); diff --git a/tests/mcp-vendors.test.ts b/tests/mcp-vendors.test.ts new file mode 100644 index 0000000..fcfa06b --- /dev/null +++ b/tests/mcp-vendors.test.ts @@ -0,0 +1,135 @@ +/** + * Hermetic regression tests for the vendor MCP tools (query_vendors, + * get_vendor_risk). getDb is mocked (same approach as tests/mcp.test.ts) so + * these run without a database and pin the exact bug Codex flagged on #119: + * at_risk filtering + risk recompute must happen BEFORE limiting, and ordering + * must use live risk — not the stored (possibly stale) risk_score column. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { Hono } from 'hono'; +import type { Env } from '../src/index'; +import { mcpAuthMiddleware } from '../src/middleware/auth'; +import type { AuthVariables } from '../src/middleware/auth'; + +// Mutable row set the mocked sql tagged-template resolves to. Hoisted so the +// vi.mock factory can close over it. +const dbState = vi.hoisted(() => ({ rows: [] as Array> })); + +vi.mock('../src/lib/db', () => ({ + // getDb returns an sql() tagged-template that ignores the query and resolves + // the current dbState.rows — each vendor tool issues a single SELECT. + getDb: () => async () => dbState.rows, + typedRows: (rows: readonly Record[]): T[] => rows as unknown as T[], +})); + +import { mcpRoutes } from '../src/routes/mcp'; + +function makeEnv(): Pick & Partial { + return { + ENVIRONMENT: 'test', + COMMAND_KV: { + get: vi.fn().mockResolvedValue(null), + put: vi.fn().mockResolvedValue(undefined), + } as unknown as KVNamespace, + }; +} + +function buildApp() { + const app = new Hono<{ Bindings: Env; Variables: AuthVariables }>(); + app.use('/mcp/*', mcpAuthMiddleware); + app.route('/mcp', mcpRoutes); + const env = makeEnv(); + return async function callTool(name: string, args: Record = {}) { + const req = new Request('http://localhost/mcp', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name, arguments: args } }), + }); + const res = await app.fetch(req, env as unknown as Env); + const json = (await res.json()) as Record; + const result = json.result as { content: Array<{ text: string }>; isError?: boolean }; + return { isError: result.isError === true, data: JSON.parse(result.content[0].text) }; + }; +} + +// A vendor row as Neon returns it (snake_case, NUMERIC as strings). +function row(over: Partial>): Record { + return { + id: over.id ?? `id-${over.vendor_name}`, + vendor_name: over.vendor_name, + category: over.category ?? 'other', + billing_cycle: over.billing_cycle ?? 'monthly', + expected_amount: over.expected_amount ?? '10.00', + currency: 'USD', + next_bill_date: over.next_bill_date ?? null, + auto_pay: over.auto_pay ?? false, + payment_status: over.payment_status ?? 'active', + payment_method: null, + spending_limit: over.spending_limit ?? null, + mtd_spend: over.mtd_spend ?? null, + budget_limit: over.budget_limit ?? null, + status: over.status ?? 'active', + risk_score: over.risk_score ?? null, // deliberately stale/null to prove live recompute + }; +} + +// Fixture: 5 active vendors. Healthy/low first, high-risk LAST in array order, +// every stored risk_score null — so any reliance on stored order/score breaks. +const FIXTURE = [ + row({ vendor_name: 'healthy', payment_status: 'active', auto_pay: true, mtd_spend: '10', spending_limit: '100' }), // 0 (low) + row({ vendor_name: 'unknown-low', payment_status: 'unknown' }), // 5 (low) + row({ vendor_name: 'limited', payment_status: 'limited' }), // 35 (medium) + row({ vendor_name: 'failed-hi', payment_status: 'failed', mtd_spend: '200', spending_limit: '100' }), // 75 (critical) + row({ vendor_name: 'failed-50', payment_status: 'failed', mtd_spend: '50', spending_limit: '100' }), // 50 (high) +]; + +beforeEach(() => { + dbState.rows = FIXTURE.map((r) => ({ ...r })); +}); + +describe('query_vendors (MCP)', () => { + it('at_risk=true keeps high-risk vendors even when a small limit would page them out', async () => { + const callTool = buildApp(); + // limit=1: the buggy version applied LIMIT in SQL before filtering, which + // could drop the at-risk vendor entirely. Now: filter → sort → slice. + const { data } = await callTool('query_vendors', { at_risk: true, limit: 1 }); + expect(data.count).toBe(1); + // Highest live risk among the two at-risk vendors wins the single slot. + expect(data.vendors[0].vendor_name).toBe('failed-hi'); + expect(data.vendors[0].risk_score).toBe(75); + }); + + it('at_risk=true returns ALL vendors at/over threshold (not just the first page)', async () => { + const callTool = buildApp(); + const { data } = await callTool('query_vendors', { at_risk: true }); + expect(data.count).toBe(2); + expect(data.vendors.map((v: { vendor_name: string }) => v.vendor_name).sort()).toEqual(['failed-50', 'failed-hi']); + }); + + it('orders by LIVE risk, not the stale stored risk_score column', async () => { + const callTool = buildApp(); + const { data } = await callTool('query_vendors', { limit: 2 }); + expect(data.vendors.map((v: { vendor_name: string }) => v.vendor_name)).toEqual(['failed-hi', 'failed-50']); + }); + + it('recomputes risk live when stored risk_score is null', async () => { + const callTool = buildApp(); + const { data } = await callTool('query_vendors', {}); + const healthy = data.vendors.find((v: { vendor_name: string }) => v.vendor_name === 'healthy'); + expect(healthy.risk_score).toBe(0); + expect(healthy.risk_level).toBe('low'); + }); +}); + +describe('get_vendor_risk (MCP)', () => { + it('aggregates by risk level and lists the at-risk vendors', async () => { + const callTool = buildApp(); + const { data } = await callTool('get_vendor_risk', {}); + expect(data.vendor_count).toBe(5); + expect(data.by_level).toEqual({ critical: 1, high: 1, medium: 1, low: 2 }); + expect(data.at_risk).toHaveLength(2); + expect(data.at_risk[0].vendor_name).toBe('failed-hi'); // sorted desc by score + // total MTD spend: 10 + 200 + 50 = 260 (others null → 0) + expect(data.total_mtd_spend).toBe(260); + }); +}); diff --git a/tests/routes/vendors.spec.ts b/tests/routes/vendors.spec.ts new file mode 100644 index 0000000..51ede16 --- /dev/null +++ b/tests/routes/vendors.spec.ts @@ -0,0 +1,125 @@ +/** + * Integration tests for /api/vendors (vendor spend control). + * + * Real Neon. Skipped without DATABASE_URL — mirrors the established pattern in + * tests/routes/triage-roux.spec.ts (no-mocks rule per CLAUDE.md). The cc_vendors + * table is created by the vitest globalSetup (migration 0019 is registered in + * ADDITIVE_PREFIXES). + * + * Regression coverage for the two Codex findings on PR #119: + * - PATCH must persist `metadata` (it was accepted but never written). + * - at_risk filtering uses live recompute, not the stored risk_score. + * Plus the documented FULL-representation upsert semantics of POST. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { neon } from '@neondatabase/serverless'; +import { Hono } from 'hono'; +import type { Env } from '../../src/index'; +import { vendorRoutes } from '../../src/routes/vendors'; + +const DATABASE_URL = process.env.DATABASE_URL; +const SKIP = !DATABASE_URL || process.env.SKIP_INTEGRATION === '1'; +const TAG = `vtest-${Date.now()}-${Math.floor(Math.random() * 1e6)}`; + +const env = { DATABASE_URL } as unknown as Env; + +const app = new Hono<{ Bindings: Env }>(); +app.route('/api/vendors', vendorRoutes); + +async function api(method: string, path: string, body?: unknown) { + const req = new Request(`http://localhost/api/vendors${path}`, { + method, + headers: { 'Content-Type': 'application/json' }, + body: body === undefined ? undefined : JSON.stringify(body), + }); + const res = await app.fetch(req, env); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const json = (await res.json().catch(() => null)) as any; + return { status: res.status, json }; +} + +async function cleanup() { + if (!DATABASE_URL) return; + const sql = neon(DATABASE_URL); + await sql`DELETE FROM cc_vendors WHERE vendor_name LIKE ${TAG + '%'}`; +} + +describe.skipIf(SKIP)('/api/vendors (real Neon)', () => { + beforeAll(cleanup); + afterAll(cleanup); + + it('POST creates a vendor and computes a risk score', async () => { + const { status, json } = await api('POST', '/', { + vendor_name: `${TAG}-create`, + category: 'infra', + billing_cycle: 'monthly', + expected_amount: 25, + payment_status: 'active', + auto_pay: true, + }); + expect(status).toBe(201); + expect(json.id).toBeTruthy(); + expect(typeof json.risk_score).toBe('number'); + }); + + it('PATCH persists metadata (regression: was accepted but never written)', async () => { + const created = await api('POST', '/', { vendor_name: `${TAG}-meta`, category: 'data' }); + expect(created.status).toBe(201); + const id = created.json.id; + + const patched = await api('PATCH', `/${id}`, { metadata: { owner_note: 'hello', team: 'ops' } }); + expect(patched.status).toBe(200); + + const got = await api('GET', `/${id}`); + expect(got.status).toBe(200); + expect(got.json.metadata).toMatchObject({ owner_note: 'hello', team: 'ops' }); + }); + + it('GET ?at_risk=true filters by live risk (failed in, healthy out)', async () => { + await api('POST', '/', { + vendor_name: `${TAG}-failed`, + category: 'ai_inference', + payment_status: 'failed', // → risk 50 (at risk) + }); + await api('POST', '/', { + vendor_name: `${TAG}-healthy`, + category: 'dev_tooling', + payment_status: 'active', + auto_pay: true, // → risk 0 (not at risk) + }); + + const { status, json } = await api('GET', '/?at_risk=true'); + expect(status).toBe(200); + const names: string[] = json.vendors.map((v: { vendor_name: string }) => v.vendor_name); + expect(names).toContain(`${TAG}-failed`); + expect(names).not.toContain(`${TAG}-healthy`); + }); + + it('POST is a FULL upsert: a partial re-POST clobbers omitted fields to defaults', async () => { + const name = `${TAG}-upsert`; + const first = await api('POST', '/', { + vendor_name: name, + category: 'infra', + payment_status: 'failed', + mtd_spend: 200, + }); + expect(first.status).toBe(201); + expect(parseFloat(first.json.mtd_spend)).toBe(200); + + // Re-POST with only name+category — documented behaviour resets the rest. + const second = await api('POST', '/', { vendor_name: name, category: 'data' }); + expect(second.status).toBe(201); + expect(second.json.category).toBe('data'); + expect(parseFloat(second.json.mtd_spend)).toBe(0); // clobbered to default + expect(second.json.payment_status).toBe('unknown'); // clobbered to default + }); + + it('GET /summary returns spend rollups', async () => { + const { status, json } = await api('GET', '/summary'); + expect(status).toBe(200); + expect(typeof json.total_mtd_spend).toBe('number'); + expect(json.by_level).toHaveProperty('critical'); + expect(Array.isArray(json.at_risk)).toBe(true); + expect(Array.isArray(json.upcoming_bills)).toBe(true); + }); +}); From b347c8a1679ce5de13627e478dd45b62398a4203 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 30 Jun 2026 15:26:32 +0000 Subject: [PATCH 2/2] fix(vendors): POST upsert replaces metadata on conflict (CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ON CONFLICT was the one column omitted from the full-overwrite upsert, so a re-POST kept stale metadata — contradicting the documented "full representation" contract. Add `metadata = EXCLUDED.metadata` and assert the clobber (set then reset) in the route spec. https://claude.ai/code/session_015mkdG1VYH3AdqLe4E3i9H6 --- src/routes/vendors.ts | 1 + tests/routes/vendors.spec.ts | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/routes/vendors.ts b/src/routes/vendors.ts index d61153c..6832e1e 100644 --- a/src/routes/vendors.ts +++ b/src/routes/vendors.ts @@ -146,6 +146,7 @@ vendorRoutes.post('/', async (c) => { status = EXCLUDED.status, owner = EXCLUDED.owner, account_id = EXCLUDED.account_id, + metadata = EXCLUDED.metadata, risk_score = EXCLUDED.risk_score, updated_at = NOW() RETURNING * diff --git a/tests/routes/vendors.spec.ts b/tests/routes/vendors.spec.ts index 51ede16..c55e16f 100644 --- a/tests/routes/vendors.spec.ts +++ b/tests/routes/vendors.spec.ts @@ -102,16 +102,20 @@ describe.skipIf(SKIP)('/api/vendors (real Neon)', () => { category: 'infra', payment_status: 'failed', mtd_spend: 200, + metadata: { note: 'first' }, }); expect(first.status).toBe(201); expect(parseFloat(first.json.mtd_spend)).toBe(200); + expect(first.json.metadata).toEqual({ note: 'first' }); - // Re-POST with only name+category — documented behaviour resets the rest. + // Re-POST with only name+category — documented behaviour resets the rest, + // including metadata (ON CONFLICT now assigns metadata = EXCLUDED.metadata). const second = await api('POST', '/', { vendor_name: name, category: 'data' }); expect(second.status).toBe(201); expect(second.json.category).toBe('data'); expect(parseFloat(second.json.mtd_spend)).toBe(0); // clobbered to default expect(second.json.payment_status).toBe('unknown'); // clobbered to default + expect(second.json.metadata).toEqual({}); // clobbered to default (was {note:'first'}) }); it('GET /summary returns spend rollups', async () => {