From cc2544c7eed201f251400d00a5fc81eb7f1c412a Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:36:03 -0400 Subject: [PATCH] docs(18): update code review report after gap-closure re-review --- .../18-REVIEW.md | 430 +++++++++--------- 1 file changed, 218 insertions(+), 212 deletions(-) diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md index 971561f..8b5badf 100644 --- a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md @@ -1,69 +1,179 @@ --- phase: 18-campaign-grouping-phishing-analysis-api -reviewed: 2026-07-15T23:41:39Z +reviewed: 2026-07-16T02:32:12Z depth: standard -files_reviewed: 8 +files_reviewed: 3 files_reviewed_list: - lib/services/campaign-grouping-service.ts - lib/services/campaign-grouping-service.test.ts - - lib/permissions.ts - - app/api/phishing/tickets/[ticket_id]/analyze/route.ts - - lib/services/webhook-service.ts - - lib/services/phishing-sweep-service.ts - app/api/phishing/campaigns/route.ts - - app/api/phishing/campaigns/[id]/route.ts findings: - critical: 1 - warning: 5 + critical: 2 + warning: 2 info: 3 - total: 9 + total: 7 status: issues_found --- -# Phase 18: Code Review Report +# Phase 18: Code Review Report (re-review, post gap-closure fix 18-04) -**Reviewed:** 2026-07-15T23:41:39Z +**Reviewed:** 2026-07-16T02:32:12Z **Depth:** standard -**Files Reviewed:** 8 +**Files Reviewed:** 3 **Status:** issues_found ## Summary -The core tiered-matching design (`campaign-grouping-service.ts`) is well documented and its -self-exclusion logic (own-row filtering in the Tier 1/2/3 queries) is correctly implemented and -tested. The automatic paths (webhook, sweep) correctly use `skipIfAlreadyGrouped` to avoid -redundant work. However, the one caller that intentionally re-runs grouping unconditionally — -`POST /api/phishing/tickets/{id}/analyze` — exposes a real data-integrity bug: `report_count` is -incremented every time a match is found, even when the match is the report's *own current* -campaign, which is the common case once a campaign has two or more members. This is a BLOCKER -because it silently corrupts the exact metric (`report_count`) analysts use to judge the size of -a phishing campaign. Several secondary issues (an un-clamped negative `limit` query param, a -duplicate-row risk in `messages`, an inconsistent JOIN type between two report-matching queries, -and a latent race condition when concurrent reports contend for the same emerging campaign) round -out the findings below. +This is a re-review of `campaign-grouping-service.ts`, its test file, and the +`/api/phishing/campaigns` route after plan 18-04's gap-closure fix. Both +originally-flagged issues are **verified fixed**: + +- **Prior CR-01 (double-increment on sibling re-match)** — fixed. + `groupReportIntoCampaign` now selects the report's own `campaign_id` + (`campaign-grouping-service.ts:158`) and compares it against + `matchCampaignId` (`:332-334`), short-circuiting with a no-op + `{ created: false }` result when a tier match resolves back to the + report's own current campaign. Verified against the new regression test at + `campaign-grouping-service.test.ts:337-362`, which asserts zero + `UPDATE campaigns`, zero `INSERT INTO campaigns`, and zero + `UPDATE reports` calls for exactly the sibling-re-match scenario the + original finding described. +- **Prior WR-02 (unclamped `limit` → Postgres 500 on negative input)** — + fixed. `route.ts:31-33` now clamps `limit` through `Number.isFinite` + + `Math.max(...,0)` + `Math.min(...,200)`, so a negative, NaN, or oversized + `limit` can no longer reach the SQL `LIMIT` clause unclamped. + +However, this pass surfaced **two new, material bugs** in the same code +paths: + +1. A parameter-binding bug in the campaigns list route (unrelated to the + `limit`/`offset` fix) that makes every status-filtered request 500. +2. A gap in the CR-01 fix itself: the added guard only covers "tier match + resolves to the report's own *current* campaign." It does not cover "tier + match resolves to a *different, already-existing* campaign" — which is + the exact Tier-3→Tier-1 upgrade scenario this phase's own docstring (D-08) + describes as expected, and which the **prior review's own suggested fix + explicitly warned about** ("If the intent is also to support a report + *moving* from one campaign to another, the old campaign's `report_count` + must be decremented at the same time — currently there is no code path + that does this at all"). That caveat was not addressed by the 18-04 fix; + the gap is still open today, just narrower than before. ## Critical Issues -### CR-01: `groupReportIntoCampaign` double-counts `report_count` on every re-run against an already-linked campaign +### CR-01: `/api/phishing/campaigns?status=...` throws a Postgres parameter-binding error (500) on every status-filtered request -**File:** `lib/services/campaign-grouping-service.ts:156-165, 299-334` -**Issue:** -The transaction's own-report query never selects the report's *current* `campaign_id`: +**File:** `app/api/phishing/campaigns/route.ts:36-56` +**Issue:** `statusFilter` is built once, against the parameter list used by +the *first* query (`params = [limit, offset, status]`, so the placeholder is +computed as `$${params.length}` = `$3`): ```ts -const ownReportRes = await client.query( - `SELECT title, requester_contact_id, company_id, created_at - FROM reports - WHERE id = $1`, - [reportId] +const params: unknown[] = [limit, offset]; +let statusFilter = ''; +if (status) { + params.push(status); + statusFilter = `WHERE status = $${params.length}`; // "WHERE status = $3" +} +``` + +That same `statusFilter` string (still referencing `$3`) is then reused +verbatim for the **total count** query, but with a completely different, +much shorter params array: + +```ts +const totalRes = await postgresClient.query<{ count: string }>( + `SELECT COUNT(*)::text AS count FROM campaigns ${statusFilter}`, + status ? [status] : [] // only one bind value supplied, but the SQL text references $3 ); ``` -When a match is found on any tier, the code unconditionally bumps the target campaign's counter -and re-points the report at it: +Postgres's extended query protocol requires every placeholder referenced in +the SQL text to have a bound value at that ordinal position. With `status` +set, the total-count query text contains `$3` but only one bind parameter is +supplied — `pg` throws a bind-mismatch error, which is caught by the route's +`catch` block and returned as a `500`. **Every** call to this endpoint with a +`status` filter (e.g. `?status=open`, exactly what a campaigns dashboard +would call by default) fails outright. + +**Fix:** Give the total-count query its own independent placeholder numbering +instead of reusing `statusFilter`: + +```ts +const totalRes = await postgresClient.query<{ count: string }>( + `SELECT COUNT(*)::text AS count FROM campaigns ${status ? 'WHERE status = $1' : ''}`, + status ? [status] : [] +); +``` + +### CR-02: CR-01's same-campaign guard doesn't cover cross-campaign migration — old campaign's `report_count` is never decremented + +**File:** `lib/services/campaign-grouping-service.ts:324-346` +**Issue:** The gap-closure fix added exactly one guard: + +```ts +if (matchCampaignId === ownReport.campaign_id) { + return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; +} +``` + +This correctly no-ops when a tier match resolves back to the report's own +*current* campaign (the originally-flagged scenario). But it does nothing +for the case where `ownReport.campaign_id` is non-null and `matchCampaignId` +resolves to a **different, already-existing** campaign. That case is not +hypothetical — it's exactly what the file's own docstring describes as +expected behavior: + +> D-08: ... it may upgrade a Tier-3-only report to Tier 1 after +> `parseAndStoreMessage` has just populated `messages`/`indicators` for the +> first time. + +Concretely: report X is created, Tier 3 matches nothing, and a new campaign +A is created for it (`report_count = 1`, `reports.campaign_id = A`). Later, +`/analyze` is re-run on X after `parseAndStoreMessage` has populated its +`messages`/`indicators` row. Tier 1 now finds a *different*, pre-existing +campaign B (formed by another report sharing the same `message_id`). Since +`B !== A`, the new guard doesn't trigger, and the code falls through to: + +```ts +await client.query( + `UPDATE campaigns SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW() WHERE id = $1`, + [matchCampaignId] // increments B +); +await client.query( + `UPDATE reports SET campaign_id = $1, updated_at = NOW() WHERE id = $2`, + [matchCampaignId, reportId] // moves X from A to B +); +``` + +Campaign A's `report_count` is **never decremented**. A now has +`report_count = 1` with zero actually-linked reports — a permanently +stale/inflated count that the exact endpoint under review +(`GET /api/phishing/campaigns`) surfaces to users indefinitely, with no +reconciliation path anywhere in the codebase. + +This is not a newly-introduced regression — the prior review's suggested fix +for the original bug explicitly flagged this exact risk ("a moved report +also leaves the origin campaign's count permanently inflated") — but the +18-04 fix implemented only the narrower same-campaign guard and left this +caveat unaddressed. It should be treated as still-open, not resolved. + +**Fix:** Decrement the outgoing campaign when the report is moving to a +genuinely different campaign, not just skip when it's the same one: ```ts if (matchCampaignId && matchGroupMethod) { + if (matchCampaignId === ownReport.campaign_id) { + return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; + } + if (ownReport.campaign_id) { + await client.query( + `UPDATE campaigns + SET report_count = GREATEST(report_count - 1, 0), updated_at = NOW() + WHERE id = $1`, + [ownReport.campaign_id] + ); + } await client.query( `UPDATE campaigns SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW() @@ -77,203 +187,99 @@ if (matchCampaignId && matchGroupMethod) { return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; } ``` - -There is no check for `matchCampaignId === (report's existing campaign_id)`. This function is -called *unconditionally* (no `skipIfAlreadyGrouped`) from -`app/api/phishing/tickets/[ticket_id]/analyze/route.ts:75`, by design ("D-08: /analyze always -re-runs grouping unconditionally"). Any campaign with two or more reports has, by construction, at -least one *other* report already linked to the same campaign that satisfies Tier 1 (shared -`message_id`), Tier 2 (shared attachment/URL + subject/sender), or Tier 3 (sender + subject + -company) against the report being re-analyzed. So the very next time an analyst (or an automated -retry, or a double-clicked "re-analyze" button) calls `/analyze` on any report belonging to a -multi-report campaign, `groupReportIntoCampaign` will re-match that same campaign and increment -`report_count` again for a report that was already counted — with no bound on how many times this -can happen. `report_count` is the number surfaced verbatim by both -`GET /api/phishing/campaigns` and `GET /api/phishing/campaigns/[id]` as the primary signal of a -campaign's blast radius; this bug silently inflates that number every time `/analyze` is -re-invoked on an already-grouped ticket, which is an explicitly supported, documented use case -for this route (upgrading Tier 3 -> Tier 1/2 after EML parsing). - -None of the existing tests cover this path — the "self-exclusion" test in -`campaign-grouping-service.test.ts:305-335` only verifies the case where *nothing else* matches -(so a fresh campaign is created); it does not cover the case where a *different, sibling* report -in the same campaign is found again on re-run. - -**Fix:** -Select the report's current `campaign_id` in `ownReportRes` and short-circuit when the tiered -match resolves to that same campaign: - -```ts -const ownReportRes = await client.query( - `SELECT title, requester_contact_id, company_id, created_at, campaign_id::text AS campaign_id - FROM reports - WHERE id = $1`, - [reportId] -); -... -if (matchCampaignId && matchGroupMethod) { - if (matchCampaignId === ownReport.campaign_id) { - // Already correctly linked — re-run found the same campaign again; no-op. - return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; - } - await client.query(`UPDATE campaigns SET report_count = report_count + 1, ... WHERE id = $1`, [matchCampaignId]); - await client.query(`UPDATE reports SET campaign_id = $1, updated_at = NOW() WHERE id = $2`, [matchCampaignId, reportId]); - return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; -} -``` -(If the intent is also to support a report *moving* from one campaign to another, the old -campaign's `report_count` must be decremented at the same time — currently there is no code path -that does this at all, so a moved report also leaves the origin campaign's count permanently -inflated.) +(A campaign left at `report_count = 0` raises a separate follow-up question +— whether to soft-delete/hide it from the list endpoint — but that's a +smaller design decision than leaving the count permanently wrong.) ## Warnings -### WR-01: No serialization around concurrent campaign matching — concurrent reports can create duplicate campaigns for the same lure +### WR-01: Tier 2 candidate query has no `ORDER BY` — non-deterministic campaign selection among multiple qualifying siblings -**File:** `lib/services/campaign-grouping-service.ts:140-389`, `lib/services/webhook-service.ts:458-495` -**Issue:** Each `POST /api/webhooks/autotask` request that carries a new phishing-flagged ticket -fires `triggerPhishingDetection()` fire-and-forget (`webhook-service.ts:119-121`), with no queue -or lock serializing concurrent invocations. `groupReportIntoCampaign` reads candidate matches and -then writes the result inside a single `postgresClient.transaction()` at the default -(`READ COMMITTED`) isolation level, with no `SELECT ... FOR UPDATE` / advisory lock on the -dedup key. If two reports of the *same* mass-phishing lure arrive close together (the scenario -this whole feature exists to catch — many employees reporting the same email via the KnowBe4 -button within seconds of each other), both transactions can run their Tier 1/2/3 lookups before -either commits, both see "no match", and both create separate campaigns for what should be one. -This is a distinct failure mode from the already-documented "no merge logic" limitation (D-04) — -that limitation is about *not undoing* a bad grouping after the fact; this is about *never -grouping correctly in the first place* under concurrent load. -**Fix:** Use a Postgres advisory lock keyed by a normalized dedup signal (e.g. -`hashtext(sender || normalized_subject || company_id)`) around the tiered-matching block, or -serialize phishing-ticket processing through a single-writer queue, so two concurrent reports of -the same lure cannot both observe "no existing campaign" at once. +**File:** `lib/services/campaign-grouping-service.ts:241-251` +**Issue:** Tier 1 (`campaign-grouping-service.ts:192-201`, `ORDER BY +r.created_at ASC LIMIT 1`) and Tier 3 (`:301-311`, `ORDER BY r.created_at +ASC`) both deterministically pick the earliest-created match. The Tier 2 +candidate query has no `ORDER BY` at all: -### WR-02: `GET /api/phishing/campaigns` — negative `limit` is not clamped and `limit=0` silently becomes 50 - -**File:** `app/api/phishing/campaigns/route.ts:31` -**Issue:** ```ts -const limit = Math.min(parseInt(url.searchParams.get('limit') ?? '50', 10) || 50, 200); -``` -- `?limit=0` → `parseInt` yields `0`, and `0 || 50` evaluates the fallback because `0` is falsy, - silently overriding an explicit, valid request for zero rows with 50. -- `?limit=-5` → `parseInt` yields `-5`, `-5 || 50` is `-5` (truthy, non-zero), and - `Math.min(-5, 200)` stays `-5`. Unlike `offset` two lines below (which is correctly floored with - `Math.max(..., 0)`), `limit` has no lower bound. `-5` is then passed straight into - `LIMIT $1 OFFSET $2`, and Postgres raises `ERROR: LIMIT must not be negative`, surfacing as an - unhandled 500 from this endpoint for a trivially-supplied query string. -**Fix:** -```ts -const limitParam = parseInt(url.searchParams.get('limit') ?? '', 10); -const limit = Math.min(Math.max(Number.isFinite(limitParam) ? limitParam : 50, 0), 200); -``` - -### WR-03: Repeated `/analyze` calls insert duplicate `messages`/`indicators` rows, and `groupReportIntoCampaign` picks one non-deterministically - -**File:** `app/api/phishing/tickets/[ticket_id]/analyze/route.ts:70`, `lib/services/campaign-grouping-service.ts:172-179` -**Issue:** `parseAndStoreMessage` is invoked unconditionally on every `POST /analyze` call, and -(per `lib/services/phishing-eml-service.ts`) inserts a new `messages` row plus new `indicators` -rows with no existence check and no `ON CONFLICT` — there is no unique constraint on -`messages.report_id`. Any repeat call to `/analyze` for the same ticket (a normal thing to do, -since this route is explicitly meant to be re-run — see the file's own header comment) therefore -accumulates duplicate `messages`/`indicators` rows for the same report. Downstream, -`groupReportIntoCampaign`'s own-message lookup: -```ts -const ownMessageRes = await client.query( - `SELECT id::text AS id, message_id FROM messages WHERE report_id = $1 LIMIT 1`, - [reportId] +const tier2Candidates = await client.query( + `SELECT r.id::text AS report_id, r.campaign_id::text AS campaign_id, r.title, + m.id::text AS message_id + FROM messages m + JOIN reports r ON r.id = m.report_id + WHERE r.campaign_id IS NOT NULL + AND r.id != $1 + AND r.created_at BETWEEN $2::timestamptz - INTERVAL '24 hours' + AND $2::timestamptz + INTERVAL '24 hours'`, + [reportId, ownReport.created_at] ); ``` -has no `ORDER BY`, so which duplicate row is used for Tier 1/2 matching becomes arbitrary and can -vary between calls. `GET /api/phishing/campaigns/[id]` (`app/api/phishing/campaigns/[id]/route.ts:100-107`) -also has no dedup and will surface every duplicate `messages`/`indicators` row verbatim in its -response. -**Fix:** In `parseAndStoreMessage` (or at the call site in `analyze/route.ts`), check for an -existing `messages` row for the `report_id` first and skip/short-circuit if one already exists -(mirroring the `content_hash` idempotency guard already used in `phishing-detector.ts`), or add a -unique constraint on `messages.report_id` and upsert. -### WR-04: Tier 3 matching query uses an `INNER JOIN` to `contacts` that filters out legitimate candidates +If two distinct existing campaigns both have a sibling report matching by +attachment-hash/URL-domain + subject + sender within the 24h window (plausible +during a large phishing wave, where multiple independent campaigns can share +a lure template), which one gets attached is whatever order Postgres happens +to return rows in — not guaranteed stable across query plans/analyze cycles, +and inconsistent with Tier 1/Tier 3's explicit earliest-wins ordering. +**Fix:** Add `ORDER BY r.created_at ASC` to match the Tier 1/Tier 3 +convention. -**File:** `lib/services/campaign-grouping-service.ts:300-312` -**Issue:** -```sql -SELECT r.campaign_id::text AS campaign_id, r.title - FROM reports r - JOIN contacts c ON c.id = r.requester_contact_id - WHERE r.requester_contact_id = $1 - AND r.company_id = $2 - ... -``` -No column from `contacts` is selected or filtered on — the join exists only to (silently) require -that `requester_contact_id` currently resolves to a row in `contacts`. `reports.requester_contact_id` -has no `FOREIGN KEY` constraint (see `migrations/097_phishing_triage_schema.sql:60`), so a contact -that has since been deleted, merged, or not-yet-synced from Autotask will cause this `INNER JOIN` -to silently drop an otherwise-valid Tier 3 candidate, understating campaign membership. Contrast -with `app/api/phishing/campaigns/[id]/route.ts:91`, which correctly uses a `LEFT JOIN` for the -identical `contacts` relationship. -**Fix:** Either drop the join (nothing from `contacts` is used) or change it to a `LEFT JOIN` to -match the read-path's handling of the same relationship. +### WR-02: No regression test covers the same-campaign guard for Tier 1/Tier 2, nor the CR-02 cross-campaign-migration gap -### WR-05: Tier 2 sender-indicator comparison is case-sensitive with no normalization - -**File:** `lib/services/campaign-grouping-service.ts:270-273` -**Issue:** -```ts -const candidateSender = candidateIndicators.find((i) => i.indicator_type === 'sender')?.value; -if (candidateSender !== ownSenderValue) continue; -``` -Both values are raw email-address strings taken directly from `indicators.value` (populated in -`phishing-eml-service.ts` from the parsed `From:` header with no case normalization). -`normalizeSubject()` is used to guard against case drift in the subject, but there is no -equivalent normalization for the sender email before this exact-string comparison, so two -messages from literally the same sender with different casing in the address (a real possibility -across different mail clients re-forwarding the same lure) will fail to Tier-2-match even when the -attachment hash / URL domain otherwise line up. -**Fix:** Compare with `candidateSender?.toLowerCase() !== ownSenderValue?.toLowerCase()`, and -apply the same normalization when building `computeTier2Key`'s `senderValue` component so newly -created campaign keys stay consistent. +**File:** `lib/services/campaign-grouping-service.test.ts:337-362` +**Issue:** The regression test added by 18-04 +(`'re-analyzing a report already linked to a campaign that a sibling report +re-matches...'`) only exercises the same-campaign guard via a **Tier 3** +match. The guard is generic — applied once after all three tiers resolve — +so this is a coverage gap rather than proof the fix generalizes, and nothing +in the current suite would have caught CR-02 above. +**Fix:** Add two more cases: (1) a Tier 1 sibling match that resolves to the +report's own current campaign (should still no-op — currently unverified for +this tier), and (2) a case where `ownReport.campaign_id = 'campaign-A'` and +the tier match resolves to a *different* `'campaign-B'` — asserting the fix +for CR-02 (old campaign decremented, new campaign incremented, report moved) +once implemented, so this doesn't regress silently again. ## Info -### IN-01: Ticket-row-fetch query duplicated verbatim across three call sites +### IN-01: `limit=0` or negative is silently clamped to `0` (empty page) rather than falling back to the documented default of 50 -**File:** `app/api/phishing/tickets/[ticket_id]/analyze/route.ts:32-44`, `lib/services/webhook-service.ts:459-471`, `lib/services/phishing-sweep-service.ts:45-61` -**Issue:** The exact same `SELECT id, ticket_number, title, description, company_id, contact_id, -created_by_contact_id FROM tickets WHERE id = $1` query (and the matching `DetectableTicket` -object construction) is duplicated near-verbatim in all three trigger sites. The module headers of -`campaign-grouping-service.ts` and `phishing-detector.ts` explicitly call out "no duplicated -matching logic between callers" as a design goal for the *matching* logic — this row-fetch/mapping -step is the one piece that didn't get the same shared-helper treatment. -**Fix:** Extract a small `loadDetectableTicket(ticketId): Promise` helper -(e.g. in `phishing-detector.ts`, alongside `DetectableTicket`) and call it from all three sites. +**File:** `app/api/phishing/campaigns/route.ts:32` +**Issue:** `Math.min(Math.max(Number.isFinite(rawLimit) ? rawLimit : 50, 0), 200)` +correctly prevents the Postgres 500 that the prior WR-02 flagged, but a +request like `?limit=-5` now silently returns an empty `items` array with +`limit: 0` in the response rather than an error or the documented default. +Callers may interpret an empty page as "no campaigns" instead of "your +`limit` param was invalid." Not a crash, just a silent-success footgun. +**Fix:** Consider treating `<= 0` the same as "unparseable" and falling back +to `50`, or explicitly document that `limit=0` is a valid "return nothing" +request. -### IN-02: Campaign matching ignores `campaigns.status` +### IN-02: `status` query param is not validated against known campaign statuses -**File:** `lib/services/campaign-grouping-service.ts:184-318` -**Issue:** None of the Tier 1/2/3 matching queries filter on `campaigns.status`. A new (or -re-analyzed) report can silently attach to a campaign an analyst has already marked resolved/closed, -bumping `report_count`/`last_seen_at` with no visible signal that a "closed" investigation just -grew a new member. The code's own comments explicitly defer `status` transitions to Phase 19/20, -so this may be intentional scope-narrowing rather than an oversight — flagging for visibility since -it interacts with the `report_count` semantics analysts will rely on. -**Fix (if in scope for a later phase):** Either exclude non-`open` campaigns from matching, or -surface a `reopened` flag / audit event when a report attaches to a non-open campaign. +**File:** `app/api/phishing/campaigns/route.ts:34` +**Issue:** `status` is read straight from the query string and used directly +in the parameterized `WHERE` clause (safe from injection) but never checked +against the actual `campaigns.status` enum/values. A typo (`?status=opne`) +silently returns zero rows with a `200` rather than a `400`. Consistent with +the project's "no Zod in route handlers unless it matters" convention, but +worth a one-line comment noting it's intentional. -### IN-03: No test coverage for the CR-01 double-increment scenario +### IN-03: Documented concurrency gap (`campaign_key` has no UNIQUE constraint) remains unaddressed — pre-existing, not touched by this fix -**File:** `lib/services/campaign-grouping-service.test.ts` -**Issue:** The suite thoroughly covers first-time Tier 1/2/3 matches, campaign creation, and -"self only, nothing else matches" self-exclusion (`campaign-grouping-service.test.ts:305-335`), but -has no test for the case exercised by CR-01: a report that is already linked to campaign X, whose -tiered re-match finds a *different* report also linked to X (i.e., re-matches its own current -campaign via a sibling row, not via its own row). -**Fix:** Add a test staging `ownReport.campaign_id` (once selected, per the CR-01 fix) equal to a -tier match's `campaign_id`, asserting `UPDATE campaigns ... report_count = report_count + 1` is -**not** called a second time. +**File:** `lib/services/campaign-grouping-service.ts:125-126` +**Issue:** The file's own comment ("Pitfall 2 — `campaigns.campaign_key` has +no UNIQUE constraint") acknowledges that concurrent calls to +`groupReportIntoCampaign` (e.g. webhook fire + cron sweep landing at nearly +the same time for two reports that would both create/join the same new +campaign) can race and create duplicate campaign rows or double-apply +increments, since there's no row-level locking (`SELECT ... FOR UPDATE`) or +DB-level uniqueness backing the find-or-create. Unrelated to the CR-01/WR-02 +fixes verified in this pass — listed here only to confirm it's still open, +not silently resolved. --- -_Reviewed: 2026-07-15T23:41:39Z_ +_Reviewed: 2026-07-16T02:32:12Z_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_