lorentz/wulf-pulse
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

docs(18): update code review report after gap-closure re-review

Browse source
This commit is contained in:
lorentz 2026-07-15 22:36:03 -04:00
parent 650f9b8100
commit cc2544c7ee
1 changed files with 218 additions and 212 deletions
Download patch file Download diff file Expand all files Collapse all files

430
.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md
View file

@ -1,69 +1,179 @@
--- ---
phase: 18-campaign-grouping-phishing-analysis-api phase: 18-campaign-grouping-phishing-analysis-api
reviewed: 2026-07-15T23:41:39Z reviewed: 2026-07-16T02:32:12Z
depth: standard depth: standard
files_reviewed: 8 files_reviewed: 3
files_reviewed_list: files_reviewed_list:
- lib/services/campaign-grouping-service.ts - lib/services/campaign-grouping-service.ts
- lib/services/campaign-grouping-service.test.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/route.ts
- app/api/phishing/campaigns/[id]/route.ts
findings: findings:
critical: 1 critical: 2
warning: 5 warning: 2
info: 3 info: 3
total: 9 total: 7
status: issues_found 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 **Depth:** standard
**Files Reviewed:** 8 **Files Reviewed:** 3
**Status:** issues_found **Status:** issues_found
## Summary ## Summary
The core tiered-matching design (`campaign-grouping-service.ts`) is well documented and its This is a re-review of `campaign-grouping-service.ts`, its test file, and the
self-exclusion logic (own-row filtering in the Tier 1/2/3 queries) is correctly implemented and `/api/phishing/campaigns` route after plan 18-04's gap-closure fix. Both
tested. The automatic paths (webhook, sweep) correctly use `skipIfAlreadyGrouped` to avoid originally-flagged issues are **verified fixed**:
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 - **Prior CR-01 (double-increment on sibling re-match)** — fixed.
incremented every time a match is found, even when the match is the report's *own current* `groupReportIntoCampaign` now selects the report's own `campaign_id`
campaign, which is the common case once a campaign has two or more members. This is a BLOCKER (`campaign-grouping-service.ts:158`) and compares it against
because it silently corrupts the exact metric (`report_count`) analysts use to judge the size of `matchCampaignId` (`:332-334`), short-circuiting with a no-op
a phishing campaign. Several secondary issues (an un-clamped negative `limit` query param, a `{ created: false }` result when a tier match resolves back to the
duplicate-row risk in `messages`, an inconsistent JOIN type between two report-matching queries, report's own current campaign. Verified against the new regression test at
and a latent race condition when concurrent reports contend for the same emerging campaign) round `campaign-grouping-service.test.ts:337-362`, which asserts zero
out the findings below. `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 ## 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` **File:** `app/api/phishing/campaigns/route.ts:36-56`
**Issue:** **Issue:** `statusFilter` is built once, against the parameter list used by
The transaction's own-report query never selects the report's *current* `campaign_id`: the *first* query (`params = [limit, offset, status]`, so the placeholder is
computed as `$${params.length}` = `$3`):
```ts ```ts
const ownReportRes = await client.query<OwnReportRow>( const params: unknown[] = [limit, offset];
`SELECT title, requester_contact_id, company_id, created_at let statusFilter = '';
FROM reports if (status) {
WHERE id = $1`, params.push(status);
[reportId] 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 Postgres's extended query protocol requires every placeholder referenced in
and re-points the report at it: 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 ```ts
if (matchCampaignId && matchGroupMethod) { 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( await client.query(
`UPDATE campaigns `UPDATE campaigns
SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW() 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 }; return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false };
} }
``` ```
(A campaign left at `report_count = 0` raises a separate follow-up question
There is no check for `matchCampaignId === (report's existing campaign_id)`. This function is — whether to soft-delete/hide it from the list endpoint — but that's a
called *unconditionally* (no `skipIfAlreadyGrouped`) from smaller design decision than leaving the count permanently wrong.)
`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<OwnReportRow>(
`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.)
## Warnings ## 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` **File:** `lib/services/campaign-grouping-service.ts:241-251`
**Issue:** Each `POST /api/webhooks/autotask` request that carries a new phishing-flagged ticket **Issue:** Tier 1 (`campaign-grouping-service.ts:192-201`, `ORDER BY
fires `triggerPhishingDetection()` fire-and-forget (`webhook-service.ts:119-121`), with no queue r.created_at ASC LIMIT 1`) and Tier 3 (`:301-311`, `ORDER BY r.created_at
or lock serializing concurrent invocations. `groupReportIntoCampaign` reads candidate matches and ASC`) both deterministically pick the earliest-created match. The Tier 2
then writes the result inside a single `postgresClient.transaction()` at the default candidate query has no `ORDER BY` at all:
(`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.
### 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 ```ts
const limit = Math.min(parseInt(url.searchParams.get('limit') ?? '50', 10) || 50, 200); const tier2Candidates = await client.query<Tier2CandidateRow>(
``` `SELECT r.id::text AS report_id, r.campaign_id::text AS campaign_id, r.title,
- `?limit=0` → `parseInt` yields `0`, and `0 || 50` evaluates the fallback because `0` is falsy, m.id::text AS message_id
silently overriding an explicit, valid request for zero rows with 50. FROM messages m
- `?limit=-5` → `parseInt` yields `-5`, `-5 || 50` is `-5` (truthy, non-zero), and JOIN reports r ON r.id = m.report_id
`Math.min(-5, 200)` stays `-5`. Unlike `offset` two lines below (which is correctly floored with WHERE r.campaign_id IS NOT NULL
`Math.max(..., 0)`), `limit` has no lower bound. `-5` is then passed straight into AND r.id != $1
`LIMIT $1 OFFSET $2`, and Postgres raises `ERROR: LIMIT must not be negative`, surfacing as an AND r.created_at BETWEEN $2::timestamptz - INTERVAL '24 hours'
unhandled 500 from this endpoint for a trivially-supplied query string. AND $2::timestamptz + INTERVAL '24 hours'`,
**Fix:** [reportId, ownReport.created_at]
```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<OwnMessageRow>(
`SELECT id::text AS id, message_id FROM messages WHERE report_id = $1 LIMIT 1`,
[reportId]
); );
``` ```
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` ### WR-02: No regression test covers the same-campaign guard for Tier 1/Tier 2, nor the CR-02 cross-campaign-migration gap
**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-05: Tier 2 sender-indicator comparison is case-sensitive with no normalization **File:** `lib/services/campaign-grouping-service.test.ts:337-362`
**Issue:** The regression test added by 18-04
**File:** `lib/services/campaign-grouping-service.ts:270-273` (`'re-analyzing a report already linked to a campaign that a sibling report
**Issue:** re-matches...'`) only exercises the same-campaign guard via a **Tier 3**
```ts match. The guard is generic — applied once after all three tiers resolve —
const candidateSender = candidateIndicators.find((i) => i.indicator_type === 'sender')?.value; so this is a coverage gap rather than proof the fix generalizes, and nothing
if (candidateSender !== ownSenderValue) continue; 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
Both values are raw email-address strings taken directly from `indicators.value` (populated in report's own current campaign (should still no-op — currently unverified for
`phishing-eml-service.ts` from the parsed `From:` header with no case normalization). this tier), and (2) a case where `ownReport.campaign_id = 'campaign-A'` and
`normalizeSubject()` is used to guard against case drift in the subject, but there is no the tier match resolves to a *different* `'campaign-B'` — asserting the fix
equivalent normalization for the sender email before this exact-string comparison, so two for CR-02 (old campaign decremented, new campaign incremented, report moved)
messages from literally the same sender with different casing in the address (a real possibility once implemented, so this doesn't regress silently again.
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.
## Info ## 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` **File:** `app/api/phishing/campaigns/route.ts:32`
**Issue:** The exact same `SELECT id, ticket_number, title, description, company_id, contact_id, **Issue:** `Math.min(Math.max(Number.isFinite(rawLimit) ? rawLimit : 50, 0), 200)`
created_by_contact_id FROM tickets WHERE id = $1` query (and the matching `DetectableTicket` correctly prevents the Postgres 500 that the prior WR-02 flagged, but a
object construction) is duplicated near-verbatim in all three trigger sites. The module headers of request like `?limit=-5` now silently returns an empty `items` array with
`campaign-grouping-service.ts` and `phishing-detector.ts` explicitly call out "no duplicated `limit: 0` in the response rather than an error or the documented default.
matching logic between callers" as a design goal for the *matching* logic — this row-fetch/mapping Callers may interpret an empty page as "no campaigns" instead of "your
step is the one piece that didn't get the same shared-helper treatment. `limit` param was invalid." Not a crash, just a silent-success footgun.
**Fix:** Extract a small `loadDetectableTicket(ticketId): Promise<DetectableTicket | null>` helper **Fix:** Consider treating `<= 0` the same as "unparseable" and falling back
(e.g. in `phishing-detector.ts`, alongside `DetectableTicket`) and call it from all three sites. 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` **File:** `app/api/phishing/campaigns/route.ts:34`
**Issue:** None of the Tier 1/2/3 matching queries filter on `campaigns.status`. A new (or **Issue:** `status` is read straight from the query string and used directly
re-analyzed) report can silently attach to a campaign an analyst has already marked resolved/closed, in the parameterized `WHERE` clause (safe from injection) but never checked
bumping `report_count`/`last_seen_at` with no visible signal that a "closed" investigation just against the actual `campaigns.status` enum/values. A typo (`?status=opne`)
grew a new member. The code's own comments explicitly defer `status` transitions to Phase 19/20, silently returns zero rows with a `200` rather than a `400`. Consistent with
so this may be intentional scope-narrowing rather than an oversight — flagging for visibility since the project's "no Zod in route handlers unless it matters" convention, but
it interacts with the `report_count` semantics analysts will rely on. worth a one-line comment noting it's intentional.
**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.
### 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` **File:** `lib/services/campaign-grouping-service.ts:125-126`
**Issue:** The suite thoroughly covers first-time Tier 1/2/3 matches, campaign creation, and **Issue:** The file's own comment ("Pitfall 2 — `campaigns.campaign_key` has
"self only, nothing else matches" self-exclusion (`campaign-grouping-service.test.ts:305-335`), but no UNIQUE constraint") acknowledges that concurrent calls to
has no test for the case exercised by CR-01: a report that is already linked to campaign X, whose `groupReportIntoCampaign` (e.g. webhook fire + cron sweep landing at nearly
tiered re-match finds a *different* report also linked to X (i.e., re-matches its own current the same time for two reports that would both create/join the same new
campaign via a sibling row, not via its own row). campaign) can race and create duplicate campaign rows or double-apply
**Fix:** Add a test staging `ownReport.campaign_id` (once selected, per the CR-01 fix) equal to a increments, since there's no row-level locking (`SELECT ... FOR UPDATE`) or
tier match's `campaign_id`, asserting `UPDATE campaigns ... report_count = report_count + 1` is DB-level uniqueness backing the find-or-create. Unrelated to the CR-01/WR-02
**not** called a second time. 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)_ _Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_ _Depth: standard_