From afd7af70c82c506a81d6c77169229ef332cbe463 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:24:17 -0400 Subject: [PATCH 1/5] test(18-04): add failing regression test for campaign report_count double-increment - Sibling report already in campaign-1 re-matches via Tier 3; asserts zero UPDATE campaigns / INSERT campaigns / UPDATE reports calls (CAMP-02/CR-01) --- .../campaign-grouping-service.test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/lib/services/campaign-grouping-service.test.ts b/lib/services/campaign-grouping-service.test.ts index e0e5d21..b507662 100644 --- a/lib/services/campaign-grouping-service.test.ts +++ b/lib/services/campaign-grouping-service.test.ts @@ -333,4 +333,31 @@ describe('groupReportIntoCampaign', () => { // a second time for this report re-run. expect(callsContaining('UPDATE campaigns')).toHaveLength(0); }); + + it('re-analyzing a report already linked to a campaign that a sibling report re-matches does not increment report_count a second time', async () => { + // Own report is already linked to campaign-1. A SIBLING report (not this + // report's own excluded row) also belongs to campaign-1 and matches via + // Tier 3 (sender + subject + client). This is the CAMP-02/CR-01 gap: + // tier queries only exclude the report's OWN row, not siblings already + // in the same campaign, so a naive re-match would double-increment. + stage({ + ownReport: [{ ...REPORT_ROW, campaign_id: 'campaign-1' }], + ownMessage: [], + tier3: [{ campaign_id: 'campaign-1', title: 'Invoice Alert' }], + }); + + const result = await groupReportIntoCampaign('report-self'); + + expect(result).toEqual({ + campaignId: 'campaign-1', + groupMethod: 'sender_subject_client', + created: false, + }); + + // Critical assertions: no double-increment, no phantom new campaign, no + // redundant re-link. + expect(callsContaining('UPDATE campaigns')).toHaveLength(0); + expect(callsContaining('INSERT INTO campaigns')).toHaveLength(0); + expect(callsContaining('UPDATE reports SET campaign_id')).toHaveLength(0); + }); }); From 9ca2ccf1c7e681cd5e07c5b9fea30b0c65d88f23 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:25:04 -0400 Subject: [PATCH 2/5] fix(18-04): short-circuit own-campaign re-match to stop report_count double-increment - OwnReportRow now selects campaign_id::text; match branch no-ops (created: false, no UPDATE) when the tiered match resolves to the report's own current campaign_id via a sibling row - Closes CAMP-02 gap / CR-01: /analyze can be re-run indefinitely without inflating campaigns.report_count --- lib/services/campaign-grouping-service.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/services/campaign-grouping-service.ts b/lib/services/campaign-grouping-service.ts index 39ba1df..ee3a2d1 100644 --- a/lib/services/campaign-grouping-service.ts +++ b/lib/services/campaign-grouping-service.ts @@ -66,6 +66,7 @@ interface OwnReportRow { requester_contact_id: number | null; company_id: number | null; created_at: string; + campaign_id: string | null; } interface OwnMessageRow { @@ -154,7 +155,7 @@ export async function groupReportIntoCampaign( return await postgresClient.transaction(async (client) => { const ownReportRes = await client.query( - `SELECT title, requester_contact_id, company_id, created_at + `SELECT title, requester_contact_id, company_id, created_at, campaign_id::text AS campaign_id FROM reports WHERE id = $1`, [reportId] @@ -321,6 +322,16 @@ export async function groupReportIntoCampaign( // Find-or-create against `campaigns` (CAMP-02). // --------------------------------------------------------------------- if (matchCampaignId && matchGroupMethod) { + // CR-01/CAMP-02 no-op guard: tier queries only exclude the report's + // OWN row (`r.id != $x`), not its siblings already in the same + // campaign. Since `/analyze` re-runs grouping unconditionally + // (D-08), a report already linked to campaign X can re-match X via + // a sibling row on every repeat call, inflating `report_count` + // without bound. If the match resolves to the report's own current + // campaign, it's already correctly linked — skip both UPDATEs. + if (matchCampaignId === ownReport.campaign_id) { + return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; + } await client.query( `UPDATE campaigns SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW() From abe3d4b900452ab4776e37cfe14d9d74a8f43896 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:25:29 -0400 Subject: [PATCH 3/5] fix(18-04): clamp campaigns list limit param to [0, 200] (WR-02) - Parse limit once with Number.isFinite instead of `|| 50`, so an explicit limit=0 is honored instead of silently replaced by the default - Math.max/min clamps to [0, 200], preventing a negative limit from reaching the SQL LIMIT clause and raising an unhandled 500 --- app/api/phishing/campaigns/route.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/api/phishing/campaigns/route.ts b/app/api/phishing/campaigns/route.ts index 56e8c04..aac35d2 100644 --- a/app/api/phishing/campaigns/route.ts +++ b/app/api/phishing/campaigns/route.ts @@ -28,7 +28,8 @@ export async function GET(request: NextRequest) { try { const url = request.nextUrl; - const limit = Math.min(parseInt(url.searchParams.get('limit') ?? '50', 10) || 50, 200); + const rawLimit = parseInt(url.searchParams.get('limit') ?? '50', 10); + const limit = Math.min(Math.max(Number.isFinite(rawLimit) ? rawLimit : 50, 0), 200); const offset = Math.max(parseInt(url.searchParams.get('offset') ?? '0', 10) || 0, 0); const status = url.searchParams.get('status'); From 21fde9eb3d2ab3f4c58f26d55eaee619044cc180 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:26:21 -0400 Subject: [PATCH 4/5] docs(18-04): complete gap-closure plan summary - CAMP-02 report_count double-increment fix and WR-02 limit-clamp fix, with a passing regression test and clean type check --- .../18-04-SUMMARY.md | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 .planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md new file mode 100644 index 0000000..9aa9f0d --- /dev/null +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md @@ -0,0 +1,106 @@ +--- +phase: 18-campaign-grouping-phishing-analysis-api +plan: 04 +subsystem: api +tags: [postgres, vitest, tdd, campaign-grouping, phishing] + +# Dependency graph +requires: + - phase: 18-01 + provides: groupReportIntoCampaign tiered matching (Tier 1/2/3), campaigns find-or-create core + - phase: 18-03 + provides: GET /api/phishing/campaigns list route +provides: + - Idempotent re-grouping — re-running /analyze on an already-grouped report no longer double-increments campaigns.report_count + - Regression test proving the sibling-re-match no-op path + - Clamped [0, 200] limit query param on GET /api/phishing/campaigns (explicit limit=0 honored, negative limit no longer 500s) +affects: [19-classification, 20-remediation-approval-audit, 21-autotask-triage-note] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "Own-row current-state guard before mutating an aggregate: fetch the row's own current FK (campaign_id) alongside its match candidates, and short-circuit as a no-op when a tiered/derived match resolves back to the row's own current state — prevents unbounded self-reinforcing increments under repeat invocation." + - "Parse-then-clamp for untrusted numeric query params: parse once with Number.isFinite (not `|| default`, which swallows explicit 0), then Math.min(Math.max(parsed, floor), ceiling)." + +key-files: + created: [] + modified: + - lib/services/campaign-grouping-service.ts + - lib/services/campaign-grouping-service.test.ts + - app/api/phishing/campaigns/route.ts + +key-decisions: + - "Guard placed inside the match branch (matchCampaignId === ownReport.campaign_id) rather than adding a new pre-check query — reuses the ownReport row already fetched, no extra round trip" + - "Did not implement campaign-move / origin-count-decrement — out of scope per VERIFICATION.md, that code path doesn't exist today" + +requirements-completed: [CAMP-02, CAMP-03] + +duration: 2min +completed: 2026-07-16 +--- + +# Phase 18 Plan 04: Campaign re-match idempotency + limit param clamp Summary + +**Fixed unbounded `campaigns.report_count` double-increment on repeat `/analyze` calls by short-circuiting when a tiered match resolves to the report's own current campaign, plus clamped the `limit` query param on `GET /api/phishing/campaigns` to [0, 200].** + +## Performance + +- **Duration:** 2 min +- **Started:** 2026-07-16T02:23:52Z +- **Completed:** 2026-07-16T02:25:43Z +- **Tasks:** 2 completed +- **Files modified:** 3 + +## Accomplishments +- Closed the CAMP-02 gap (CR-01 in 18-REVIEW.md): re-running `POST /api/phishing/tickets/{id}/analyze` on a report already linked to a multi-report campaign no longer inflates that campaign's `report_count`. +- Added a regression test (`campaign-grouping-service.test.ts`) that fails without the fix and proves zero `UPDATE campaigns` / `INSERT INTO campaigns` / `UPDATE reports SET campaign_id` calls on a same-campaign sibling re-match. +- Closed the WR-02 finding: `GET /api/phishing/campaigns?limit=-5` no longer raises an unhandled Postgres 500 ("LIMIT must not be negative"), and `limit=0` is now honored instead of silently replaced by the default of 50. +- Verified no regression: all 16 pre-existing grouping tests plus the new test are green (17/17); `npx tsc --noEmit --pretty` exits 0. + +## Task Commits + +Each task was committed atomically (Task 1 followed RED → GREEN TDD flow): + +1. **Task 1 (RED): add failing regression test** - `afd7af7` (test) +2. **Task 1 (GREEN): short-circuit own-campaign re-match** - `9ca2ccf` (fix) +3. **Task 2: clamp campaigns list limit param (WR-02)** - `abe3d4b` (fix) + +**Plan metadata:** committed with this SUMMARY.md (see final commit) + +## Files Created/Modified +- `lib/services/campaign-grouping-service.ts` - `OwnReportRow` now includes `campaign_id: string | null`; the own-report SELECT fetches `campaign_id::text AS campaign_id` (appended after `created_at` to preserve the test-mock routing substring); the match branch short-circuits to a no-op (`created: false`, no UPDATEs) when `matchCampaignId === ownReport.campaign_id`. +- `lib/services/campaign-grouping-service.test.ts` - New test: "re-analyzing a report already linked to a campaign that a sibling report re-matches does not increment report_count a second time" — stages `ownReport.campaign_id: 'campaign-1'` with a Tier 3 sibling match on the same campaign, asserts zero mutating calls. +- `app/api/phishing/campaigns/route.ts` - `limit` parsing changed from `Math.min(parseInt(...) || 50, 200)` to parse-once-then-clamp: `Math.min(Math.max(Number.isFinite(rawLimit) ? rawLimit : 50, 0), 200)`. `offset`, `status` filter, auth gate, and response shape unchanged. + +## Decisions Made +- Reused the already-fetched `ownReport` row for the no-op guard instead of an extra query — the row is already in scope inside the transaction, so no additional round trip is needed. +- Left the campaign-move / origin-decrement path unimplemented, per VERIFICATION.md's explicit scoping — that behavior doesn't exist anywhere in the codebase today and was called out as conditionally out of scope ("if ever supported"). + +## Deviations from Plan + +None - plan executed exactly as written. Both tasks matched their specified `` blocks precisely (including the exact SQL column-list append point and the `Number.isFinite` clamp shape). + +## TDD Gate Compliance + +Task 1 was `tdd="true"`. Gate sequence verified in git log: +1. RED: `afd7af7 test(18-04): add failing regression test for campaign report_count double-increment` — test run confirmed failure before the fix (1 failed / 16 passed). +2. GREEN: `9ca2ccf fix(18-04): short-circuit own-campaign re-match to stop report_count double-increment` — test run confirmed all 17 tests pass after the fix. +3. No REFACTOR commit needed — implementation was minimal and required no cleanup pass. + +Note: the GREEN commit uses `fix(...)` rather than `feat(...)` since this is a bug fix (per the project's commit-type convention: `fix` for bug corrections), not new feature addition. The RED→GREEN sequencing (test commit before implementation commit) is the gate requirement and is satisfied. + +## Issues Encountered +None. + +## User Setup Required + +None - no external service configuration required. + +## Next Phase Readiness +- CAMP-02 and CAMP-03 requirements are now fully closed; `campaigns.report_count` is accurate and idempotent under repeat `/analyze` calls, and the read API (`GET /api/phishing/campaigns`) safely handles malformed pagination params. +- No blockers for Phase 19 (Classification), which consumes campaign data as an input. + +--- +*Phase: 18-campaign-grouping-phishing-analysis-api* +*Completed: 2026-07-16* From eb660354ef60451b4aa47574b0a12f86c41d0954 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:26:37 -0400 Subject: [PATCH 5/5] docs(18-04): record self-check results in plan summary --- .../18-04-SUMMARY.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md index 9aa9f0d..44b9981 100644 --- a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md @@ -104,3 +104,7 @@ None - no external service configuration required. --- *Phase: 18-campaign-grouping-phishing-analysis-api* *Completed: 2026-07-16* + +## Self-Check: PASSED + +All created/modified files verified present; all commit hashes (afd7af7, 9ca2ccf, abe3d4b) verified in git log.