docs(18): fix plan-checker warnings (tier self-exclusion + resolved open questions)

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

@@ -141,7 +141,7 @@ From lib/permissions.ts (existing structure — closest analog is the rmm resour
     - skipIfAlreadyGrouped:true returns null immediately when reports.campaign_id IS NOT NULL (D-08); omitting the option always re-runs full tiered matching
   </behavior>
   <action>
-    Extend `campaign-grouping-service.ts` with `export interface GroupReportResult { campaignId: string; groupMethod: 'message_id' | 'attachment_or_url' | 'sender_subject_client'; created: boolean; }` and `export async function groupReportIntoCampaign(reportId: string, opts?: { skipIfAlreadyGrouped?: boolean }): Promise<GroupReportResult | null>`. When `opts.skipIfAlreadyGrouped` is set, run a pre-check `SELECT campaign_id FROM reports WHERE id = $1` and return null if already grouped (D-08). Otherwise run the whole find-or-create inside `postgresClient.transaction(async (client) => { ... })` using `client.query` (never `postgresClient.query`) so all reads and writes share one transaction (Pitfall 2 — campaign_key has no UNIQUE constraint). Implement the three tiers in order, each gated on the previous returning nothing: Tier 1 keys on `messages.message_id` (join messages->reports where campaign_id IS NOT NULL); Tier 2 keys on `indicators` rows (indicator_type 'attachment_hash' matched by value, 'url' matched by extractUrlDomain(value) at read time) plus normalizeSubject and sender within a 24h window (D-02); Tier 3 keys on sender (reports.requester_contact_id -> contacts) + normalizeSubject(title) + company_id within 24h (D-02). Compute keys in JS then run targeted parameterized queries (never encode fuzzy tier logic in one WHERE clause — RESEARCH anti-pattern). On match, UPDATE campaigns (report_count+1, last_seen_at=NOW) + UPDATE reports.campaign_id + updated_at=NOW. On no match, INSERT a campaigns row with the computed campaign_key/group_method + link the report. Do NOT set/transition campaigns.status (leave the migration default 'open' — status transitions are Phase 19/20). Do NOT implement multi-campaign merge (D-04 — attach to first/best match). Add a file-level doc comment stating the D-07 limitation explicitly: the automatic webhook/cron path only ever reaches Tier 3 until a report has been through an explicit `/analyze` call, because parseAndStoreMessage (the only writer of messages/indicators) is not wired into the automatic path this phase. Wrap the body in try/catch with `console.error('[CAMPAIGN-GROUPING] ...', reportId, error)` + rethrow. Add mocked-DB tests to the test file covering the behavior cases above (transactionMock invokes its callback with a fake client whose query returns staged rows).
+    Extend `campaign-grouping-service.ts` with `export interface GroupReportResult { campaignId: string; groupMethod: 'message_id' | 'attachment_or_url' | 'sender_subject_client'; created: boolean; }` and `export async function groupReportIntoCampaign(reportId: string, opts?: { skipIfAlreadyGrouped?: boolean }): Promise<GroupReportResult | null>`. When `opts.skipIfAlreadyGrouped` is set, run a pre-check `SELECT campaign_id FROM reports WHERE id = $1` and return null if already grouped (D-08). Otherwise run the whole find-or-create inside `postgresClient.transaction(async (client) => { ... })` using `client.query` (never `postgresClient.query`) so all reads and writes share one transaction (Pitfall 2 — campaign_key has no UNIQUE constraint). Implement the three tiers in order, each gated on the previous returning nothing: Tier 1 keys on `messages.message_id` (join messages->reports where campaign_id IS NOT NULL AND reports.id != $reportId); Tier 2 keys on `indicators` rows (indicator_type 'attachment_hash' matched by value, 'url' matched by extractUrlDomain(value) at read time) plus normalizeSubject and sender within a 24h window (D-02), also excluding `reports.id = $reportId` from the candidate set; Tier 3 keys on sender (reports.requester_contact_id -> contacts) + normalizeSubject(title) + company_id within 24h (D-02), same `reports.id != $reportId` exclusion. Every tier query MUST exclude the report currently being grouped from its own candidate set (`AND r.id != $reportId` or equivalent) — without this, re-running `/analyze` on an already-grouped report (D-08 always re-runs, no skip) can match the report's own `messages`/`indicators` row against itself and double-increment `report_count` for the same report (plan-checker finding). Compute keys in JS then run targeted parameterized queries (never encode fuzzy tier logic in one WHERE clause — RESEARCH anti-pattern). On match, UPDATE campaigns (report_count+1, last_seen_at=NOW) + UPDATE reports.campaign_id + updated_at=NOW. On no match, INSERT a campaigns row with the computed campaign_key/group_method + link the report. Do NOT set/transition campaigns.status (leave the migration default 'open' — status transitions are Phase 19/20). Do NOT implement multi-campaign merge (D-04 — attach to first/best match). Add a file-level doc comment stating the D-07 limitation explicitly: the automatic webhook/cron path only ever reaches Tier 3 until a report has been through an explicit `/analyze` call, because parseAndStoreMessage (the only writer of messages/indicators) is not wired into the automatic path this phase. Wrap the body in try/catch with `console.error('[CAMPAIGN-GROUPING] ...', reportId, error)` + rethrow. Add mocked-DB tests to the test file covering the behavior cases above (transactionMock invokes its callback with a fake client whose query returns staged rows), including a test asserting that re-running `groupReportIntoCampaign(reportId)` (no `skipIfAlreadyGrouped`) on a report that is already linked to a campaign, and whose own `messages`/`indicators` rows would otherwise satisfy Tier 1 against itself, does NOT increment that campaign's `report_count` a second time for the same report.
   </action>
   <verify>
     <automated>npx vitest run lib/services/campaign-grouping-service.test.ts && npx tsc --noEmit --pretty</automated>
@@ -153,6 +153,8 @@ From lib/permissions.ts (existing structure — closest analog is the rmm resour
     - A file-level comment names the D-07 Tier-3-only automatic-path limitation (grep for `Tier 3` or `parseAndStoreMessage` in a comment near the top)
     - Test asserts: a matching second report increments report_count and does NOT insert a second campaign (transactionMock/queryMock call assertions)
     - Test asserts: skipIfAlreadyGrouped:true returns null when the pre-check SELECT reports a non-null campaign_id
+    - Every tier query string in the implementation contains a self-exclusion clause (e.g. `!= $` against the report's own id) — grep the source for the exclusion parameter alongside each tier's WHERE clause
+    - Test asserts: re-running groupReportIntoCampaign (no skipIfAlreadyGrouped) on an already-grouped report whose own messages/indicators would otherwise self-match does not increment report_count a second time for that report
     - `npx vitest run lib/services/campaign-grouping-service.test.ts` exits 0; `npx tsc --noEmit --pretty` exits 0
   </acceptance_criteria>
 </task>

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

@@ -536,7 +536,7 @@ above are read directly from this repo's current `master` branch as of 2026-07-1
 migration files, service files, route files, and config, not from training-data
 assumptions about how this codebase "probably" works.
 
-## Open Questions
+## Open Questions (RESOLVED)
 
 1. **(RESOLVED: see CONTEXT.md D-07 — do NOT auto-wire, document Tier-3-only automatic-path limitation)** Should this phase also wire `parseAndStoreMessage()` into the automatic
    webhook/cron path, or leave it reachable only via the on-demand `/analyze` endpoint?**
@@ -575,7 +575,7 @@ assumptions about how this codebase "probably" works.
      operator request and may follow a fresh `/analyze` call that just populated
      `messages`/`indicators` for the first time, deserving a chance to upgrade to Tier 1).
 
-3. **Does `lib/auth-utils.ts` import `statement`/`hasPermission` from `lib/permissions.ts`
+3. **(RESOLVED: confirmed — `lib/auth-utils.ts:4` does `import { hasPermission, type Permission } from "./permissions"`; also independently confirmed in 18-PATTERNS.md §6 as "Assumption A1 resolved")** Does `lib/auth-utils.ts` import `statement`/`hasPermission` from `lib/permissions.ts`
    directly?** (See Assumption A1.) Confirm the import path when adding the `phishing`
    resource so it lands in the file `requirePermission()` actually reads from.