From a494cd0c0be44ba779db042b9527a77fe5953f88 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 16:50:46 -0400 Subject: [PATCH] docs(18): fix plan-checker warnings (tier self-exclusion + resolved open questions) --- .../18-campaign-grouping-phishing-analysis-api/18-01-PLAN.md | 4 +++- .../18-campaign-grouping-phishing-analysis-api/18-RESEARCH.md | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-01-PLAN.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-01-PLAN.md index a481a22..106a913 100644 --- a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-01-PLAN.md +++ b/.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 - 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`. 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`. 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. npx vitest run lib/services/campaign-grouping-service.test.ts && npx tsc --noEmit --pretty @@ -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 diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-RESEARCH.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-RESEARCH.md index 65cafb4..5c64de0 100644 --- a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-RESEARCH.md +++ b/.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.