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
new file mode 100644
index 0000000..971561f
--- /dev/null
+++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md
@@ -0,0 +1,279 @@
+---
+phase: 18-campaign-grouping-phishing-analysis-api
+reviewed: 2026-07-15T23:41:39Z
+depth: standard
+files_reviewed: 8
+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
+  info: 3
+  total: 9
+status: issues_found
+---
+
+# Phase 18: Code Review Report
+
+**Reviewed:** 2026-07-15T23:41:39Z
+**Depth:** standard
+**Files Reviewed:** 8
+**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.
+
+## Critical Issues
+
+### CR-01: `groupReportIntoCampaign` double-counts `report_count` on every re-run against an already-linked campaign
+
+**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`:
+
+```ts
+const ownReportRes = await client.query<OwnReportRow>(
+  `SELECT title, requester_contact_id, company_id, created_at
+     FROM reports
+    WHERE id = $1`,
+  [reportId]
+);
+```
+
+When a match is found on any tier, the code unconditionally bumps the target campaign's counter
+and re-points the report at it:
+
+```ts
+if (matchCampaignId && matchGroupMethod) {
+  await client.query(
+    `UPDATE campaigns
+        SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW()
+      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 };
+}
+```
+
+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<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
+
+### WR-01: No serialization around concurrent campaign matching — concurrent reports can create duplicate campaigns for the same lure
+
+**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.
+
+### 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<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
+
+**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-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.
+
+## Info
+
+### IN-01: Ticket-row-fetch query duplicated verbatim across three call sites
+
+**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<DetectableTicket | null>` helper
+(e.g. in `phishing-detector.ts`, alongside `DetectableTicket`) and call it from all three sites.
+
+### IN-02: Campaign matching ignores `campaigns.status`
+
+**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.
+
+### IN-03: No test coverage for the CR-01 double-increment scenario
+
+**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.
+
+---
+
+_Reviewed: 2026-07-15T23:41:39Z_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_