docs(18): extend 18-05 gap-closure plan to decrement origin on signal-diverged create-new too

This commit is contained in:
lorentz 2026-07-16 00:02:53 -04:00
parent 9c69b9033c
commit b7a5273848

View file

@ -0,0 +1,243 @@
---
phase: 18-campaign-grouping-phishing-analysis-api
plan: 05
type: execute
wave: 1
depends_on: ["18-01", "18-04"]
files_modified:
- lib/services/campaign-grouping-service.ts
- lib/services/campaign-grouping-service.test.ts
autonomous: false
gap_closure: true
requirements: [CAMP-01, CAMP-02]
tags: [postgres, vitest, campaign-grouping, phishing, idempotency]
must_haves:
truths:
- "Re-analyzing a single-report campaign (no sibling report exists yet) returns the report's OWN existing campaign_id with created:false — no duplicate campaigns row with the same campaign_key is ever created (CR-03 closed)."
- "On that re-analyze, the report's existing campaign report_count is unchanged (no increment) and no second campaigns row is inserted and the report is not re-linked away from its valid campaign."
- "A report that gains a genuinely BETTER sibling match on re-analyze still upgrades to that sibling's campaign (D-08 preserved) — the own-campaign guard runs only when NO sibling matched, so it never suppresses a legitimate upgrade."
- "First-time grouping of a genuinely new report (no existing campaign_id, no sibling) still creates exactly one new campaign — no regression to the create-new path."
- "(CR-02, bundled) When a report migrates from campaign A to a DIFFERENT existing campaign B on re-analyze, campaign A's report_count is decremented so A no longer shows a stale count for a report it no longer holds."
- "(plan-checker warning, closed) When the own-campaign guard falls through to create-new because the report's signal genuinely diverged from its stored campaign_key (not just 'no sibling found'), the abandoned origin campaign's report_count is also decremented — this is the same abandonment as CR-02's cross-campaign migration, just landing on a brand-new campaign instead of an existing sibling's, so it must not be left stale either."
artifacts:
- path: "lib/services/campaign-grouping-service.ts"
provides: "own-campaign revalidation guard before the create-new fall-through; origin-campaign decrement on cross-campaign migration"
contains: "groupReportIntoCampaign"
- path: "lib/services/campaign-grouping-service.test.ts"
provides: "regression tests for CR-03 (single-report re-analyze), no-regression create-new, D-08 sibling upgrade, and CR-02 origin decrement"
key_links:
- from: "lib/services/campaign-grouping-service.ts groupReportIntoCampaign"
to: "campaigns row for ownReport.campaign_id"
via: "SELECT campaign_key, group_method WHERE id = ownReport.campaign_id, compared against freshly-recomputed current tier keys before falling through to create"
pattern: "SELECT campaign_key"
coverage_caveat: "Mocked-only unit coverage was INSUFFICIENT last time — the 18-04 mocked regression test passed while CR-03 shipped and reproduced live in production. This codebase has NO real-Postgres integration test harness (all tests mock postgresClient; vitest include is lib/**/*.test.ts). Therefore the automated tests here are still mocked, and a BLOCKING human live-verification checkpoint (Task 3) is REQUIRED to prove no-duplicate-on-reanalyze against a real database before this gap is considered closed."
---
<objective>
Close CR-03 (BLOCKING): re-analyzing a single-report campaign — the common case for any brand-new phishing lure before a second person reports it — creates a DUPLICATE `campaigns` row with the identical `campaign_key` instead of reusing the report's own existing campaign. Confirmed live against production (`POST /api/phishing/tickets/627088/analyze` run twice → two campaigns with the same `campaign_key`).
Root cause: all three tiers in `groupReportIntoCampaign` search only for SIBLING reports that already have a campaign (`r.id != $x` self-exclusion on every tier query). None check whether the calling report's OWN current campaign already satisfies its freshly-computed tier key. For a one-report campaign, every tier returns zero rows, `matchCampaignId` stays null, and control falls through to "create new campaign", abandoning the report's still-valid campaign. The 18-04 no-op guard (`matchCampaignId === ownReport.campaign_id`) can never fire in this path because it only runs when a sibling WAS found.
This plan also bundles CR-02 (companion, lower-severity): a genuine cross-campaign migration (report moves from campaign A to a different existing campaign B) never decrements A's `report_count`, leaving A permanently stale. Both stem from the same missing "check my own existing campaign" step; the CR-02 decrement is a small, well-understood, closely-coupled addition and is bundled here per the gap author's guidance. Per plan-checker review, the CR-02 decrement is applied in BOTH places a report can abandon its origin campaign: the existing sibling-migration branch, and Task 1's own signal-diverged create-new fall-through (a new campaign, not a sibling's, but the same abandonment).
Purpose: restore the core CAMP-01/CAMP-02 guarantee — "never create a second campaign for the same key" — and keep `report_count` accurate under repeat `/analyze`, which Phase 19/20 will rely on for classification/triage weighting.
Output: a revalidation guard + origin decrement in `campaign-grouping-service.ts`, mocked regression tests that stage the exact previously-missed scenarios, and a live human verification against a real database.
</objective>
<execution_context>
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
@$HOME/.claude/get-shit-done/templates/summary.md
</execution_context>
<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/18-campaign-grouping-phishing-analysis-api/18-CONTEXT.md
@.planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md
@lib/services/campaign-grouping-service.ts
@lib/services/campaign-grouping-service.test.ts
<interfaces>
<!-- Contracts the executor needs. Already in the codebase — no exploration required. -->
From lib/services/campaign-grouping-service.ts:
export interface GroupReportResult {
campaignId: string;
groupMethod: 'message_id' | 'attachment_or_url' | 'sender_subject_client';
created: boolean;
}
interface OwnReportRow { // already includes campaign_id: string | null (added by 18-04)
title, requester_contact_id, company_id, created_at, campaign_id
}
// Pure key builders already exist and are used in the create-new block:
computeTier1Key(messageId): string | null // `message_id:<id>` or null
computeTier2Key(hashes, domains, subj, sender): string | null // `attachment_or_url:...` or null
computeTier3Key(contactId, subj, companyId): string | null // `sender_subject_client:...` or null
// Fallback key when no signal exists: `report:<reportId>`
campaigns table columns (migration 097): id, campaign_key, group_method, first_seen_at, last_seen_at, report_count, status.
Test mock discipline (lib/services/campaign-grouping-service.test.ts): the fake transaction client routes each query to staged rows by a DISTINGUISHING SQL SUBSTRING (e.g. `WHERE m.message_id = $1`, `BETWEEN $4::timestamptz`, `UPDATE campaigns`, `INSERT INTO campaigns`). Any NEW query added to the service MUST have a matching new branch in `makeClient()` or the test throws `Unstaged query in test mock`.
</interfaces>
</context>
<tasks>
<task type="auto" tdd="true">
<name>Task 1: Own-campaign revalidation guard before create-new (CR-03 — BLOCKING)</name>
<files>lib/services/campaign-grouping-service.ts, lib/services/campaign-grouping-service.test.ts</files>
<read_first>
- lib/services/campaign-grouping-service.ts lines 320-395 (the match branch at 324-346 and the create-new fall-through at 348-394) — this is exactly where the guard is inserted.
- lib/services/campaign-grouping-service.ts lines 93-119 (computeTier1Key/computeTier2Key/computeTier3Key — reused, not reimplemented).
- lib/services/campaign-grouping-service.test.ts lines 79-117 (makeClient SQL-substring router) and 337-362 (the 18-04 regression test that PASSED yet missed CR-03 — the new tests must cover the single-report path it did not stage).
- .planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md lines 127-141 (the CR-03 section — authoritative bug description).
</read_first>
<behavior>
- Test A (CR-03 core, single-report re-analyze, must FAIL before the fix): stage ownReport with campaign_id='campaign-own', an ownMessage with a message_id, tier1/tier2Candidates/tier3 all empty (no sibling exists), and a NEW staged query returning the report's own campaign row {campaign_key: 'message_id:<that-message-id>', group_method: 'message_id'}. Assert result === {campaignId:'campaign-own', groupMethod:'message_id', created:false}; assert ZERO `INSERT INTO campaigns`, ZERO `UPDATE campaigns`, ZERO `UPDATE reports SET campaign_id`.
- Test B (CR-03 Tier-3-only variant): stage ownReport with campaign_id='campaign-own', ownMessage empty, tier3 empty (no sibling), own campaign row {campaign_key:'sender_subject_client:5:invoice alert:10', group_method:'sender_subject_client'}. Assert existing campaign returned, created:false, zero mutating calls.
- Test C (no-regression, genuinely new report): stage ownReport with campaign_id=null, all tiers empty. Assert exactly one `INSERT INTO campaigns`, created:true, zero `UPDATE campaigns` — the own-campaign guard must NOT run when campaign_id is null.
- Test D (signal genuinely diverged): stage ownReport campaign_id='campaign-own', all tiers empty, own campaign row whose campaign_key matches NONE of the freshly-computed current keys (e.g. stored 'sender_subject_client:5:old subject:10' but current subject differs). Assert it falls through to create a new campaign (created:true) — the guard only reuses when the stored key still matches a current tier key. (The origin-decrement for this fall-through is added in Task 2's Test H, which reuses this same staged scenario — Task 1 only needs to prove the create-new fall-through itself still fires correctly here.)
</behavior>
<action>
Add the CR-03 revalidation guard in groupReportIntoCampaign. First refactor so the three key builders run ONCE before both the own-campaign check and the existing create-new block: compute tier1Key from ownMessage.message_id, tier2Key from ownAttachmentHashes/ownUrlDomains/normalizedSubject/ownSenderValue, tier3Key from ownReport fields, and the fallback `report:${reportId}` — reusing computeTier1Key/computeTier2Key/computeTier3Key. Build a `currentKeys` string array from the non-null values of [tier1Key, tier2Key, tier3Key, `report:${reportId}`].
Then, AFTER the existing `if (matchCampaignId && matchGroupMethod)` migration branch and BEFORE the create-new INSERT, insert a new guard that runs only when `matchCampaignId` is null (no sibling matched) AND `ownReport.campaign_id` is non-null: SELECT campaign_key, group_method FROM campaigns WHERE id = ownReport.campaign_id (parameterized, `$1`). If a row is returned AND its campaign_key is included in `currentKeys`, the report is still validly linked to its own campaign — return { campaignId: ownReport.campaign_id, groupMethod: (the stored group_method cast to GroupReportResult['groupMethod']), created: false } WITHOUT any INSERT/UPDATE. If no row is returned (campaign was deleted) or the stored campaign_key matches none of currentKeys (the report's signal genuinely diverged), fall through to the existing create-new logic unchanged.
Reuse the tierNKey variables already computed for the create-new block (do not compute them twice). Add a comment referencing CR-03 and why the guard sits outside the sibling-match branch. Do NOT weaken the existing `r.id != $x` self-exclusion on any tier query and do NOT change the sibling-match/migration branch in this task (CR-02 is Task 2).
In the test file, add a new branch to makeClient() that routes the new own-campaign SELECT (distinguishing substring `SELECT campaign_key` / `FROM campaigns` + `WHERE id = $1`) to a staged `ownCampaign` rows array on the MockRows interface. Add Tests A-D from the behavior block. Follow the existing SQL-substring routing and callsContaining() assertion style exactly.
TDD: write Tests A and B first, run vitest, confirm they FAIL (the current code creates a duplicate campaign / falls through to INSERT), then implement the guard, then confirm all tests green.
</action>
<verify>
<automated>npx vitest run lib/services/campaign-grouping-service.test.ts</automated>
<automated>npx tsc --noEmit --pretty</automated>
</verify>
<acceptance_criteria>
- New Tests A-D exist and pass; Tests A/B demonstrably fail against the pre-fix code (verified during RED).
- Single-report re-analyze returns the report's own existing campaign_id with created:false and issues zero INSERT/UPDATE calls.
- campaign_id=null path still creates exactly one campaign (no regression).
- Signal-diverged path still falls through to create.
- `npx tsc --noEmit --pretty` exits 0; full suite (existing 17 + new tests) green.
</acceptance_criteria>
<done>Single-report re-analyze returns the report's own existing campaign_id with created:false and zero mutating queries; no duplicate campaigns row is ever created for the same campaign_key; create-new and signal-diverged paths unregressed; full suite + tsc green.</done>
</task>
<task type="auto" tdd="true">
<name>Task 2: Decrement origin campaign report_count on cross-campaign migration AND on signal-diverged create-new (CR-02 + plan-checker-flagged divergence case) + D-08 upgrade regression</name>
<files>lib/services/campaign-grouping-service.ts, lib/services/campaign-grouping-service.test.ts</files>
<read_first>
- lib/services/campaign-grouping-service.ts lines 324-346 (the sibling-match/migration branch — where a report is re-linked from ownReport.campaign_id to a different matchCampaignId without decrementing the origin).
- .planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md lines 48-66 (CR-02 Escalation — the authoritative description and the intended D-08 upgrade flow it must not break).
- 18-CONTEXT.md D-08 (lines ~107-115) — the /analyze route intentionally allows upgrading a Tier-3-only grouping to a Tier-1 sibling match; that upgrade MUST keep working.
- Task 1's create-new fall-through (both "no own-campaign row found" and "stored campaign_key matches none of currentKeys" branches) — this is the SECOND place a report can be abandoned from its origin campaign, and it needs the same decrement added here.
</read_first>
<behavior>
- Test E (CR-02 migration decrements origin): stage ownReport with campaign_id='campaign-A', a sibling match via Tier 1 or Tier 3 resolving to a DIFFERENT 'campaign-B'. Assert: one `UPDATE campaigns` incrementing B (report_count+1) AND one `UPDATE campaigns` decrementing A (report_count-1); one `UPDATE reports SET campaign_id` to B; created:false; result.campaignId==='campaign-B'.
- Test F (D-08 upgrade preserved, no origin when none): stage ownReport with campaign_id=null (never grouped) and a sibling Tier 1 match to 'campaign-B'. Assert increment of B only, NO decrement UPDATE (there is no origin to decrement), report re-linked to B, created:false.
- Test G (same-campaign re-match still a pure no-op): the existing 18-04 scenario (ownReport.campaign_id === matchCampaignId) still returns created:false with ZERO UPDATE campaigns / INSERT / re-link — the CR-02 decrement must NOT fire when the match resolves back to the report's own campaign.
- Test H (signal-diverged create-new also decrements origin — closes the plan-checker warning): reuse Task 1's Test D staging (ownReport.campaign_id='campaign-A', all tiers empty, own campaign row whose campaign_key matches none of currentKeys). Assert: one `INSERT INTO campaigns` (new campaign, created:true) AND one `UPDATE campaigns` decrementing campaign-A (report_count-1); report re-linked to the new campaign id.
</behavior>
<action>
In the sibling-match/migration branch (the code path where `matchCampaignId && matchGroupMethod` is true and `matchCampaignId !== ownReport.campaign_id`): after incrementing matchCampaignId's report_count/last_seen_at and re-linking the report, add — only when `ownReport.campaign_id` is non-null — an `UPDATE campaigns SET report_count = report_count - 1, updated_at = NOW() WHERE id = $1` against `ownReport.campaign_id` to correct the origin's now-stale count. Keep the existing `matchCampaignId === ownReport.campaign_id` early-return no-op AHEAD of the increment/decrement so the same-campaign case still issues zero mutations.
ALSO apply the identical decrement in Task 1's create-new fall-through: both when no own-campaign row is found for `ownReport.campaign_id`, and when the stored `campaign_key` matches none of `currentKeys` (the signal-diverged case) — whenever `ownReport.campaign_id` is non-null and control is about to INSERT a brand-new campaign, decrement the origin first. This is the same "abandoning an origin campaign" situation as the migration branch, just landing on a new row instead of an existing sibling's; leaving it undecremented would reproduce the same class of staleness the CR-02 fix exists to close. Factor the decrement into one small shared helper/inline block if that reads cleanly, or duplicate the single UPDATE statement — either is fine, just don't skip it in either location.
Scope note (explicit): DECREMENT ONLY — do NOT delete the origin campaign even if its report_count reaches 0. Deleting rows introduces FK-cascade and detail-route risk that is out of scope for this gap fix; an empty (report_count=0) campaign is a strictly lesser problem than a stale count and can be filtered/cleaned in a later phase. Record this scoping in a code comment referencing CR-02.
The origin decrement UPDATE uses the SAME `UPDATE campaigns` SQL prefix as the increment/create paths, so the test router already matches it — Tests E/F/G/H distinguish by inspecting the SQL body (`report_count + 1` vs `report_count - 1`) and bound params, following the existing callsContaining() pattern. Add Tests E-H.
TDD: write Test E first, confirm it FAILS (no decrement today), implement the migration-branch decrement, confirm green. Then write Test H, confirm it FAILS (create-new fall-through still doesn't decrement), implement the create-new-path decrement, confirm green. Then confirm F and G pass (guard against over-firing).
</action>
<verify>
<automated>npx vitest run lib/services/campaign-grouping-service.test.ts</automated>
<automated>npx tsc --noEmit --pretty</automated>
</verify>
<acceptance_criteria>
- Cross-campaign migration decrements the origin campaign's report_count exactly once; the destination is incremented exactly once.
- Signal-diverged create-new ALSO decrements the origin campaign's report_count exactly once (Test H) — this closes the plan-checker's warning that Task 1 alone would leave this path stale.
- No decrement fires when there is no origin (campaign_id=null) or when the match resolves to the report's own campaign.
- D-08 sibling upgrade path (Tier-3-only → Tier-1 sibling) still re-links correctly.
- Full test suite green; `npx tsc --noEmit --pretty` exits 0.
</acceptance_criteria>
<done>Cross-campaign migration AND signal-diverged create-new both decrement the origin campaign's report_count exactly once; the decrement never fires with a null origin or a same-campaign re-match; D-08 upgrade preserved; suite + tsc green.</done>
</task>
<task type="checkpoint:human-verify" gate="blocking">
<name>Task 3: Live database verification of no-duplicate-on-reanalyze (mocked coverage was insufficient last time)</name>
<files>none (live verification only — no file changes)</files>
<read_first>
- .planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md lines 127-141 (CR-03 reproduction steps used against production — mirror them, then clean up).
</read_first>
<what-built>
Tasks 1-2 add an own-campaign revalidation guard (CR-03) and an origin-count decrement (CR-02) to `groupReportIntoCampaign`, covered by new MOCKED vitest regression tests. Mocked-only coverage is explicitly NOT sufficient to close this gap: the 18-04 mocked regression test passed while CR-03 shipped and reproduced live in production. This codebase has no real-Postgres integration harness, so a human must confirm the fix against an actual database.
</what-built>
<action>
This is a blocking human-verification checkpoint — no code changes. The executor pauses here and the human performs the live database verification described in how-to-verify, then reports the outcome via the resume signal. Do not auto-approve; the legitimacy of the fix depends on a real Postgres round-trip because mocked-only coverage missed this exact bug once.
</action>
<how-to-verify>
Run this worktree's code against a real database — prefer a local dev instance (`npm run dev`, port 3100) pointed at a dev/test Postgres. If only production is available, use a known throwaway phishing ticket and CLEAN UP afterward (delete the test campaign(s), the report, its message/indicator rows), exactly as the verifier did for ticket 627088.
1. Pick a phishing ticket that currently has NO campaign (or create/reset test data for one). Call `POST /api/phishing/tickets/{ticket_id}/analyze` with an admin session cookie. Note the returned campaignId (call it C1) and that `created:true`.
2. Immediately call the IDENTICAL `POST /api/phishing/tickets/{ticket_id}/analyze` again (same ticket, no new sibling report). Expected: response returns the SAME campaignId C1 with `created:false` — NOT a new campaign id, NOT created:true.
3. Query the database directly: `SELECT id, campaign_key, report_count FROM campaigns WHERE campaign_key = (SELECT campaign_key FROM campaigns WHERE id = '<C1>');` Expected: exactly ONE row (no duplicate campaign_key), report_count unchanged from step 1 (not incremented), and `reports.campaign_id` for the ticket still points at C1.
4. (D-08 spot-check, optional but recommended) If two tickets share a Message-ID, confirm that /analyze on the second one still links both into one campaign and the first campaign's count does not go stale.
5. Clean up any test rows created (campaigns, report, messages, indicators) so the database returns to its pre-test state.
Confirm: step 2 returned the same id with created:false, and step 3 shows exactly one campaigns row for that key with a stable report_count.
</how-to-verify>
<verify>
<human-check>A second consecutive /analyze on the same single-report ticket returns the same campaignId with created:false, and a direct DB query shows exactly one campaigns row for that campaign_key with an unchanged report_count.</human-check>
</verify>
<acceptance_criteria>
- A second consecutive /analyze on the same single-report ticket returns the same campaignId with created:false.
- Direct DB query shows exactly one campaigns row for that campaign_key (no duplicate) and an unchanged report_count.
- No orphaned/abandoned campaign with a stale report_count remains for the tested ticket.
- Any test data created is cleaned up.
</acceptance_criteria>
<done>Live re-analyze of a single-report ticket produced no duplicate campaign and a stable report_count, confirmed by a direct database query; human typed "approved".</done>
<resume-signal>Type "approved" once the live re-analyze produces no duplicate campaign and a stable report_count, or describe the observed divergence.</resume-signal>
</task>
</tasks>
<threat_model>
## Trust Boundaries
| Boundary | Description |
|----------|-------------|
| operator → POST /api/phishing/tickets/{id}/analyze | Authenticated (requirePermission('phishing','analyze')) request repeatedly triggers grouping; already gated upstream, unchanged this plan |
| service → campaigns/reports tables | groupReportIntoCampaign writes campaign rows inside a transaction; the defect under fix is data-integrity, not access |
## STRIDE Threat Register
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|-----------|----------|-----------|-------------|-----------------|
| T-18-01 | Denial of Service / data integrity | groupReportIntoCampaign create-new fall-through | mitigate | CR-03 fix: revalidate the report's own existing campaign before creating a new one, so repeat /analyze can no longer spawn unbounded duplicate campaigns rows sharing one campaign_key |
| T-18-02 | Tampering (integrity) | campaigns.report_count | mitigate | CR-02 fix: decrement the origin campaign on cross-campaign migration so report_count is not left stale/inflated |
| T-18-03 | Tampering | concurrent find-or-create without UNIQUE constraint on campaign_key | accept | Pre-existing (IN-03, carried forward); out of scope for this gap fix. Grouping already runs inside postgresClient.transaction(); a UNIQUE constraint + row-lock hardening is tracked separately, not introduced here |
| T-18-SC | Tampering | npm/pip/cargo installs | N/A | No package installs in this plan — pure edits to an existing service + test file |
</threat_model>
<verification>
- `npx vitest run lib/services/campaign-grouping-service.test.ts` — all existing tests plus new CR-03 (A-D) and CR-02 (E-H) regression tests pass; Tests A/B, E, and H demonstrably failed against pre-fix code during RED.
- `npx tsc --noEmit --pretty` exits 0.
- Live human verification (Task 3): a second consecutive /analyze on a single-report ticket returns the same campaignId with created:false, and a direct DB query shows exactly one campaigns row for that campaign_key with an unchanged report_count.
- Scope confirmation: WR-01 (Tier 2 missing ORDER BY), WR-04/WR-05, and IN-* findings from 18-REVIEW.md are NOT addressed here (out of scope for this gap-closure plan).
</verification>
<success_criteria>
- CR-03 closed: re-analyzing a single-report campaign reuses the report's own existing campaign (created:false) and never creates a duplicate campaigns row with the same campaign_key — proven by mocked regression tests AND a live database check.
- CR-02 closed: cross-campaign migration decrements the origin campaign's report_count, AND the signal-diverged create-new fall-through does the same (no abandoned origin is ever left stale, whether the report lands on an existing sibling's campaign or a brand-new one).
- D-08 preserved: a report with a genuinely better sibling match still upgrades to that sibling's campaign; the own-campaign guard runs only when no sibling matched.
- No regression: first-time grouping of a new report still creates exactly one campaign; the 18-04 same-campaign re-match no-op still fires with zero mutations.
</success_criteria>
<output>
Create `.planning/phases/18-campaign-grouping-phishing-analysis-api/18-05-SUMMARY.md` when done.
</output>