docs(18-04): complete gap-closure plan summary
- CAMP-02 report_count double-increment fix and WR-02 limit-clamp fix, with a passing regression test and clean type check
This commit is contained in:
parent
abe3d4b900
commit
21fde9eb3d
1 changed files with 106 additions and 0 deletions
|
|
@ -0,0 +1,106 @@
|
|||
---
|
||||
phase: 18-campaign-grouping-phishing-analysis-api
|
||||
plan: 04
|
||||
subsystem: api
|
||||
tags: [postgres, vitest, tdd, campaign-grouping, phishing]
|
||||
|
||||
# Dependency graph
|
||||
requires:
|
||||
- phase: 18-01
|
||||
provides: groupReportIntoCampaign tiered matching (Tier 1/2/3), campaigns find-or-create core
|
||||
- phase: 18-03
|
||||
provides: GET /api/phishing/campaigns list route
|
||||
provides:
|
||||
- Idempotent re-grouping — re-running /analyze on an already-grouped report no longer double-increments campaigns.report_count
|
||||
- Regression test proving the sibling-re-match no-op path
|
||||
- Clamped [0, 200] limit query param on GET /api/phishing/campaigns (explicit limit=0 honored, negative limit no longer 500s)
|
||||
affects: [19-classification, 20-remediation-approval-audit, 21-autotask-triage-note]
|
||||
|
||||
# Tech tracking
|
||||
tech-stack:
|
||||
added: []
|
||||
patterns:
|
||||
- "Own-row current-state guard before mutating an aggregate: fetch the row's own current FK (campaign_id) alongside its match candidates, and short-circuit as a no-op when a tiered/derived match resolves back to the row's own current state — prevents unbounded self-reinforcing increments under repeat invocation."
|
||||
- "Parse-then-clamp for untrusted numeric query params: parse once with Number.isFinite (not `|| default`, which swallows explicit 0), then Math.min(Math.max(parsed, floor), ceiling)."
|
||||
|
||||
key-files:
|
||||
created: []
|
||||
modified:
|
||||
- lib/services/campaign-grouping-service.ts
|
||||
- lib/services/campaign-grouping-service.test.ts
|
||||
- app/api/phishing/campaigns/route.ts
|
||||
|
||||
key-decisions:
|
||||
- "Guard placed inside the match branch (matchCampaignId === ownReport.campaign_id) rather than adding a new pre-check query — reuses the ownReport row already fetched, no extra round trip"
|
||||
- "Did not implement campaign-move / origin-count-decrement — out of scope per VERIFICATION.md, that code path doesn't exist today"
|
||||
|
||||
requirements-completed: [CAMP-02, CAMP-03]
|
||||
|
||||
duration: 2min
|
||||
completed: 2026-07-16
|
||||
---
|
||||
|
||||
# Phase 18 Plan 04: Campaign re-match idempotency + limit param clamp Summary
|
||||
|
||||
**Fixed unbounded `campaigns.report_count` double-increment on repeat `/analyze` calls by short-circuiting when a tiered match resolves to the report's own current campaign, plus clamped the `limit` query param on `GET /api/phishing/campaigns` to [0, 200].**
|
||||
|
||||
## Performance
|
||||
|
||||
- **Duration:** 2 min
|
||||
- **Started:** 2026-07-16T02:23:52Z
|
||||
- **Completed:** 2026-07-16T02:25:43Z
|
||||
- **Tasks:** 2 completed
|
||||
- **Files modified:** 3
|
||||
|
||||
## Accomplishments
|
||||
- Closed the CAMP-02 gap (CR-01 in 18-REVIEW.md): re-running `POST /api/phishing/tickets/{id}/analyze` on a report already linked to a multi-report campaign no longer inflates that campaign's `report_count`.
|
||||
- Added a regression test (`campaign-grouping-service.test.ts`) that fails without the fix and proves zero `UPDATE campaigns` / `INSERT INTO campaigns` / `UPDATE reports SET campaign_id` calls on a same-campaign sibling re-match.
|
||||
- Closed the WR-02 finding: `GET /api/phishing/campaigns?limit=-5` no longer raises an unhandled Postgres 500 ("LIMIT must not be negative"), and `limit=0` is now honored instead of silently replaced by the default of 50.
|
||||
- Verified no regression: all 16 pre-existing grouping tests plus the new test are green (17/17); `npx tsc --noEmit --pretty` exits 0.
|
||||
|
||||
## Task Commits
|
||||
|
||||
Each task was committed atomically (Task 1 followed RED → GREEN TDD flow):
|
||||
|
||||
1. **Task 1 (RED): add failing regression test** - `afd7af7` (test)
|
||||
2. **Task 1 (GREEN): short-circuit own-campaign re-match** - `9ca2ccf` (fix)
|
||||
3. **Task 2: clamp campaigns list limit param (WR-02)** - `abe3d4b` (fix)
|
||||
|
||||
**Plan metadata:** committed with this SUMMARY.md (see final commit)
|
||||
|
||||
## Files Created/Modified
|
||||
- `lib/services/campaign-grouping-service.ts` - `OwnReportRow` now includes `campaign_id: string | null`; the own-report SELECT fetches `campaign_id::text AS campaign_id` (appended after `created_at` to preserve the test-mock routing substring); the match branch short-circuits to a no-op (`created: false`, no UPDATEs) when `matchCampaignId === ownReport.campaign_id`.
|
||||
- `lib/services/campaign-grouping-service.test.ts` - New test: "re-analyzing a report already linked to a campaign that a sibling report re-matches does not increment report_count a second time" — stages `ownReport.campaign_id: 'campaign-1'` with a Tier 3 sibling match on the same campaign, asserts zero mutating calls.
|
||||
- `app/api/phishing/campaigns/route.ts` - `limit` parsing changed from `Math.min(parseInt(...) || 50, 200)` to parse-once-then-clamp: `Math.min(Math.max(Number.isFinite(rawLimit) ? rawLimit : 50, 0), 200)`. `offset`, `status` filter, auth gate, and response shape unchanged.
|
||||
|
||||
## Decisions Made
|
||||
- Reused the already-fetched `ownReport` row for the no-op guard instead of an extra query — the row is already in scope inside the transaction, so no additional round trip is needed.
|
||||
- Left the campaign-move / origin-decrement path unimplemented, per VERIFICATION.md's explicit scoping — that behavior doesn't exist anywhere in the codebase today and was called out as conditionally out of scope ("if ever supported").
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
None - plan executed exactly as written. Both tasks matched their specified `<action>` blocks precisely (including the exact SQL column-list append point and the `Number.isFinite` clamp shape).
|
||||
|
||||
## TDD Gate Compliance
|
||||
|
||||
Task 1 was `tdd="true"`. Gate sequence verified in git log:
|
||||
1. RED: `afd7af7 test(18-04): add failing regression test for campaign report_count double-increment` — test run confirmed failure before the fix (1 failed / 16 passed).
|
||||
2. GREEN: `9ca2ccf fix(18-04): short-circuit own-campaign re-match to stop report_count double-increment` — test run confirmed all 17 tests pass after the fix.
|
||||
3. No REFACTOR commit needed — implementation was minimal and required no cleanup pass.
|
||||
|
||||
Note: the GREEN commit uses `fix(...)` rather than `feat(...)` since this is a bug fix (per the project's commit-type convention: `fix` for bug corrections), not new feature addition. The RED→GREEN sequencing (test commit before implementation commit) is the gate requirement and is satisfied.
|
||||
|
||||
## Issues Encountered
|
||||
None.
|
||||
|
||||
## User Setup Required
|
||||
|
||||
None - no external service configuration required.
|
||||
|
||||
## Next Phase Readiness
|
||||
- CAMP-02 and CAMP-03 requirements are now fully closed; `campaigns.report_count` is accurate and idempotent under repeat `/analyze` calls, and the read API (`GET /api/phishing/campaigns`) safely handles malformed pagination params.
|
||||
- No blockers for Phase 19 (Classification), which consumes campaign data as an input.
|
||||
|
||||
---
|
||||
*Phase: 18-campaign-grouping-phishing-analysis-api*
|
||||
*Completed: 2026-07-16*
|
||||
Loading…
Add table
Add a link
Reference in a new issue