docs(phase-18): add verification report (gaps found)

.planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md

@@ -0,0 +1,159 @@
+---
+phase: 18-campaign-grouping-phishing-analysis-api
+verified: 2026-07-15T20:05:00Z
+status: gaps_found
+score: 4/5 must-haves verified
+overrides_applied: 0
+gaps:
+  - truth: "A campaign accumulates additional linked ticket reports over time as duplicates are detected, without corrupting report_count (CAMP-02)"
+    status: failed
+    reason: >
+      groupReportIntoCampaign's own-report query (lib/services/campaign-grouping-service.ts:156-165)
+      never selects the report's current campaign_id, and the match branch
+      (lines 323-335) has no check for matchCampaignId === the report's own
+      existing campaign. Every tier query correctly excludes the report's own
+      row from candidates (r.id != reportId), but a SIBLING report already in
+      the same campaign will legitimately satisfy Tier 1/2/3 once a campaign
+      has 2+ members. POST /api/phishing/tickets/{id}/analyze intentionally
+      re-runs groupReportIntoCampaign with no skipIfAlreadyGrouped (by design,
+      to allow Tier-3->Tier-1/2 upgrades), so every repeat /analyze call on a
+      report belonging to a multi-report campaign re-matches that campaign and
+      increments report_count again for a report already counted. There is no
+      upper bound — a double-clicked "re-analyze" or automated retry inflates
+      the count indefinitely. report_count is the exact field GET
+      /api/phishing/campaigns and GET /api/phishing/campaigns/{id} surface
+      verbatim as the campaign's blast-radius signal, so CAMP-03's read API
+      exposes an unreliable number for any campaign that has been re-analyzed.
+      Already identified and classified as a blocker in the phase's own code
+      review (18-REVIEW.md CR-01); confirmed by direct code read during
+      verification, and confirmed that no test in
+      campaign-grouping-service.test.ts covers this scenario (the existing
+      "self-exclusion" test at line 305 only covers the case where NOTHING
+      else matches, not the case where a sibling report in the same campaign
+      re-matches).
+    artifacts:
+      - path: "lib/services/campaign-grouping-service.ts"
+        issue: "groupReportIntoCampaign lines 156-165 and 323-334: no short-circuit when a tiered match resolves to the report's own current campaign_id; ownReportRes SELECT does not fetch campaign_id at all"
+    missing:
+      - "Select reports.campaign_id in ownReportRes and compare it against matchCampaignId before the UPDATE campaigns / UPDATE reports block; no-op (return existing result) when they're equal"
+      - "A test staging a tier match whose campaign_id equals the report's own current campaign_id (via a sibling report), asserting UPDATE campaigns is not called a second time"
+      - "If report-moves-between-campaigns is ever supported, decrement the origin campaign's report_count in the same transaction (currently no code path does this at all)"
+human_verification:
+  - test: "curl POST /api/phishing/tickets/{knownPhishingTicketId}/analyze with admin session cookie, then with no cookie, then with a user-role cookie, then with a non-numeric ticket_id"
+    expected: "200 with {reportId,campaignId,groupMethod,created} (admin); 401 (no cookie); 403 (user-role, lacks phishing:analyze); 400 (non-numeric ticket_id)"
+    why_human: "Both 18-02-SUMMARY.md and the plan's <human-check> step were explicitly deferred — no executor ran this against a live server with real session cookies; code-level requirePermission('phishing','analyze') gate was confirmed by static read only"
+  - test: "curl GET /api/phishing/campaigns and GET /api/phishing/campaigns/{id} with admin cookie, no cookie, malformed id, absent id"
+    expected: "200 camelCase paginated list / nested detail (admin); 401 (no cookie); 400 (malformed UUID); 404 (absent campaign)"
+    why_human: "18-03-SUMMARY.md explicitly documents this deviation — the shared dev server on port 3100 was running master, not this worktree's code, so the live curl pass was never executed; only grep/tsc-level static verification was performed"
+---
+
+# Phase 18: Campaign Grouping & Phishing Analysis API Verification Report
+
+**Phase Goal:** Duplicate reports of the same phishing/spam campaign are automatically grouped and accumulate over time, and an operator can trigger analysis of a specific ticket or browse campaigns through a properly access-controlled `/api/phishing/*` surface.
+**Verified:** 2026-07-15T20:05:00Z
+**Status:** gaps_found
+**Re-verification:** No — initial verification
+
+## Goal Achievement
+
+### Observable Truths
+
+| # | Truth | Status | Evidence |
+|---|-------|--------|----------|
+| 1 | Two reports sharing Message-ID are grouped into the same campaign; absent that, attachment-hash/URL-domain+subject+sender+24h; absent that, sender+normalized-subject+client+24h fallback (CAMP-01) | VERIFIED | `lib/services/campaign-grouping-service.ts:184-318` implements Tier 1→2→3 in order, gated on previous tier failing; 16/16 tests pass (`npx vitest run lib/services/campaign-grouping-service.test.ts`), covering Tier 1 short-circuit, Tier 2 hash+URL-domain matching, Tier 3 sender+subject+client matching |
+| 2 | A campaign accumulates additional linked reports over time as duplicates are detected, without ever creating a second campaign, and `report_count` stays accurate (CAMP-02) | **FAILED** | First-time grouping is correct (verified by tests) but `groupReportIntoCampaign` double-increments `report_count` whenever a tiered re-match resolves to the report's *own current* campaign via a sibling row — see Gaps below. Confirmed by direct source read (`campaign-grouping-service.ts:156-165, 323-334`) and by the phase's own code review (18-REVIEW.md CR-01, blocker) |
+| 3 | `POST /api/phishing/tickets/{ticket_id}/analyze` runs detect→parse→group for one ticket and returns the campaign linkage on demand (DETECT-03) | VERIFIED (code); live behavior not executed | `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` calls `detectPhishingTicket` → `parseAndStoreMessage` → `groupReportIntoCampaign(detection.reportId)` (no skipIfAlreadyGrouped) and returns camelCase `{reportId,campaignId,groupMethod,created}`; validates ticket_id (400), 404 for missing ticket, 400 for non-phishing ticket. `npx tsc --noEmit --pretty` clean. Live curl pass explicitly deferred per 18-02-SUMMARY.md — see Human Verification |
+| 4 | `GET /api/phishing/campaigns` lists campaigns (paginated) and `GET /api/phishing/campaigns/{id}` returns full nested detail (reports/messages/indicators/classifications) (CAMP-03) | VERIFIED (code); live behavior not executed | `app/api/phishing/campaigns/route.ts` and `.../[id]/route.ts` both exist, return camelCase shapes matching the plan exactly (verified by direct read); bulk-fetch via `= ANY($1::uuid[])`, UUID validation, 404 handling all present. `npx tsc --noEmit --pretty` clean. Live curl pass explicitly deferred per 18-03-SUMMARY.md |
+| 5 | Every `/api/phishing/*` route introduced this phase calls `requireAuth()`/`requirePermission()` and rejects unauthenticated/unauthorized requests with 401/403 (ACCESS-01) | VERIFIED (code); live behavior not executed | All 4 route files (`analyze`, `campaigns`, `campaigns/[id]`) have `const { error } = await requirePermission('phishing', <action>); if (error) return error;` as their first handler statement (grep-confirmed). `lib/permissions.ts` grants `phishing: ["read","analyze"]` to admin/super-admin, `phishing: ["read"]` to user, and leaves `approve`/`remediate` ungranted to any role — matches D-05/ACCESS-01 foundation exactly |
+
+**Score:** 4/5 truths verified (CAMP-02 failed)
+
+### Required Artifacts
+
+| Artifact | Expected | Status | Details |
+|----------|----------|--------|---------|
+| `lib/services/campaign-grouping-service.ts` | `groupReportIntoCampaign`, `normalizeSubject`, `extractUrlDomain`, `GroupReportResult` exported | ✓ VERIFIED (exists+substantive+wired) but **hollow on repeat-analyze data integrity** | All exports present; tiered logic implemented; called from 3 sites (webhook, sweep, analyze route). Correctness gap: CR-01 (see Gaps) |
+| `lib/services/campaign-grouping-service.test.ts` | Unit tests for helpers + mocked-DB grouping | ✓ VERIFIED | 16 tests, all passing; does NOT cover the CR-01 sibling-re-match scenario (only covers "nothing else matches" self-exclusion) |
+| `lib/permissions.ts` | `phishing` resource + role grants | ✓ VERIFIED | `grep -c 'phishing:'` = 4; grants match spec exactly |
+| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | POST on-demand analyze | ✓ VERIFIED | Matches plan exactly; `requirePermission('phishing','analyze')` first line; detect→parse→group chain intact |
+| `lib/services/webhook-service.ts` | `groupReportIntoCampaign` wired into `triggerPhishingDetection` | ✓ VERIFIED | `groupReportIntoCampaign(detection.reportId, { skipIfAlreadyGrouped: true })` present at line 493 |
+| `lib/services/phishing-sweep-service.ts` | `groupReportIntoCampaign` wired into per-ticket sweep loop | ✓ VERIFIED | Call at line 91, inside the same try/catch as `detectPhishingTicket`, before the catch that increments `result.errors` |
+| `app/api/phishing/campaigns/route.ts` | GET paginated list | ✓ VERIFIED, with a minor known bug (WR-02, non-blocking) | `?limit=0` silently becomes 50, `?limit=-5` triggers an unhandled 500 (Postgres `LIMIT must not be negative`) — see 18-REVIEW.md WR-02 |
+| `app/api/phishing/campaigns/[id]/route.ts` | GET nested detail | ✓ VERIFIED | UUID validation, 404, bulk-fetch via `ANY($1::uuid[])`, camelCase nested assembly all present and match plan exactly |
+
+### Key Link Verification
+
+| From | To | Via | Status | Details |
+|------|----|-----|--------|---------|
+| `campaign-grouping-service.ts` | `postgresClient.transaction` | find-or-create wrapped in one transaction | ✓ WIRED | `postgresClient.transaction(async (client) => {...})` wraps the entire tiered-match + update/insert flow |
+| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | `detectPhishingTicket → parseAndStoreMessage → groupReportIntoCampaign` | orchestration chain, no skipIfAlreadyGrouped | ✓ WIRED | Confirmed via direct read, matches plan's exact call shape |
+| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | `requirePermission('phishing','analyze')` | first-line auth gate | ✓ WIRED | Confirmed |
+| `app/api/phishing/campaigns/route.ts` + `[id]/route.ts` | `requirePermission('phishing','read')` | first-line auth gate | ✓ WIRED | Confirmed in both files |
+| `app/api/phishing/campaigns/[id]/route.ts` | `reports/messages/indicators/classifications` tables | bulk-fetch via `ANY($1::uuid[])` + Map assembly | ✓ WIRED | Confirmed; empty-array short-circuit present for messages/indicators |
+| `lib/permissions.ts statement` | `phishing` resource key | `keyof typeof statement` picks up `phishing` | ✓ WIRED | `npx tsc --noEmit --pretty` exits 0, confirming `requirePermission('phishing', ...)` type-checks across all 4 route files |
+
+### Behavioral Spot-Checks
+
+| Behavior | Command | Result | Status |
+|----------|---------|--------|--------|
+| campaign-grouping-service unit tests | `npx vitest run lib/services/campaign-grouping-service.test.ts` | 16/16 passed | ✓ PASS |
+| Repository-wide type check | `npx tsc --noEmit --pretty` | exits 0, no output | ✓ PASS |
+| Live HTTP behavior of the 4 new/modified routes (401/403/200/400/404) | curl against a running dev server | not run — no safe running instance of this worktree's code available | ? SKIP (routed to Human Verification) |
+
+Step 7b note: this phase's routes require live authenticated sessions and real ticket/campaign rows in Postgres; the executor could not safely run these against the shared dev server (per both 18-02 and 18-03 SUMMARY.md's own documented deviations). No server was started as part of this verification per the "do not start servers" constraint. All 4 route files were instead verified through direct source read + `requirePermission` grep + `tsc` type-checking.
+
+### Requirements Coverage
+
+| Requirement | Source Plan | Description | Status | Evidence |
+|-------------|-------------|--------------|--------|----------|
+| CAMP-01 | 18-01, 18-02 | Message-ID → attachment-hash/URL-domain+subject+sender+window → sender+subject+client+window tiered grouping | ✓ SATISFIED | Tiered matching implemented and tested end-to-end for first-time grouping |
+| CAMP-02 | 18-01, 18-02 | Campaign accumulates many linked reports over time as duplicates are detected | ✗ BLOCKED | `report_count` accumulation is corrupted on re-run against an already-linked campaign (CR-01) — the exact metric CAMP-02 describes is not reliably accurate |
+| CAMP-03 | 18-03 | Operator can list campaigns and view full detail via API | ✓ SATISFIED | Both routes exist, correctly shaped, camelCase, gated; minor unclamped-limit bug (WR-02) does not block the requirement itself |
+| DETECT-03 | 18-02 | Operator can trigger analysis of one ticket by ID on demand | ✓ SATISFIED (code); live confirmation pending | Route implements the full orchestration chain correctly at the code level |
+| ACCESS-01 | 18-01, 18-02, 18-03 | All `/api/phishing/*` endpoints enforce existing Pulse auth conventions, approve/remediate requiring elevated permission | ✓ SATISFIED | All 4 routes gate on `requirePermission('phishing', ...)`; `approve`/`remediate` declared but ungranted to any role this phase (deferred to Phase 20 by design, matching the requirement's "requiring elevated permission beyond plain read access" — no role can call approve/remediate yet, which is the safe default) |
+
+No orphaned requirements: REQUIREMENTS.md maps exactly CAMP-01, CAMP-02, CAMP-03, DETECT-03, ACCESS-01 to Phase 18, and all five appear in at least one plan's `requirements:` frontmatter.
+
+### Anti-Patterns Found
+
+| File | Line | Pattern | Severity | Impact |
+|------|------|---------|----------|--------|
+| `lib/services/campaign-grouping-service.ts` | 156-165, 323-334 | Missing own-campaign short-circuit before incrementing `report_count` | 🛑 Blocker | Corrupts the CAMP-02 accumulation metric on repeat `/analyze` calls (CR-01, carried forward from 18-REVIEW.md) |
+| `app/api/phishing/campaigns/route.ts` | 31 | `Math.min(parseInt(...) \|\| 50, 200)` has no lower bound; `limit=0` silently becomes 50, `limit=-5` produces an unhandled 500 | ⚠️ Warning | Non-blocking for CAMP-03's core requirement but a real bug in the shipped read API (18-REVIEW.md WR-02) |
+| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` (calls `parseAndStoreMessage`) | n/a | Repeat `/analyze` calls insert duplicate `messages`/`indicators` rows (no existence check, no `ON CONFLICT`, no unique constraint on `messages.report_id`) | ⚠️ Warning | Downstream Tier 1/2 matching picks an arbitrary duplicate row (18-REVIEW.md WR-03); also means `GET /campaigns/{id}` can surface duplicate message/indicator rows |
+| `lib/services/campaign-grouping-service.ts` | 300-312 | Tier 3 query uses `INNER JOIN contacts` for a relationship where nothing from `contacts` is selected/filtered — silently drops candidates whose `requester_contact_id` doesn't resolve | ⚠️ Warning | Understates campaign membership in an edge case (18-REVIEW.md WR-04); read-path detail route correctly uses `LEFT JOIN` for the same relationship |
+| `lib/services/campaign-grouping-service.ts` | 270-273 | Tier 2 sender comparison is case-sensitive, no normalization | ⚠️ Warning | Two messages from the same sender with different address casing fail to Tier-2-match (18-REVIEW.md WR-05) |
+| `lib/services/campaign-grouping-service.ts`, `webhook-service.ts`, `phishing-sweep-service.ts` | multiple | No serialization (advisory lock) around concurrent campaign matching | ⚠️ Warning | Two reports of the same lure arriving near-simultaneously can create two separate campaigns instead of one (18-REVIEW.md WR-01) — a distinct failure mode from CR-01, not re-verified independently here but confirmed present by reading the same transaction code |
+
+No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers found in any of the 8 files this phase created/modified.
+
+### Human Verification Required
+
+### 1. On-demand analyze route — auth + validation behavior
+
+**Test:** `curl -X POST http://localhost:3100/api/phishing/tickets/{knownPhishingTicketId}/analyze` with (a) a valid admin session cookie, (b) no cookie, (c) a user-role session cookie, (d) a non-numeric `ticket_id`.
+**Expected:** (a) 200 with `{reportId,campaignId,groupMethod,created}`; (b) 401; (c) 403; (d) 400.
+**Why human:** Requires a live server running this worktree's code plus real authenticated session cookies for each role — never executed by either executor (18-02-SUMMARY.md explicitly defers this).
+
+### 2. Campaign list + detail routes — auth + shape + error handling
+
+**Test:** `curl http://localhost:3100/api/phishing/campaigns` and `.../campaigns/{id}` with an admin cookie, no cookie, a malformed id, and an absent-but-well-formed UUID.
+**Expected:** 200 camelCase paginated list / nested detail; 401 without a cookie; 400 for malformed id; 404 for absent campaign.
+**Why human:** Same constraint — 18-03-SUMMARY.md explicitly documents that the shared dev server on port 3100 was running `master`, not this worktree's new route files, so this was never executed end-to-end.
+
+### 3. Confirm CR-01 fix (once applied) with a live re-analyze scenario
+
+**Test:** Create two reports that group into the same campaign, note `report_count`, then call `POST /analyze` again on one of the two reports (which is already linked) and re-check `report_count`.
+**Expected (after fix):** `report_count` does not increase on the repeat call.
+**Why human:** Requires live Postgres state (two real linked reports) and a running server — cannot be verified via static analysis alone, and the current implementation is expected to FAIL this check until CR-01 is fixed.
+
+### Gaps Summary
+
+The read/write surface (`POST /analyze`, `GET /campaigns`, `GET /campaigns/{id}`), the auth gating (ACCESS-01), and the first-time tiered-matching logic (CAMP-01) are all genuinely implemented and match the plan precisely — this is not a stub phase. The one blocking gap is a real data-integrity bug in `groupReportIntoCampaign` (CR-01, already flagged by the phase's own code review): when the intentionally-unconditional `/analyze` re-run finds a tiered match that resolves to the report's *own current* campaign (via a sibling report, since the report's own row is excluded but a second/third member of the same campaign is not), the code has no check for "this is the campaign I'm already in" and bumps `report_count` again. Since `report_count` is the exact number both `GET /api/phishing/campaigns` and `GET /api/phishing/campaigns/{id}` surface as the primary campaign-size signal, and re-running `/analyze` on an already-grouped ticket is an explicitly documented, supported use case (not an edge case), this directly undermines CAMP-02's "accumulates... as duplicates are detected" requirement — the accumulation is real, but its count is not trustworthy. This must be fixed (select the report's current `campaign_id` and short-circuit when the match equals it) before this phase can be considered goal-complete; the fix is small and already spelled out in 18-REVIEW.md's CR-01 remediation.
+
+Secondary, non-blocking warnings (unclamped negative `limit`, duplicate `messages`/`indicators` rows on repeat `/analyze`, an inconsistent JOIN type, case-sensitive sender comparison, and a latent concurrent-campaign-creation race) are carried forward from 18-REVIEW.md for visibility but do not by themselves block the phase goal.
+
+---
+
+*Verified: 2026-07-15T20:05:00Z*
+*Verifier: Claude (gsd-verifier)*