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

test(18): persist human verification items as UAT

Browse source
This commit is contained in:
lorentz 2026-07-15 22:44:19 -04:00
parent cc2544c7ee
commit a48c0583bd
3 changed files with 129 additions and 93 deletions
Download patch file Download diff file Expand all files Collapse all files

2
.planning/ROADMAP.md
View file

@ -358,7 +358,7 @@ summarizes classification, blast radius, and recommended/approved remediation st
- [x] **Phase 15: Data Model, Detection & Ticket Evidence** — New phishing schema (migration 097) + idempotent Autotask ticket scanner + base ticket evidence capture (completed 2026-07-15)
- [x] **Phase 16: EML/MIME Evidence Parser** — Pure RFC822/MIME parser: `.eml` selection (`rfc.eml` over `OriginatingEmail.eml`), normalized headers/URLs/attachments, sanitized body preview, synthetic-fixture tests (completed 2026-07-15)
- [x] **Phase 17: Mimecast Blast Radius Lookup** — Blast-radius abstraction with graceful `unavailable` degradation when Mimecast isn't configured (completed 2026-07-15)
- [x] **Phase 18: Campaign Grouping & Phishing Analysis API** — Message-ID-first dedupe/grouping, on-demand single-ticket analysis, and the first `/api/phishing/*` routes with the ACCESS-01 auth convention (gaps found in verification 2026-07-15 — see 18-VERIFICATION.md) (completed 2026-07-16)
- [ ] **Phase 18: Campaign Grouping & Phishing Analysis API** — Message-ID-first dedupe/grouping, on-demand single-ticket analysis, and the first `/api/phishing/*` routes with the ACCESS-01 auth convention (5/5 truths verified 2026-07-16, human verification pending — see 18-VERIFICATION.md)
- [ ] **Phase 19: Classification Engine** — Deterministic SPAM/UNWANTED/THREAT rule classifier over bounded structured evidence, KnowBe4-simulation guard, (re-)trigger API
- [ ] **Phase 20: Remediation, Approval & Audit Safety** — Proposed-only remediation actions, approve/remediate/mark-false-positive APIs, idempotent re-run, full audit trail
- [ ] **Phase 21: Autotask Triage Note** — Sanitized internal triage note posted via existing safe note-write path, or returned via API if no such path exists

36
.planning/phases/18-campaign-grouping-phishing-analysis-api/18-HUMAN-UAT.md Normal file
View file

@ -0,0 +1,36 @@
---
status: partial
phase: 18-campaign-grouping-phishing-analysis-api
source: [18-VERIFICATION.md]
started: 2026-07-16T02:44:01Z
updated: 2026-07-16T02:44:01Z
---
## Current Test
[awaiting human testing]
## Tests
### 1. On-demand analyze route — auth + validation behavior
expected: `curl -X POST http://localhost:3100/api/phishing/tickets/{knownPhishingTicketId}/analyze` with (a) valid admin session cookie → 200 with `{reportId,campaignId,groupMethod,created}`; (b) no cookie → 401; (c) user-role session cookie → 403; (d) non-numeric `ticket_id` → 400
result: [pending]
### 2. Campaign list + detail routes — auth + shape + error handling (incl. fixed `?status=` filter)
expected: `curl http://localhost:3100/api/phishing/campaigns`, `.../campaigns?status=open`, and `.../campaigns/{id}` with admin cookie → 200 camelCase paginated list / nested detail (status filter now works, previously 500'd); no cookie → 401; malformed id → 400; absent-but-well-formed UUID → 404
result: [pending]
### 3. CR-02 escalation — scoping decision (policy call, not a runtime test)
expected: A developer decision on record — either accept the cross-campaign migration `report_count` staleness (CR-02) as a documented limitation via an `overrides:` entry in 18-VERIFICATION.md, or open a follow-up gap-closure plan targeting it before Phase 19/20 depend on `report_count` for weighting. Full reasoning in 18-VERIFICATION.md's "CR-02 Escalation" section.
result: [pending]
## Summary
total: 3
passed: 0
issues: 0
pending: 3
skipped: 0
blocked: 0
## Gaps

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

@ -1,58 +1,34 @@
---
phase: 18-campaign-grouping-phishing-analysis-api
verified: 2026-07-15T20:05:00Z
status: gaps_found
score: 4/5 must-haves verified
verified: 2026-07-16T02:45:00Z
status: human_needed
score: 5/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)"
re_verification:
previous_status: gaps_found
previous_score: 4/5
gaps_closed:
- "A campaign accumulates additional linked ticket reports over time as duplicates are detected, without corrupting report_count (CAMP-02) — same-campaign sibling re-match no longer double-increments report_count"
gaps_remaining: []
regressions: []
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"
why_human: "Still never executed against a live server with real session cookies (carried forward from initial verification — code unchanged since then, so the deferral still applies); code-level requirePermission('phishing','analyze') gate confirmed by static read only"
- test: "curl GET /api/phishing/campaigns (including ?status=open) and GET /api/phishing/campaigns/{id} with admin cookie, no cookie, malformed id, absent id"
expected: "200 camelCase paginated list / nested detail (admin), including status-filtered requests now that the count-query bind-parameter bug is fixed; 401 (no cookie); 400 (malformed UUID); 404 (absent campaign)"
why_human: "Still never executed against a live server running this worktree's code (carried forward). This item is now MORE important than before the re-verification: the orchestrator's direct fix (commit 650f9b8) to the ?status= 500 has only been confirmed by static read + a standalone Node simulation of the SQL placeholder numbering in this report, not by an actual Postgres round-trip"
- test: "Escalation decision: accept or reject CR-02 (cross-campaign migration leaves the origin campaign's report_count permanently stale) as an in-scope fast-follow vs. a documented, accepted limitation for this phase"
expected: "A human decision on record (either an explicit override added to this file, or a new gap-closure plan targeting CR-02)"
why_human: "This is a policy/scope call, not a testability gap — see 'CR-02 escalation' in Gaps Summary below for the full analysis and why the verifier partially disagrees with the original 'if ever supported' scoping language"
---
# 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
**Verified:** 2026-07-16T02:45:00Z
**Status:** human_needed
**Re-verification:** Yes — after gap closure (plan 18-04) plus one orchestrator-applied fix (commit `650f9b8`) surfaced by a follow-up code review
## Goal Achievement
@ -60,72 +36,92 @@ human_verification:
| # | 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 |
| 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-319` — tiered logic unchanged since initial verification. `npx vitest run lib/services/campaign-grouping-service.test.ts` → 17/17 pass (16 original + 1 new regression test, no regressions) |
| 2 | A campaign accumulates additional linked reports over time as duplicates are detected, without ever creating a second campaign for the same key, and `report_count` stays accurate for the common (same-campaign) re-analyze case (CAMP-02) | ✓ VERIFIED (with a scoped, disclosed caveat — see CR-02 escalation below) | `groupReportIntoCampaign` now selects `ownReport.campaign_id` (`campaign-grouping-service.ts:158`) and short-circuits to a no-op (`created:false`, zero UPDATEs) when a tiered match resolves back to the report's own current campaign (`:332-334`). New regression test `campaign-grouping-service.test.ts:337-362` asserts zero `UPDATE campaigns` / `INSERT INTO campaigns` / `UPDATE reports SET campaign_id` calls for exactly the sibling-re-match scenario that was previously double-incrementing. Direct source read confirms the fix; 18-REVIEW.md's re-review (dated 2026-07-16T02:32:12Z, after the fix landed) independently confirms CR-01 fixed. **Caveat:** the fix only covers "match resolves to the report's OWN current campaign" — it does not cover "match resolves to a genuinely different, already-existing campaign" (a report moving from campaign A to campaign B), which leaves campaign A's `report_count` permanently stale in that narrower scenario (CR-02, still open — see escalation below) |
| 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` unchanged since initial verification (confirmed via `git log` — no commits touch this file after `18-02`). Chain intact: `detectPhishingTicket` → `parseAndStoreMessage` → `groupReportIntoCampaign(detection.reportId)`. `npx tsc --noEmit --pretty` exits 0. Live curl pass still deferred — 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` now clamps `limit` via `Number.isFinite(...) ? rawLimit : 50` + `Math.min(Math.max(...,0),200)` (verified with a standalone Node simulation: `limit=-5`→0, `limit=0`→0, `limit=abc`→50, `limit=9999`→200 — no more unhandled Postgres 500 for negative limits). The pre-existing `?status=` bind-parameter mismatch (the count query reused the list query's `$3` placeholder text against a 1-element params array) is fixed in commit `650f9b8`: the count query now builds its own independent `countParams`/`countFilter` (verified by direct read, lines 53-63, and a standalone simulation confirming `$1` always matches exactly one bound param). `app/api/phishing/campaigns/[id]/route.ts` unchanged since initial verification — UUID validation, 404, bulk-fetch via `ANY($1::uuid[])`, camelCase nested assembly all still present. `npx tsc --noEmit --pretty` exits 0. Live curl pass still deferred — see Human Verification |
| 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 unchanged since initial verification (`git log` confirms no commits since `18-03`/`18-04` touch the `requirePermission` call sites). `lib/permissions.ts` unchanged: `phishing: ["read","analyze"]` for admin/super-admin, `phishing: ["read"]` for user, `approve`/`remediate` ungranted to any role |
**Score:** 4/5 truths verified (CAMP-02 failed)
**Score:** 5/5 truths verified (CAMP-02 gap from prior verification closed; one narrower, disclosed caveat remains open pending a human scoping decision — see below)
### CR-02 Escalation (Human Decision Required)
**What it is:** `groupReportIntoCampaign`'s CR-01 fix only guards the case where a tiered match resolves to the report's own *current* campaign. It does not guard the case where `ownReport.campaign_id` is a *different*, already-existing campaign (a genuine campaign-to-campaign migration). Concretely: report X is Tier-3-grouped into campaign A (`report_count=1`). Later, `/analyze` is re-run on X after `parseAndStoreMessage` populates its message row, and Tier 1 now finds a *different* pre-existing campaign B (formed by another report sharing the same Message-ID). Since `B !== A`, the guard added by 18-04 doesn't trigger: campaign B is correctly incremented and X is correctly re-linked to B, but campaign A's `report_count` is never decremented — A is left permanently showing `report_count=1` with zero actually-linked reports, indefinitely surfaced by `GET /api/phishing/campaigns`. Confirmed present by direct read of `campaign-grouping-service.ts:324-346` (no decrement of `ownReport.campaign_id` anywhere in the fall-through branch) and independently identified in 18-REVIEW.md as CR-02.
**Why this needs a human call, not just a verifier judgment:**
- The *previous* verification pass explicitly scoped this out, but with conditional language: *"If report-moves-between-campaigns is ever supported, decrement the origin campaign's report_count..."* That premise is not accurate — this migration path **is** supported today, by explicit design (D-08 in `18-CONTEXT.md`: `/analyze` intentionally omits `skipIfAlreadyGrouped` specifically so it can "upgrade an existing Tier-3-only grouping to a Tier-1 match"). Given D-07's stated limitation that the automatic webhook/cron path can only ever reach Tier 3 until a report has been through at least one explicit `/analyze` call, **every** report that later gets `/analyze`'d and has a Tier-1/2-eligible sibling is a candidate for this exact migration — this is not a rare theoretical edge the way D-04's "two campaigns match simultaneously" scenario is.
- Against that: this is a narrower failure mode than the original CR-01 bug (CR-01 fired on *every* repeat `/analyze` call against any multi-report campaign; CR-02 only fires when the tiered match resolves to a *different pre-existing* campaign than the report's current one — it requires two independently-created campaigns for what turns out to be the same underlying lure). The literal REQUIREMENTS.md text for CAMP-02 ("A campaign can accumulate many linked ticket reports and recipients over time as duplicates are detected") is satisfied in the common case; report_count only goes stale on an *abandoned* campaign, not on the campaign an operator is actually looking at for the report they just analyzed. This also matches the project's own documented precedent (`17-CONTEXT.md` D-05, referenced in `18-CONTEXT.md`) of "simple now, documented limitation over silent gap, defer complexity" for exactly this class of rare-but-real cross-entity reconciliation problem.
- The fix itself is well-understood and small (spelled out verbatim in 18-REVIEW.md's CR-02 remediation) — this is a scope/priority call, not a technical blocker.
**Verifier's recommendation:** Treat as a disclosed, non-blocking limitation for this phase (consistent with, but more precisely reasoned than, the original scoping call), and open a small follow-up plan before Phase 19/20 relies heavily on `report_count` for classification/triage weighting. **Escalating rather than silently accepting** because the original "if ever supported" framing was based on an incorrect premise, and a developer should explicitly confirm this trade-off rather than have it re-inherited unchanged.
To formally accept this as a documented limitation rather than opening a gap-closure plan, add to this file's frontmatter:
```yaml
overrides:
- must_have: "A campaign accumulates additional linked ticket reports over time... without corrupting report_count (CAMP-02)"
reason: "CR-02 cross-campaign migration decrement gap accepted as a documented limitation, consistent with D-04's rare-edge-case scoping philosophy; tracked for a future fast-follow before Phase 19/20 depend on report_count for weighting"
accepted_by: "<name>"
accepted_at: "<ISO timestamp>"
```
### 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 |
| `lib/services/campaign-grouping-service.ts` | `groupReportIntoCampaign`, `normalizeSubject`, `extractUrlDomain`, `GroupReportResult` exported; own-campaign re-match guard | ✓ VERIFIED (exists+substantive+wired) | `OwnReportRow` now includes `campaign_id: string | null` (line 69); own-report SELECT fetches `campaign_id::text` (line 158); guard at lines 332-334. CR-02 caveat noted above but does not regress CAMP-01/first-time-grouping correctness |
| `lib/services/campaign-grouping-service.test.ts` | Unit tests for helpers + mocked-DB grouping, including the CAMP-02/CR-01 regression | ✓ VERIFIED | 17/17 tests pass. New test at lines 337-362 asserts zero mutating calls on sibling re-match. Per 18-REVIEW.md's WR-02 finding, coverage does not yet extend the same-campaign guard check to Tier 1/2 (only Tier 3 tested) nor cover the CR-02 migration path — non-blocking test-coverage gap, not a functional regression |
| `app/api/phishing/campaigns/route.ts` | GET paginated list, clamped `limit`, correct `?status=` filtering on both list and count queries | ✓ VERIFIED | `limit` clamp confirmed via direct read + standalone simulation (lines 31-32); count-query bind fix confirmed via direct read + standalone simulation (lines 53-63, commit `650f9b8`) |
| `app/api/phishing/campaigns/[id]/route.ts` | GET nested detail | ✓ VERIFIED (unchanged, regression-checked) | No commits since `18-03` touch this file; still matches plan |
| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | POST on-demand analyze | ✓ VERIFIED (unchanged, regression-checked) | No commits since `18-02` touch this file |
| `lib/services/webhook-service.ts`, `lib/services/phishing-sweep-service.ts` | `groupReportIntoCampaign` wired with `skipIfAlreadyGrouped: true` | ✓ VERIFIED (unchanged, regression-checked) | Not touched by 18-04 or the orchestrator fix; no reason to re-derive from scratch — call sites confirmed present in initial verification and untouched since (`git log` clean for both files after `18-02`) |
| `lib/permissions.ts` | `phishing` resource + role grants | ✓ VERIFIED (unchanged, regression-checked) | `phishing: ["read","analyze","approve","remediate"]` declared; only `read`/`analyze` granted to any role |
### 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 |
| `campaign-grouping-service.ts` groupReportIntoCampaign | own-campaign no-op guard | `matchCampaignId === ownReport.campaign_id` before `UPDATE campaigns` | ✓ WIRED | Confirmed at lines 332-334; regression test proves it fires and short-circuits both UPDATEs |
| `app/api/phishing/campaigns/route.ts` list query | count query | independent `countParams`/`countFilter`, not reused from the list query's `statusFilter` | ✓ WIRED | Confirmed at lines 53-63 (commit `650f9b8`); standalone simulation confirms `$1` placeholder always matches exactly 1 bound param regardless of `status` presence |
| `app/api/phishing/campaigns/route.ts` | `Number.isFinite` limit clamp | `Math.min(Math.max(...,0),200)` | ✓ WIRED | Confirmed at line 32; simulation confirms `-5→0`, `0→0`, `abc→50`, `9999→200`, no path reaches `LIMIT` with a negative value |
| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | `detectPhishingTicket → parseAndStoreMessage → groupReportIntoCampaign` | orchestration chain, no `skipIfAlreadyGrouped` | ✓ WIRED (unchanged) | Regression-checked, no commits since `18-02` |
| All 4 `/api/phishing/*` route files | `requirePermission('phishing', <action>)` | first-line auth gate | ✓ WIRED (unchanged) | Regression-checked, no commits touch these call sites since `18-01`-`18-03` |
### 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 |
| campaign-grouping-service unit tests (incl. new regression) | `npx vitest run lib/services/campaign-grouping-service.test.ts` | 17/17 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) |
| `limit` clamp logic simulated standalone | `node -e "..."` reproducing the exact clamp expression | `-5→0`, `0→0`, `abc→50`, `9999→200`, `null→50` | ✓ PASS |
| Count-query placeholder numbering simulated standalone | `node -e "..."` reproducing the exact `countParams`/`countFilter` build | `status='open'` → `WHERE status = $1` with `params=['open']`; `status=null` → no WHERE, `params=[]` | ✓ PASS |
| Live HTTP behavior of the 4 routes (401/403/200/400/404, incl. `?status=` filter) | 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.
Step 7c note: no `scripts/*/tests/probe-*.sh` files exist for this phase and none are referenced by any PLAN/SUMMARY file; probe execution step is not applicable here.
### 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) |
| CAMP-01 | 18-01, 18-02 | Message-ID → attachment-hash/URL-domain+subject+sender+window → sender+subject+client+window tiered grouping | ✓ SATISFIED | Unchanged, regression-checked; 17/17 tests green |
| CAMP-02 | 18-01, 18-02, 18-04 | Campaign accumulates many linked reports over time as duplicates are detected | ✓ SATISFIED (with disclosed CR-02 caveat requiring a human scoping decision — see escalation above) | CR-01 double-increment fixed and regression-tested; CR-02 cross-campaign migration decrement gap remains open, narrower in scope, flagged for explicit human accept/reject |
| CAMP-03 | 18-03, 18-04 | Operator can list campaigns and view full detail via API | ✓ SATISFIED | Both routes exist, correctly shaped, camelCase, gated; `limit` clamp (WR-02) and `?status=` count-query bind bug (new CR-01) both fixed and verified |
| DETECT-03 | 18-02 | Operator can trigger analysis of one ticket by ID on demand | ✓ SATISFIED (code); live confirmation pending | Unchanged, regression-checked |
| ACCESS-01 | 18-01, 18-02, 18-03 | All `/api/phishing/*` endpoints enforce existing Pulse auth conventions, approve/remediate requiring elevated permission | ✓ SATISFIED | Unchanged, regression-checked |
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.
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 (including 18-04's `[CAMP-02, CAMP-03]`).
### 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 |
| `lib/services/campaign-grouping-service.ts` | 324-346 | CR-02: cross-campaign migration (report moves from campaign A to a different, already-existing campaign B) never decrements A's `report_count` | ⚠️ Warning — escalated for human decision | Leaves an abandoned campaign's count permanently stale; narrower and less frequent than the now-fixed CR-01, but a real, non-hypothetical gap given D-07/D-08's documented Tier-3-first-then-upgrade flow. See CR-02 Escalation section above |
| `lib/services/campaign-grouping-service.test.ts` | 337-362 | New regression test only exercises the same-campaign guard via Tier 3; doesn't cover Tier 1/2 same-campaign guard nor CR-02 | ℹ️ Info | Coverage gap noted by 18-REVIEW.md's WR-02 (its own, distinct from the WR-02 fixed by 18-04's Task 2); does not indicate a functional regression, only an untested code path |
| `lib/services/campaign-grouping-service.ts` | 241-251 | Tier 2 candidate query has no `ORDER BY` (non-deterministic pick among multiple qualifying siblings) | ⚠️ Warning | Carried forward from 18-REVIEW.md WR-01; pre-existing, not touched by 18-04 or the orchestrator fix |
| `app/api/phishing/campaigns/route.ts` | 32 | `limit<=0` silently returns an empty page rather than falling back to the documented default of 50 | ℹ️ Info | Carried forward from 18-REVIEW.md IN-01; a footgun, not a crash — the original WR-02 (500 on negative limit) is fixed |
| `app/api/phishing/campaigns/route.ts` | 34 | `status` param not validated against known enum values | ℹ️ Info | Carried forward from 18-REVIEW.md IN-02; consistent with project's "no Zod unless it matters" convention |
| `lib/services/campaign-grouping-service.ts` | 125-126 | `campaigns.campaign_key` has no UNIQUE constraint; no row locking around concurrent find-or-create | ⚠️ Warning | Carried forward from initial verification / 18-REVIEW.md IN-03; pre-existing, not touched by this pass |
No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers found in any of the 8 files this phase created/modified.
No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers found in any of the files touched by 18-04 or the orchestrator's direct fix (`lib/services/campaign-grouping-service.ts`, `lib/services/campaign-grouping-service.test.ts`, `app/api/phishing/campaigns/route.ts`).
### Human Verification Required
@ -133,27 +129,31 @@ No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers found in any of the 8
**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).
**Why human:** Carried forward — code is unchanged since the initial verification, and this was never executed against a live server with real session cookies.
### 2. Campaign list + detail routes — auth + shape + error handling
### 2. Campaign list + detail routes — auth + shape + error handling, now including the fixed `?status=` filter
**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.
**Test:** `curl http://localhost:3100/api/phishing/campaigns`, `.../campaigns?status=open`, 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 (including successful `?status=` filtering, which previously 500'd); 401 without a cookie; 400 for malformed id; 404 for absent campaign.
**Why human:** Carried forward, and now higher-priority than before: the `?status=` fix (commit `650f9b8`) has only been confirmed via static read and a standalone Node simulation of the SQL text/param shapes in this report — never against a real Postgres connection.
### 3. Confirm CR-01 fix (once applied) with a live re-analyze scenario
### 3. CR-02 escalation — scoping decision
**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.
**Test:** N/A (not a runtime test) — a developer must explicitly decide whether the cross-campaign migration `report_count` staleness (CR-02) is accepted as a documented limitation for this phase, or whether a small follow-up gap-closure plan should be opened before Phase 19/20 depend on `report_count`.
**Expected:** An explicit decision recorded (either an `overrides:` entry in this file, per the template above, or a new plan targeting CR-02).
**Why human:** Policy/scope trade-off, not something resolvable by static analysis — see the full CR-02 Escalation section above for the verifier's reasoning and partial disagreement with the original "if ever supported" scoping language.
### 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.
The blocking CAMP-02 gap from the initial verification (CR-01: unbounded `report_count` double-increment on any repeat `/analyze` call against a multi-report campaign) is now closed and regression-tested — this was the single item that previously prevented phase completion, and it is fixed correctly. The orchestrator's direct fix to the pre-existing `?status=` count-query bind-parameter mismatch (commit `650f9b8`, surfaced by the 18-04 gap-closure re-review) is also confirmed correct by direct code read and a standalone simulation of the exact SQL/param construction.
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.
One new, narrower finding (CR-02) was surfaced by the same re-review: `groupReportIntoCampaign`'s fix only guards the "match resolves to my own current campaign" case, not the "match resolves to a different, already-existing campaign" case (a genuine cross-campaign migration, which — per D-08 — is an intentional, designed possibility for the `/analyze` route, not a hypothetical). This leaves an abandoned campaign's `report_count` permanently stale in that specific scenario. The verifier does not believe this rises to the same severity as the now-fixed CR-01 (it requires two independently-formed campaigns for the same underlying lure, not just a routine repeat call), and it is consistent with — though more precisely reasoned than — the original verification's scoping decision to defer this class of fix. It is escalated here as a human decision point rather than silently re-accepted, because the original "if ever supported" framing turns out to be factually inaccurate (the migration path is supported today, by design).
No regressions were found in any of the previously-verified truths, artifacts, or key links — the routes and permission wiring untouched by 18-04/the orchestrator fix remain exactly as they were at the initial verification pass (confirmed via `git log` showing no intervening commits touch those files).
The remaining blockers to a clean `passed` status are: (1) the two carried-forward live-curl human verification items (never executed against a running server in either verification pass), and (2) the CR-02 scoping decision, which is a policy call rather than a code defect requiring a fix before this report can be finalized.
---
*Verified: 2026-07-15T20:05:00Z*
*Verified: 2026-07-16T02:45:00Z*
*Verifier: Claude (gsd-verifier)*