docs(18-05): add partial SUMMARY for Tasks 1-2 (Task 3 checkpoint pending)
Documents the CR-03 own-campaign revalidation guard and CR-02 origin decrement fix (Tasks 1-2, committed and green), ahead of the blocking Task 3 human live-database-verification checkpoint.
This commit is contained in:
parent
6945591f76
commit
4a34ba8ed2
1 changed files with 120 additions and 0 deletions
|
|
@ -0,0 +1,120 @@
|
|||
---
|
||||
phase: 18-campaign-grouping-phishing-analysis-api
|
||||
plan: 05
|
||||
subsystem: api
|
||||
tags: [postgres, vitest, campaign-grouping, phishing, idempotency, gap-closure]
|
||||
|
||||
# Dependency graph
|
||||
requires:
|
||||
- phase: 18-01
|
||||
provides: groupReportIntoCampaign tiered matching (CAMP-01/CAMP-02) and campaigns schema (migration 097)
|
||||
- phase: 18-04
|
||||
provides: reports.campaign_id on OwnReportRow and the same-campaign no-op guard this plan extends
|
||||
provides:
|
||||
- "CR-03 fix: own-campaign revalidation guard in groupReportIntoCampaign — re-analyzing a single-report campaign reuses the report's own existing campaign instead of creating a duplicate campaigns row with the same campaign_key"
|
||||
- "CR-02 fix: decrementOriginCampaign() helper, called on cross-campaign migration AND on the signal-diverged create-new fall-through, so an abandoned origin campaign's report_count is never left stale"
|
||||
- "Mocked regression tests (Tests A-H) reproducing both CR-03 and CR-02 exactly, with RED confirmed for Tests A, B, E, H against pre-fix code"
|
||||
affects: [19-classification, 20-remediation-approval-audit]
|
||||
|
||||
# Tech tracking
|
||||
tech-stack:
|
||||
added: []
|
||||
patterns:
|
||||
- "Own-signal tier keys (tier1Key/tier2Key/tier3Key/currentKeys) computed once, shared between the own-campaign revalidation guard and the create-new fall-through"
|
||||
- "Shared decrementOriginCampaign(client, campaignId) helper called from both abandonment sites (sibling-migration branch and signal-diverged create-new) to avoid duplicating the UPDATE statement"
|
||||
|
||||
key-files:
|
||||
created: []
|
||||
modified:
|
||||
- lib/services/campaign-grouping-service.ts
|
||||
- lib/services/campaign-grouping-service.test.ts
|
||||
|
||||
key-decisions:
|
||||
- "Own-campaign guard runs ONLY when matchCampaignId is null (no sibling matched) AND ownReport.campaign_id is non-null — placed after the sibling-match branch, before create-new, so it never suppresses a legitimate D-08 sibling upgrade"
|
||||
- "CR-02 decrement is scope-limited to decrement-only — never deletes an origin campaign even if report_count reaches 0 (FK-cascade/detail-route risk deferred to a later phase per plan's explicit scope note)"
|
||||
- "CR-02 decrement applied in BOTH abandonment sites (existing sibling-migration branch AND Task 1's signal-diverged create-new fall-through) per plan-checker guidance — not just the migration branch"
|
||||
|
||||
requirements-completed: [CAMP-01, CAMP-02]
|
||||
|
||||
# Metrics
|
||||
duration: n/a (partial — Task 3 checkpoint pending)
|
||||
completed: null
|
||||
---
|
||||
|
||||
# Phase 18 Plan 05: Campaign-grouping gap closure (CR-03 + CR-02) Summary — PARTIAL (Tasks 1-2 complete, Task 3 checkpoint pending)
|
||||
|
||||
**Own-campaign revalidation guard closes CR-03 (duplicate campaigns on single-report re-analyze); shared decrementOriginCampaign() helper closes CR-02 (stale report_count on migration and signal-diverged create-new) — both proven by 8 new mocked vitest regression tests with confirmed RED on Tests A, B, E, H.**
|
||||
|
||||
**STATUS: Tasks 1 and 2 are complete and committed. Task 3 (BLOCKING human-verify checkpoint — live database verification) has NOT yet run. This summary will be superseded/finalized once Task 3 resolves.**
|
||||
|
||||
## Performance
|
||||
|
||||
- **Tasks completed:** 2 of 3 (Task 3 is the blocking checkpoint, pending)
|
||||
- **Files modified:** 2 (`lib/services/campaign-grouping-service.ts`, `lib/services/campaign-grouping-service.test.ts`)
|
||||
|
||||
## Accomplishments
|
||||
|
||||
- **CR-03 closed (code + mocked tests):** re-analyzing a single-report campaign (no sibling report exists yet) now reuses the report's own existing `campaign_id` with `created:false` — zero `INSERT`/`UPDATE` queries issued. Root cause was every tier query's required self-exclusion (`r.id != $x`) meaning a lone report could never match itself, so `matchCampaignId` stayed null and control fell straight into "create new campaign" on every repeat `/analyze` call.
|
||||
- **CR-02 closed (code + mocked tests):** cross-campaign migration (report moves from campaign A to different existing campaign B) now decrements A's `report_count` in addition to incrementing B's. The identical decrement is also applied when the signal-diverged create-new fall-through abandons an origin campaign in favor of a brand-new one (the plan-checker-flagged second abandonment site).
|
||||
- **D-08 preserved:** a report with a genuinely better sibling match still upgrades to that sibling's campaign; the own-campaign guard only runs when no sibling matched, and the CR-02 decrement correctly does not fire when there is no origin campaign (never-grouped report).
|
||||
- **No regression:** first-time grouping of a genuinely new report (`campaign_id = null`) still creates exactly one campaign; the existing 18-04 same-campaign no-op test (renamed here to "Test G") still fires zero mutations.
|
||||
|
||||
## Task Commits
|
||||
|
||||
Each task was committed atomically:
|
||||
|
||||
1. **Task 1: Own-campaign revalidation guard before create-new (CR-03)** — `633b48c` (fix)
|
||||
2. **Task 2: Decrement origin campaign report_count on migration AND signal-diverged create-new (CR-02)** — `6945591` (fix)
|
||||
3. **Task 3: Live database verification of no-duplicate-on-reanalyze** — NOT YET RUN (blocking human-verify checkpoint)
|
||||
|
||||
_Both Task 1 and Task 2 followed TDD RED-before-GREEN discipline: RED confirmed via `npx vitest run` for Tests A/B (Task 1) and Tests E/H (Task 2) against the pre-fix code, then GREEN confirmed after implementing the fix._
|
||||
|
||||
## Files Created/Modified
|
||||
|
||||
- `lib/services/campaign-grouping-service.ts` — added `decrementOriginCampaign()` helper; hoisted `tier1Key`/`tier2Key`/`tier3Key`/`currentKeys` computation above the find-or-create block; added the CR-03 own-campaign revalidation guard between the sibling-match branch and the create-new fall-through; added the CR-02 decrement call in both the sibling-migration branch and the signal-diverged create-new fall-through
|
||||
- `lib/services/campaign-grouping-service.test.ts` — added `ownCampaign` to `MockRows` + a new SQL-substring router branch (`SELECT campaign_key`); added Tests A-D (CR-03) and Tests E-H (CR-02, with the pre-existing 18-04 same-campaign no-op test relabeled as "Test G")
|
||||
|
||||
## Decisions Made
|
||||
|
||||
- Own-campaign guard placed **outside** (after) the sibling-match branch, not merged into it — the plan required this to guarantee it never suppresses a legitimate D-08 sibling upgrade, since the guard must only run when `matchCampaignId` is null.
|
||||
- Factored the CR-02 decrement into a single shared `decrementOriginCampaign(client, campaignId)` helper rather than duplicating the raw `UPDATE campaigns SET report_count = report_count - 1 ...` statement at both call sites — reduces duplication while keeping each call site's guard condition (`if (ownReport.campaign_id)`) explicit and readable at the call site.
|
||||
- Test G (the pre-existing 18-04 regression test) was renamed/annotated in place rather than duplicated, since its assertions already fully satisfy the plan's Test G requirement (same-campaign re-match remains a zero-mutation no-op).
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
**1. [Rule 1 - Bug, self-caught during GREEN] Missing null-guard on the CR-02 decrement in the migration branch**
|
||||
|
||||
- **Found during:** Task 2, first GREEN attempt
|
||||
- **Issue:** The initial implementation called `decrementOriginCampaign(client, ownReport.campaign_id)` unconditionally after the increment/re-link in the sibling-migration branch, without checking `ownReport.campaign_id` was non-null first. This broke the existing "Tier 3 match increments..." regression test and the new Test F (D-08 upgrade with no prior campaign) — both scenarios have `ownReport.campaign_id` as `undefined`/`null` (a never-grouped report), and the unconditional call issued a spurious second `UPDATE campaigns` with `campaignId = undefined` as the bound parameter.
|
||||
- **Fix:** Wrapped the call in `if (ownReport.campaign_id) { await decrementOriginCampaign(...); }`, matching the plan's explicit requirement that the decrement only fires "when there is no origin (campaign_id=null) or when the match resolves to the report's own campaign" → no decrement.
|
||||
- **Files modified:** `lib/services/campaign-grouping-service.ts`
|
||||
- **Verification:** Re-ran `npx vitest run lib/services/campaign-grouping-service.test.ts` — all 24 tests passed (previously 2 failing: the pre-existing Tier-3-match test and new Test F).
|
||||
- **Committed in:** `6945591` (part of Task 2 commit — caught and fixed before the commit was made, not a separate follow-up commit)
|
||||
|
||||
---
|
||||
|
||||
**Total deviations:** 1 auto-fixed (Rule 1 — bug caught during the plan's own TDD GREEN verification step, not a deviation from the plan's design)
|
||||
**Impact on plan:** No scope creep — this was a bug in my own first-draft implementation of the plan's specified behavior, caught by the plan's own Test F before committing. Zero impact on the plan's intended design.
|
||||
|
||||
## Issues Encountered
|
||||
|
||||
None beyond the self-caught deviation above. `npx tsc --noEmit --pretty` exits 0 after both tasks; `npx vitest run lib/services/campaign-grouping-service.test.ts` is green with all 24 tests (16 pre-existing + Tests A-H new).
|
||||
|
||||
## User Setup Required
|
||||
|
||||
None — no external service configuration required. Task 3 requires a human to run a live verification against a real Postgres database (dev instance or a cleaned-up production throwaway ticket); this is not "setup" but the plan's mandatory human-verification checkpoint, detailed in `18-05-PLAN.md` Task 3's `<how-to-verify>` block.
|
||||
|
||||
## Next Phase Readiness
|
||||
|
||||
**NOT READY — Task 3 (blocking human-verify checkpoint) has not run.** Per the plan's `coverage_caveat`: mocked-only unit coverage was proven insufficient last time (the 18-04 mocked regression test passed while CR-03 shipped and reproduced live in production), so this gap-closure plan is explicitly not considered closed until a human confirms, against a real database:
|
||||
|
||||
1. A second consecutive `/analyze` on the same single-report ticket returns the same `campaignId` with `created:false` (not a new campaign, not `created:true`).
|
||||
2. A direct DB query shows exactly one `campaigns` row for that `campaign_key` with an unchanged `report_count`.
|
||||
3. Any test data created during verification is cleaned up.
|
||||
|
||||
This worktree's code (commits `633b48c` and `6945591`) is ready for that live verification. No further code changes are anticipated unless the live verification surfaces a divergence from the mocked tests' assumptions.
|
||||
|
||||
---
|
||||
*Phase: 18-campaign-grouping-phishing-analysis-api*
|
||||
*Plan: 05*
|
||||
*Status: PARTIAL — Tasks 1-2 complete, Task 3 checkpoint pending human live-verification*
|
||||
Loading…
Add table
Add a link
Reference in a new issue