test(vendors): regression tests + AT_RISK_THRESHOLD + live-risk sort (follow-up to #119)#129
test(vendors): regression tests + AT_RISK_THRESHOLD + live-risk sort (follow-up to #119)#129chitcommit wants to merge 2 commits into
Conversation
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
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughExtracts the hardcoded at-risk score cutoff into ChangesAT_RISK_THRESHOLD Extraction and Consumer Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
To use Codex here, create a Codex account and connect to github. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
chittycommand | b347c8a | Jun 30 2026, 03:27 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/vendors.ts`:
- Around line 110-113: The POST upsert behavior in the vendor create route is
documented as a full overwrite, but the conflict path does not replace metadata,
so existing metadata can remain stale after re-posting. Update the upsert logic
in the vendors route so the ON CONFLICT branch also assigns metadata from the
incoming payload, and adjust the surrounding POST docs/comments to match the
actual behavior of the create/upsert flow.
In `@tests/mcp-vendors.test.ts`:
- Around line 49-51: Update the MCP test helper to validate the structured JSON
response shape instead of the legacy text payload. In the helper that reads
`res.json()` and returns `data`, assert that `json.result.content[0].type` is
`json` and parse the payload from `content[0].json` rather than
`content[0].text`. Keep the helper aligned with the `src/routes/mcp.ts` contract
so the suite fails unless the MCP tool returns `content: [{ type: "json", json:
... }]`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0eab47fa-7781-4b25-a594-f4d848d4496c
📒 Files selected for processing (6)
src/lib/cron.tssrc/lib/vendor-risk.tssrc/routes/mcp.tssrc/routes/vendors.tstests/mcp-vendors.test.tstests/routes/vendors.spec.ts
| const json = (await res.json()) as Record<string, unknown>; | ||
| const result = json.result as { content: Array<{ text: string }>; isError?: boolean }; | ||
| return { isError: result.isError === true, data: JSON.parse(result.content[0].text) }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the structured MCP JSON payload here.
Lines 50-51 lock this helper to the legacy content[0].text shape, so the suite will still pass against a non-compliant MCP handler and will fail once src/routes/mcp.ts is corrected to the documented content: [{ type: "json", json: ... }] contract. Read content[0].json and assert type === 'json' instead.
As per coding guidelines, src/routes/mcp.ts MCP tools must return structured JSON using content: [{ type: "json", json: ... }] format.
Suggested fix
- const result = json.result as { content: Array<{ text: string }>; isError?: boolean };
- return { isError: result.isError === true, data: JSON.parse(result.content[0].text) };
+ const result = json.result as {
+ content: Array<{ type: 'json'; json: unknown }>;
+ isError?: boolean;
+ };
+ expect(result.content[0]?.type).toBe('json');
+ return { isError: result.isError === true, data: result.content[0]?.json };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const json = (await res.json()) as Record<string, unknown>; | |
| const result = json.result as { content: Array<{ text: string }>; isError?: boolean }; | |
| return { isError: result.isError === true, data: JSON.parse(result.content[0].text) }; | |
| const json = (await res.json()) as Record<string, unknown>; | |
| const result = json.result as { | |
| content: Array<{ type: 'json'; json: unknown }>; | |
| isError?: boolean; | |
| }; | |
| expect(result.content[0]?.type).toBe('json'); | |
| return { isError: result.isError === true, data: result.content[0]?.json }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/mcp-vendors.test.ts` around lines 49 - 51, Update the MCP test helper
to validate the structured JSON response shape instead of the legacy text
payload. In the helper that reads `res.json()` and returns `data`, assert that
`json.result.content[0].type` is `json` and parse the payload from
`content[0].json` rather than `content[0].text`. Keep the helper aligned with
the `src/routes/mcp.ts` contract so the suite fails unless the MCP tool returns
`content: [{ type: "json", json: ... }]`.
Source: Coding guidelines
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
|
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
|
Thanks @coderabbitai — both findings reviewed:
Separately, the red Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
Agreed on both points:
Also understood on the dependency-audit failures being pre-existing on If you want, I can open a small follow-up issue for the separate MCP response-shape migration ( (•ᴗ•)🐇 ✏️ Learnings added
|
2 participants
Why
Follow-up to the merged vendor spend-control feature (#119). A self-review surfaced gaps that weren't blocking but are worth closing — chiefly that the two bugs Codex caught on #119 (PATCH-metadata persistence, at_risk-before-limit) shipped without a regression test. This PR adds that coverage plus two small polish fixes. No behavior change to the risk math.
What
Tests (the main point)
tests/mcp-vendors.test.ts— hermetic regression (mocksgetDb, same approach astests/mcp.test.ts, so it runs everywhere). Pins thequery_vendorsfix: fetch → recompute risk live → filterat_risk→ sort by live score → then slice, so a high-risk vendor is never paged out by a smalllimit, and ordering uses live risk even when the storedrisk_scoreis null/stale. Also coversget_vendor_risklevel aggregation.tests/routes/vendors.spec.ts— real-Neon route spec (describe.skipIf(!DATABASE_URL), per the repo's no-mocks rule;cc_vendorsis created by the vitest globalSetup since0019_is inADDITIVE_PREFIXES). Covers POST create, PATCH metadata persistence (the exact feat(finance): vendor spend control — cc_vendors + deterministic spend-risk #119 bug),at_riskfiltering (failed in / healthy out), the documented clobbering upsert, and/summary.Polish
AT_RISK_THRESHOLDconstant invendor-risk.tsreplaces the duplicated>= 50magic number across the route, both MCP tools, and the cron sweep.GET /api/vendorsnow sorts by live risk, not the stale storedrisk_scorecolumn. (The MCPquery_vendorspath already did after feat(finance): vendor spend control — cc_vendors + deterministic spend-risk #119's fix; the HTTP route still ordered on the column.)vendor_nameconflict every column is overwritten and omitted fields fall back to defaults (mtd_spend→0,payment_status→'unknown'), so a partial re-POST silently wipes spend data. Use PATCH for partial updates. (Doc comment only; behavior unchanged to avoid breaking the merged contract.)Testing
npm run typecheck— clean.npx vitest run tests/mcp-vendors.test.ts tests/mcp.test.ts tests/lib/vendor-risk.spec.ts tests/routes/vendors.spec.ts— 57 passed, 5 skipped (the route spec skips withoutDATABASE_URL, consistent with every other integration spec in the repo; it runs wherever a Neon branch is configured).Not addressed here (intentionally)
:id→ HTTP 500 (Postgres uuid cast) instead of 400/404 — consistent with the existing obligations routes; left alone rather than diverging in one place.chittyagent-financecross-repo lane).https://claude.ai/code/session_015mkdG1VYH3AdqLe4E3i9H6
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests