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..44b9981 --- /dev/null +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md @@ -0,0 +1,110 @@ +--- +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* + +## Self-Check: PASSED + +All created/modified files verified present; all commit hashes (afd7af7, 9ca2ccf, abe3d4b) verified in git log. 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'); 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); + }); }); 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()