Skip to content

Commit e6f5a25

Browse files
committed
test(vendors): regression tests + AT_RISK_THRESHOLD + live-risk sort
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
1 parent efeb9c2 commit e6f5a25

6 files changed

Lines changed: 280 additions & 10 deletions

File tree

src/lib/cron.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { generatePaymentPlan, savePaymentPlan } from './payment-planner';
99
import { reconcileNotionDisputes } from './dispute-sync';
1010
import { enqueueJob, processQueue, type ScrapeJobType } from './job-dispatcher';
1111
import { decayStaleRouxIntents } from './intent-decay';
12-
import { computeVendorRisk, vendorRiskInputFromRow } from './vendor-risk';
12+
import { computeVendorRisk, vendorRiskInputFromRow, AT_RISK_THRESHOLD } from './vendor-risk';
1313

1414
/**
1515
* Cron sync orchestrator.
@@ -267,7 +267,7 @@ async function sweepVendorRisk(
267267
const { score } = computeVendorRisk(vendorRiskInputFromRow(r));
268268
ids.push(r.id as string);
269269
scores.push(score);
270-
if (score >= 50) atRisk++;
270+
if (score >= AT_RISK_THRESHOLD) atRisk++;
271271
}
272272
await sql`
273273
UPDATE cc_vendors SET risk_score = bulk.score, updated_at = NOW()

src/lib/vendor-risk.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@
1313
*/
1414
import { urgencyLevel, type UrgencyLevel } from './urgency';
1515

16+
// Score at/above which a vendor is "at risk" (high/critical). Mirrors the
17+
// urgencyLevel 'high' boundary; centralised so routes, MCP, and cron agree.
18+
export const AT_RISK_THRESHOLD = 50;
19+
1620
export type VendorPaymentStatus = 'active' | 'failed' | 'limited' | 'unknown';
1721
export type VendorStatus = 'active' | 'paused' | 'cancelled' | 'zombie';
1822

src/routes/mcp.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ const TRIAGE_TOOL_NAMES = new Set([
1414
]);
1515
import { getDb, typedRows } from '../lib/db';
1616
import type { NeonQueryFunction } from '@neondatabase/serverless';
17-
import { computeVendorRisk, vendorRiskInputFromRow, numOrNull } from '../lib/vendor-risk';
17+
import { computeVendorRisk, vendorRiskInputFromRow, numOrNull, AT_RISK_THRESHOLD } from '../lib/vendor-risk';
1818
import { listJobs, getJobStatus, retryJob, getDeadLetters, enqueueJob } from '../lib/job-dispatcher';
1919
import type { ScrapeJobType, ScrapeJobStatus } from '../lib/job-dispatcher';
2020
import { evidenceClient, ledgerClient, govClient } from '../lib/integrations';
@@ -1163,7 +1163,7 @@ async function executeTool(env: Env, sql: NeonQueryFunction<false, false>, toolN
11631163
const risk = computeVendorRisk(vendorRiskInputFromRow(r));
11641164
return { ...r, risk_score: risk.score, risk_level: risk.level, risk_reasons: risk.reasons };
11651165
});
1166-
if (atRisk) vendors = vendors.filter((v) => (v.risk_score as number) >= 50);
1166+
if (atRisk) vendors = vendors.filter((v) => (v.risk_score as number) >= AT_RISK_THRESHOLD);
11671167
vendors.sort((a, b) => (b.risk_score as number) - (a.risk_score as number));
11681168
const limited = vendors.slice(0, limit);
11691169
return { count: limited.length, vendors: limited };
@@ -1183,7 +1183,7 @@ async function executeTool(env: Env, sql: NeonQueryFunction<false, false>, toolN
11831183
if (r.billing_cycle === 'monthly') monthlyCommitted += numOrNull(r.expected_amount) ?? 0;
11841184
const risk = computeVendorRisk(vendorRiskInputFromRow(r));
11851185
byLevel[risk.level] = (byLevel[risk.level] || 0) + 1;
1186-
if (risk.score >= 50) atRisk.push({ vendor_name: r.vendor_name, category: r.category, score: risk.score, level: risk.level, reasons: risk.reasons });
1186+
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 });
11871187
}
11881188
atRisk.sort((a, b) => b.score - a.score);
11891189
return {

src/routes/vendors.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Hono } from 'hono';
22
import type { Env } from '../index';
33
import { getDb } from '../lib/db';
4-
import { computeVendorRisk, vendorRiskInputFromRow, numOrNull } from '../lib/vendor-risk';
4+
import { computeVendorRisk, vendorRiskInputFromRow, numOrNull, AT_RISK_THRESHOLD } from '../lib/vendor-risk';
55
import { createVendorSchema, updateVendorSchema, vendorQuerySchema } from '../lib/validators';
66

77
export const vendorRoutes = new Hono<{ Bindings: Env }>();
@@ -27,8 +27,11 @@ vendorRoutes.get('/', async (c) => {
2727
AND (${status}::text IS NULL OR status = ${status})
2828
ORDER BY risk_score DESC NULLS LAST, next_bill_date ASC NULLS LAST, vendor_name ASC
2929
`;
30-
const vendors = rows.map((r) => ({ ...r, risk: computeVendorRisk(vendorRiskInputFromRow(r)) }));
31-
const filtered = atRisk ? vendors.filter((v) => v.risk.score >= 50) : vendors;
30+
const vendors = rows
31+
.map((r) => ({ ...r, risk: computeVendorRisk(vendorRiskInputFromRow(r)) }))
32+
// Sort by *live* risk, not the stored (possibly stale) risk_score column.
33+
.sort((a, b) => b.risk.score - a.risk.score);
34+
const filtered = atRisk ? vendors.filter((v) => v.risk.score >= AT_RISK_THRESHOLD) : vendors;
3235
return c.json({ count: filtered.length, vendors: filtered });
3336
});
3437

@@ -62,7 +65,7 @@ vendorRoutes.get('/summary', async (c) => {
6265

6366
const risk = computeVendorRisk(vendorRiskInputFromRow(r));
6467
byLevel[risk.level] = (byLevel[risk.level] || 0) + 1;
65-
if (risk.score >= 50) {
68+
if (risk.score >= AT_RISK_THRESHOLD) {
6669
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 });
6770
}
6871
if (r.status === 'zombie') {
@@ -104,7 +107,10 @@ vendorRoutes.get('/:id', async (c) => {
104107
return c.json({ ...row, risk: computeVendorRisk(vendorRiskInputFromRow(row)) });
105108
});
106109

107-
// Create or upsert a vendor (keyed on vendor_name). POST is a full representation.
110+
// Create or upsert a vendor (keyed on vendor_name). POST is a FULL representation:
111+
// on conflict every column is overwritten from this payload, and omitted fields
112+
// fall back to their defaults (e.g. mtd_spend→0, payment_status→'unknown'). Use
113+
// PATCH for partial updates so existing spend/status data isn't clobbered.
108114
vendorRoutes.post('/', async (c) => {
109115
const raw = await c.req.json();
110116
const result = createVendorSchema.safeParse(raw);

tests/mcp-vendors.test.ts

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/**
2+
* Hermetic regression tests for the vendor MCP tools (query_vendors,
3+
* get_vendor_risk). getDb is mocked (same approach as tests/mcp.test.ts) so
4+
* these run without a database and pin the exact bug Codex flagged on #119:
5+
* at_risk filtering + risk recompute must happen BEFORE limiting, and ordering
6+
* must use live risk — not the stored (possibly stale) risk_score column.
7+
*/
8+
import { describe, it, expect, vi, beforeEach } from 'vitest';
9+
import { Hono } from 'hono';
10+
import type { Env } from '../src/index';
11+
import { mcpAuthMiddleware } from '../src/middleware/auth';
12+
import type { AuthVariables } from '../src/middleware/auth';
13+
14+
// Mutable row set the mocked sql tagged-template resolves to. Hoisted so the
15+
// vi.mock factory can close over it.
16+
const dbState = vi.hoisted(() => ({ rows: [] as Array<Record<string, unknown>> }));
17+
18+
vi.mock('../src/lib/db', () => ({
19+
// getDb returns an sql() tagged-template that ignores the query and resolves
20+
// the current dbState.rows — each vendor tool issues a single SELECT.
21+
getDb: () => async () => dbState.rows,
22+
typedRows: <T>(rows: readonly Record<string, unknown>[]): T[] => rows as unknown as T[],
23+
}));
24+
25+
import { mcpRoutes } from '../src/routes/mcp';
26+
27+
function makeEnv(): Pick<Env, 'ENVIRONMENT' | 'COMMAND_KV'> & Partial<Env> {
28+
return {
29+
ENVIRONMENT: 'test',
30+
COMMAND_KV: {
31+
get: vi.fn().mockResolvedValue(null),
32+
put: vi.fn().mockResolvedValue(undefined),
33+
} as unknown as KVNamespace,
34+
};
35+
}
36+
37+
function buildApp() {
38+
const app = new Hono<{ Bindings: Env; Variables: AuthVariables }>();
39+
app.use('/mcp/*', mcpAuthMiddleware);
40+
app.route('/mcp', mcpRoutes);
41+
const env = makeEnv();
42+
return async function callTool(name: string, args: Record<string, unknown> = {}) {
43+
const req = new Request('http://localhost/mcp', {
44+
method: 'POST',
45+
headers: { 'Content-Type': 'application/json' },
46+
body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name, arguments: args } }),
47+
});
48+
const res = await app.fetch(req, env as unknown as Env);
49+
const json = (await res.json()) as Record<string, unknown>;
50+
const result = json.result as { content: Array<{ text: string }>; isError?: boolean };
51+
return { isError: result.isError === true, data: JSON.parse(result.content[0].text) };
52+
};
53+
}
54+
55+
// A vendor row as Neon returns it (snake_case, NUMERIC as strings).
56+
function row(over: Partial<Record<string, unknown>>): Record<string, unknown> {
57+
return {
58+
id: over.id ?? `id-${over.vendor_name}`,
59+
vendor_name: over.vendor_name,
60+
category: over.category ?? 'other',
61+
billing_cycle: over.billing_cycle ?? 'monthly',
62+
expected_amount: over.expected_amount ?? '10.00',
63+
currency: 'USD',
64+
next_bill_date: over.next_bill_date ?? null,
65+
auto_pay: over.auto_pay ?? false,
66+
payment_status: over.payment_status ?? 'active',
67+
payment_method: null,
68+
spending_limit: over.spending_limit ?? null,
69+
mtd_spend: over.mtd_spend ?? null,
70+
budget_limit: over.budget_limit ?? null,
71+
status: over.status ?? 'active',
72+
risk_score: over.risk_score ?? null, // deliberately stale/null to prove live recompute
73+
};
74+
}
75+
76+
// Fixture: 5 active vendors. Healthy/low first, high-risk LAST in array order,
77+
// every stored risk_score null — so any reliance on stored order/score breaks.
78+
const FIXTURE = [
79+
row({ vendor_name: 'healthy', payment_status: 'active', auto_pay: true, mtd_spend: '10', spending_limit: '100' }), // 0 (low)
80+
row({ vendor_name: 'unknown-low', payment_status: 'unknown' }), // 5 (low)
81+
row({ vendor_name: 'limited', payment_status: 'limited' }), // 35 (medium)
82+
row({ vendor_name: 'failed-hi', payment_status: 'failed', mtd_spend: '200', spending_limit: '100' }), // 75 (critical)
83+
row({ vendor_name: 'failed-50', payment_status: 'failed', mtd_spend: '50', spending_limit: '100' }), // 50 (high)
84+
];
85+
86+
beforeEach(() => {
87+
dbState.rows = FIXTURE.map((r) => ({ ...r }));
88+
});
89+
90+
describe('query_vendors (MCP)', () => {
91+
it('at_risk=true keeps high-risk vendors even when a small limit would page them out', async () => {
92+
const callTool = buildApp();
93+
// limit=1: the buggy version applied LIMIT in SQL before filtering, which
94+
// could drop the at-risk vendor entirely. Now: filter → sort → slice.
95+
const { data } = await callTool('query_vendors', { at_risk: true, limit: 1 });
96+
expect(data.count).toBe(1);
97+
// Highest live risk among the two at-risk vendors wins the single slot.
98+
expect(data.vendors[0].vendor_name).toBe('failed-hi');
99+
expect(data.vendors[0].risk_score).toBe(75);
100+
});
101+
102+
it('at_risk=true returns ALL vendors at/over threshold (not just the first page)', async () => {
103+
const callTool = buildApp();
104+
const { data } = await callTool('query_vendors', { at_risk: true });
105+
expect(data.count).toBe(2);
106+
expect(data.vendors.map((v: { vendor_name: string }) => v.vendor_name).sort()).toEqual(['failed-50', 'failed-hi']);
107+
});
108+
109+
it('orders by LIVE risk, not the stale stored risk_score column', async () => {
110+
const callTool = buildApp();
111+
const { data } = await callTool('query_vendors', { limit: 2 });
112+
expect(data.vendors.map((v: { vendor_name: string }) => v.vendor_name)).toEqual(['failed-hi', 'failed-50']);
113+
});
114+
115+
it('recomputes risk live when stored risk_score is null', async () => {
116+
const callTool = buildApp();
117+
const { data } = await callTool('query_vendors', {});
118+
const healthy = data.vendors.find((v: { vendor_name: string }) => v.vendor_name === 'healthy');
119+
expect(healthy.risk_score).toBe(0);
120+
expect(healthy.risk_level).toBe('low');
121+
});
122+
});
123+
124+
describe('get_vendor_risk (MCP)', () => {
125+
it('aggregates by risk level and lists the at-risk vendors', async () => {
126+
const callTool = buildApp();
127+
const { data } = await callTool('get_vendor_risk', {});
128+
expect(data.vendor_count).toBe(5);
129+
expect(data.by_level).toEqual({ critical: 1, high: 1, medium: 1, low: 2 });
130+
expect(data.at_risk).toHaveLength(2);
131+
expect(data.at_risk[0].vendor_name).toBe('failed-hi'); // sorted desc by score
132+
// total MTD spend: 10 + 200 + 50 = 260 (others null → 0)
133+
expect(data.total_mtd_spend).toBe(260);
134+
});
135+
});

tests/routes/vendors.spec.ts

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
/**
2+
* Integration tests for /api/vendors (vendor spend control).
3+
*
4+
* Real Neon. Skipped without DATABASE_URL — mirrors the established pattern in
5+
* tests/routes/triage-roux.spec.ts (no-mocks rule per CLAUDE.md). The cc_vendors
6+
* table is created by the vitest globalSetup (migration 0019 is registered in
7+
* ADDITIVE_PREFIXES).
8+
*
9+
* Regression coverage for the two Codex findings on PR #119:
10+
* - PATCH must persist `metadata` (it was accepted but never written).
11+
* - at_risk filtering uses live recompute, not the stored risk_score.
12+
* Plus the documented FULL-representation upsert semantics of POST.
13+
*/
14+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
15+
import { neon } from '@neondatabase/serverless';
16+
import { Hono } from 'hono';
17+
import type { Env } from '../../src/index';
18+
import { vendorRoutes } from '../../src/routes/vendors';
19+
20+
const DATABASE_URL = process.env.DATABASE_URL;
21+
const SKIP = !DATABASE_URL || process.env.SKIP_INTEGRATION === '1';
22+
const TAG = `vtest-${Date.now()}-${Math.floor(Math.random() * 1e6)}`;
23+
24+
const env = { DATABASE_URL } as unknown as Env;
25+
26+
const app = new Hono<{ Bindings: Env }>();
27+
app.route('/api/vendors', vendorRoutes);
28+
29+
async function api(method: string, path: string, body?: unknown) {
30+
const req = new Request(`http://localhost/api/vendors${path}`, {
31+
method,
32+
headers: { 'Content-Type': 'application/json' },
33+
body: body === undefined ? undefined : JSON.stringify(body),
34+
});
35+
const res = await app.fetch(req, env);
36+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
37+
const json = (await res.json().catch(() => null)) as any;
38+
return { status: res.status, json };
39+
}
40+
41+
async function cleanup() {
42+
if (!DATABASE_URL) return;
43+
const sql = neon(DATABASE_URL);
44+
await sql`DELETE FROM cc_vendors WHERE vendor_name LIKE ${TAG + '%'}`;
45+
}
46+
47+
describe.skipIf(SKIP)('/api/vendors (real Neon)', () => {
48+
beforeAll(cleanup);
49+
afterAll(cleanup);
50+
51+
it('POST creates a vendor and computes a risk score', async () => {
52+
const { status, json } = await api('POST', '/', {
53+
vendor_name: `${TAG}-create`,
54+
category: 'infra',
55+
billing_cycle: 'monthly',
56+
expected_amount: 25,
57+
payment_status: 'active',
58+
auto_pay: true,
59+
});
60+
expect(status).toBe(201);
61+
expect(json.id).toBeTruthy();
62+
expect(typeof json.risk_score).toBe('number');
63+
});
64+
65+
it('PATCH persists metadata (regression: was accepted but never written)', async () => {
66+
const created = await api('POST', '/', { vendor_name: `${TAG}-meta`, category: 'data' });
67+
expect(created.status).toBe(201);
68+
const id = created.json.id;
69+
70+
const patched = await api('PATCH', `/${id}`, { metadata: { owner_note: 'hello', team: 'ops' } });
71+
expect(patched.status).toBe(200);
72+
73+
const got = await api('GET', `/${id}`);
74+
expect(got.status).toBe(200);
75+
expect(got.json.metadata).toMatchObject({ owner_note: 'hello', team: 'ops' });
76+
});
77+
78+
it('GET ?at_risk=true filters by live risk (failed in, healthy out)', async () => {
79+
await api('POST', '/', {
80+
vendor_name: `${TAG}-failed`,
81+
category: 'ai_inference',
82+
payment_status: 'failed', // → risk 50 (at risk)
83+
});
84+
await api('POST', '/', {
85+
vendor_name: `${TAG}-healthy`,
86+
category: 'dev_tooling',
87+
payment_status: 'active',
88+
auto_pay: true, // → risk 0 (not at risk)
89+
});
90+
91+
const { status, json } = await api('GET', '/?at_risk=true');
92+
expect(status).toBe(200);
93+
const names: string[] = json.vendors.map((v: { vendor_name: string }) => v.vendor_name);
94+
expect(names).toContain(`${TAG}-failed`);
95+
expect(names).not.toContain(`${TAG}-healthy`);
96+
});
97+
98+
it('POST is a FULL upsert: a partial re-POST clobbers omitted fields to defaults', async () => {
99+
const name = `${TAG}-upsert`;
100+
const first = await api('POST', '/', {
101+
vendor_name: name,
102+
category: 'infra',
103+
payment_status: 'failed',
104+
mtd_spend: 200,
105+
});
106+
expect(first.status).toBe(201);
107+
expect(parseFloat(first.json.mtd_spend)).toBe(200);
108+
109+
// Re-POST with only name+category — documented behaviour resets the rest.
110+
const second = await api('POST', '/', { vendor_name: name, category: 'data' });
111+
expect(second.status).toBe(201);
112+
expect(second.json.category).toBe('data');
113+
expect(parseFloat(second.json.mtd_spend)).toBe(0); // clobbered to default
114+
expect(second.json.payment_status).toBe('unknown'); // clobbered to default
115+
});
116+
117+
it('GET /summary returns spend rollups', async () => {
118+
const { status, json } = await api('GET', '/summary');
119+
expect(status).toBe(200);
120+
expect(typeof json.total_mtd_spend).toBe('number');
121+
expect(json.by_level).toHaveProperty('critical');
122+
expect(Array.isArray(json.at_risk)).toBe(true);
123+
expect(Array.isArray(json.upcoming_bills)).toBe(true);
124+
});
125+
});

0 commit comments

Comments
 (0)