lorentz/wulf-pulse
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

docs(18): update code review after CR-03/CR-02 gap closure

Browse source
This commit is contained in:
lorentz 2026-07-16 06:56:03 -04:00
parent b59d9c2071
commit cd9dfc3954
1 changed files with 44 additions and 245 deletions
Download patch file Download diff file Expand all files Collapse all files

289
.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md
View file

@ -1,285 +1,84 @@
---
phase: 18-campaign-grouping-phishing-analysis-api
reviewed: 2026-07-16T02:32:12Z
reviewed: 2026-07-16T10:54:20Z
depth: standard
files_reviewed: 3
files_reviewed: 2
files_reviewed_list:
- lib/services/campaign-grouping-service.ts
- lib/services/campaign-grouping-service.test.ts
- app/api/phishing/campaigns/route.ts
findings:
critical: 2
critical: 0
warning: 2
info: 3
total: 7
total: 5
status: issues_found
---
# Phase 18: Code Review Report (re-review, post gap-closure fix 18-04)
# Phase 18: Code Review Report (re-review, supersedes prior 18-REVIEW.md for these 2 files)
**Reviewed:** 2026-07-16T02:32:12Z
**Reviewed:** 2026-07-16T10:54:20Z
**Depth:** standard
**Files Reviewed:** 3
**Status:** issues_found
**Files Reviewed:** 2
**Status:** issues_found (no Critical findings; both gap-closure fixes confirmed correct)
## Summary
This is a re-review of `campaign-grouping-service.ts`, its test file, and the
`/api/phishing/campaigns` route after plan 18-04's gap-closure fix. Both
originally-flagged issues are **verified fixed**:
Re-reviewed `campaign-grouping-service.ts` and its test file fresh, focused on the two gap-closure fixes from plan 18-05 (CR-03 own-campaign revalidation guard, CR-02 `decrementOriginCampaign` helper) and the shared tier-key-computation refactor that supports both. This review scope is 2 files (`campaign-grouping-service.ts`/`.test.ts`) — the prior `18-REVIEW.md`'s `app/api/phishing/campaigns/route.ts` findings (its own CR-01/CR-02) are out of scope here and not re-verified in this pass; this report replaces the prior document's content for the 2 files listed above only.
- **Prior CR-01 (double-increment on sibling re-match)** — fixed.
`groupReportIntoCampaign` now selects the report's own `campaign_id`
(`campaign-grouping-service.ts:158`) and compares it against
`matchCampaignId` (`:332-334`), short-circuiting with a no-op
`{ created: false }` result when a tier match resolves back to the
report's own current campaign. Verified against the new regression test at
`campaign-grouping-service.test.ts:337-362`, which asserts zero
`UPDATE campaigns`, zero `INSERT INTO campaigns`, and zero
`UPDATE reports` calls for exactly the sibling-re-match scenario the
original finding described.
- **Prior WR-02 (unclamped `limit` → Postgres 500 on negative input)** —
fixed. `route.ts:31-33` now clamps `limit` through `Number.isFinite` +
`Math.max(...,0)` + `Math.min(...,200)`, so a negative, NaN, or oversized
`limit` can no longer reach the SQL `LIMIT` clause unclamped.
**CR-03 (own-campaign revalidation guard, `campaign-grouping-service.ts:399-438`):** traced correctly. Every tier query self-excludes the report's own row, so a single-report campaign (no sibling exists yet) always reaches this block with `matchCampaignId === null`. The guard recomputes `currentKeys` (tier1/tier2/tier3 + a `report:<id>` fallback, always deterministic per report — filtered non-null at `:357-359`) and checks whether the report's *own* stored `campaign_key` is still one of them. If so, it returns the existing campaign with zero mutating queries. Verified against Test A (Tier-1 stored key reused), Test B (Tier-3 stored key reused), Test C (never-grouped, guard correctly doesn't fire), and Test D (signal diverged, correctly falls through to create-new). Hand-traced additional edge cases not in the suite — empty signal falling back to the `report:<id>` key on both sides (always self-consistent since it's derived from `reportId` alone), and a stale/diverged stored key always being excluded from `currentKeys` by construction (so the diverged-fallthrough path can never collide with the abandoned key) — both consistent, no hole found.
However, this pass surfaced **two new, material bugs** in the same code
paths:
**CR-02 (`decrementOriginCampaign`, `:122-138`):** correctly shared by both abandonment sites — the sibling-migration branch (`:393-395`) and the CR-03 signal-diverged fall-through (`:437`) — and correctly gated so it never fires on a same-campaign no-op match (the CR-01 guard at `:372-374` returns before either call site is reached) or on a never-grouped report (both call sites are guarded by `ownReport.campaign_id` being truthy). Verified against Test E (cross-campaign migration: decrement origin + increment destination, exactly one each), Test F (never-grouped upgrade: increment only, no decrement), Test G (same-campaign re-match: no mutation at all — confirms the guard interposes correctly ahead of the decrement), and Test H (signal-diverged create-new: decrement origin + insert new campaign, no increment). All four scenarios traced correctly through the code and match the described production confirmation (ticket 627088 analyzed twice, single `campaigns` row, unchanged `report_count`).
1. A parameter-binding bug in the campaigns list route (unrelated to the
`limit`/`offset` fix) that makes every status-filtered request 500.
2. A gap in the CR-01 fix itself: the added guard only covers "tier match
resolves to the report's own *current* campaign." It does not cover "tier
match resolves to a *different, already-existing* campaign" — which is
the exact Tier-3→Tier-1 upgrade scenario this phase's own docstring (D-08)
describes as expected, and which the **prior review's own suggested fix
explicitly warned about** ("If the intent is also to support a report
*moving* from one campaign to another, the old campaign's `report_count`
must be decremented at the same time — currently there is no code path
that does this at all"). That caveat was not addressed by the 18-04 fix;
the gap is still open today, just narrower than before.
## Critical Issues
### CR-01: `/api/phishing/campaigns?status=...` throws a Postgres parameter-binding error (500) on every status-filtered request
**File:** `app/api/phishing/campaigns/route.ts:36-56`
**Issue:** `statusFilter` is built once, against the parameter list used by
the *first* query (`params = [limit, offset, status]`, so the placeholder is
computed as `$${params.length}` = `$3`):
```ts
const params: unknown[] = [limit, offset];
let statusFilter = '';
if (status) {
params.push(status);
statusFilter = `WHERE status = $${params.length}`; // "WHERE status = $3"
}
```
That same `statusFilter` string (still referencing `$3`) is then reused
verbatim for the **total count** query, but with a completely different,
much shorter params array:
```ts
const totalRes = await postgresClient.query<{ count: string }>(
`SELECT COUNT(*)::text AS count FROM campaigns ${statusFilter}`,
status ? [status] : [] // only one bind value supplied, but the SQL text references $3
);
```
Postgres's extended query protocol requires every placeholder referenced in
the SQL text to have a bound value at that ordinal position. With `status`
set, the total-count query text contains `$3` but only one bind parameter is
supplied — `pg` throws a bind-mismatch error, which is caught by the route's
`catch` block and returned as a `500`. **Every** call to this endpoint with a
`status` filter (e.g. `?status=open`, exactly what a campaigns dashboard
would call by default) fails outright.
**Fix:** Give the total-count query its own independent placeholder numbering
instead of reusing `statusFilter`:
```ts
const totalRes = await postgresClient.query<{ count: string }>(
`SELECT COUNT(*)::text AS count FROM campaigns ${status ? 'WHERE status = $1' : ''}`,
status ? [status] : []
);
```
### CR-02: CR-01's same-campaign guard doesn't cover cross-campaign migration — old campaign's `report_count` is never decremented
**File:** `lib/services/campaign-grouping-service.ts:324-346`
**Issue:** The gap-closure fix added exactly one guard:
```ts
if (matchCampaignId === ownReport.campaign_id) {
return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false };
}
```
This correctly no-ops when a tier match resolves back to the report's own
*current* campaign (the originally-flagged scenario). But it does nothing
for the case where `ownReport.campaign_id` is non-null and `matchCampaignId`
resolves to a **different, already-existing** campaign. That case is not
hypothetical — it's exactly what the file's own docstring describes as
expected behavior:
> D-08: ... it may upgrade a Tier-3-only report to Tier 1 after
> `parseAndStoreMessage` has just populated `messages`/`indicators` for the
> first time.
Concretely: report X is created, Tier 3 matches nothing, and a new campaign
A is created for it (`report_count = 1`, `reports.campaign_id = A`). Later,
`/analyze` is re-run on X after `parseAndStoreMessage` has populated its
`messages`/`indicators` row. Tier 1 now finds a *different*, pre-existing
campaign B (formed by another report sharing the same `message_id`). Since
`B !== A`, the new guard doesn't trigger, and the code falls through to:
```ts
await client.query(
`UPDATE campaigns SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW() WHERE id = $1`,
[matchCampaignId] // increments B
);
await client.query(
`UPDATE reports SET campaign_id = $1, updated_at = NOW() WHERE id = $2`,
[matchCampaignId, reportId] // moves X from A to B
);
```
Campaign A's `report_count` is **never decremented**. A now has
`report_count = 1` with zero actually-linked reports — a permanently
stale/inflated count that the exact endpoint under review
(`GET /api/phishing/campaigns`) surfaces to users indefinitely, with no
reconciliation path anywhere in the codebase.
This is not a newly-introduced regression — the prior review's suggested fix
for the original bug explicitly flagged this exact risk ("a moved report
also leaves the origin campaign's count permanently inflated") — but the
18-04 fix implemented only the narrower same-campaign guard and left this
caveat unaddressed. It should be treated as still-open, not resolved.
**Fix:** Decrement the outgoing campaign when the report is moving to a
genuinely different campaign, not just skip when it's the same one:
```ts
if (matchCampaignId && matchGroupMethod) {
if (matchCampaignId === ownReport.campaign_id) {
return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false };
}
if (ownReport.campaign_id) {
await client.query(
`UPDATE campaigns
SET report_count = GREATEST(report_count - 1, 0), updated_at = NOW()
WHERE id = $1`,
[ownReport.campaign_id]
);
}
await client.query(
`UPDATE campaigns
SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW()
WHERE id = $1`,
[matchCampaignId]
);
await client.query(
`UPDATE reports SET campaign_id = $1, updated_at = NOW() WHERE id = $2`,
[matchCampaignId, reportId]
);
return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false };
}
```
(A campaign left at `report_count = 0` raises a separate follow-up question
— whether to soft-delete/hide it from the list endpoint — but that's a
smaller design decision than leaving the count permanently wrong.)
No Critical-severity defects were introduced by this round's changes. Two Warnings and three Info-level items below are worth addressing for robustness/precision but do not block shipping given the already-documented scope limits (D-04, Pitfall 2, and the CR-02 scope note in the code itself).
## Warnings
### WR-01: Tier 2 candidate query has no `ORDER BY` — non-deterministic campaign selection among multiple qualifying siblings
### WR-01: `decrementOriginCampaign` has no floor — `report_count` can go negative
**File:** `lib/services/campaign-grouping-service.ts:241-251`
**Issue:** Tier 1 (`campaign-grouping-service.ts:192-201`, `ORDER BY
r.created_at ASC LIMIT 1`) and Tier 3 (`:301-311`, `ORDER BY r.created_at
ASC`) both deterministically pick the earliest-created match. The Tier 2
candidate query has no `ORDER BY` at all:
```ts
const tier2Candidates = await client.query<Tier2CandidateRow>(
`SELECT r.id::text AS report_id, r.campaign_id::text AS campaign_id, r.title,
m.id::text AS message_id
FROM messages m
JOIN reports r ON r.id = m.report_id
WHERE r.campaign_id IS NOT NULL
AND r.id != $1
AND r.created_at BETWEEN $2::timestamptz - INTERVAL '24 hours'
AND $2::timestamptz + INTERVAL '24 hours'`,
[reportId, ownReport.created_at]
);
**File:** `lib/services/campaign-grouping-service.ts:131-138`
**Issue:** The decrement is unconditional (`report_count = report_count - 1`) with no `GREATEST(...)` clamp, and there is no DB-level `CHECK (report_count >= 0)` constraint (`migrations/097_phishing_triage_schema.sql:32` — plain `INTEGER NOT NULL DEFAULT 0`). Every path in this file that reaches `decrementOriginCampaign` is, by hand-trace, reachable at most once per abandonment under single-instance, non-concurrent execution — so this is safe today. But that guarantee rests entirely on there being no concurrent double-fire of the same grouping call (see WR-02). If a race — or a future caller — ever invokes this twice for the same origin campaign, the count silently goes negative with no defensive floor and nothing downstream would catch it.
**Fix:**
```sql
UPDATE campaigns
SET report_count = GREATEST(report_count - 1, 0), updated_at = NOW()
WHERE id = $1
```
If two distinct existing campaigns both have a sibling report matching by
attachment-hash/URL-domain + subject + sender within the 24h window (plausible
during a large phishing wave, where multiple independent campaigns can share
a lure template), which one gets attached is whatever order Postgres happens
to return rows in — not guaranteed stable across query plans/analyze cycles,
and inconsistent with Tier 1/Tier 3's explicit earliest-wins ordering.
**Fix:** Add `ORDER BY r.created_at ASC` to match the Tier 1/Tier 3
convention.
### WR-02: Pre-transaction `skipIfAlreadyGrouped` check is a TOCTOU race, and the new decrement logic raises its stakes
### WR-02: No regression test covers the same-campaign guard for Tier 1/Tier 2, nor the CR-02 cross-campaign-migration gap
**File:** `lib/services/campaign-grouping-service.test.ts:337-362`
**Issue:** The regression test added by 18-04
(`'re-analyzing a report already linked to a campaign that a sibling report
re-matches...'`) only exercises the same-campaign guard via a **Tier 3**
match. The guard is generic — applied once after all three tiers resolve —
so this is a coverage gap rather than proof the fix generalizes, and nothing
in the current suite would have caught CR-02 above.
**Fix:** Add two more cases: (1) a Tier 1 sibling match that resolves to the
report's own current campaign (should still no-op — currently unverified for
this tier), and (2) a case where `ownReport.campaign_id = 'campaign-A'` and
the tier match resolves to a *different* `'campaign-B'` — asserting the fix
for CR-02 (old campaign decremented, new campaign incremented, report moved)
once implemented, so this doesn't regress silently again.
**File:** `lib/services/campaign-grouping-service.ts:165-175`
**Issue:** The `skipIfAlreadyGrouped` short-circuit reads `reports.campaign_id` via a plain `postgresClient.query` *outside* the transaction, before `BEGIN` (transaction body starts at `:175`). If the webhook or cron sweep fires twice in quick succession for the same report (webhook redelivery, or overlapping cron sweep ticks — both real call sites: `webhook-service.ts:493`, `phishing-sweep-service.ts:91`), both invocations can observe `campaign_id IS NULL` and both proceed into their own transaction. Neither transaction takes a row lock on the `reports` row or the candidate `campaigns` row, so both can independently reach the same tier match. This race pre-dates this round (already flagged as a known limitation via the "Pitfall 2 — no UNIQUE constraint on `campaign_key`" comment), but the new `decrementOriginCampaign` logic makes a concrete double-fire strictly worse than before: previously a double-fire risked only a duplicate `campaigns` row (visible/annoying); now a double-fire on the migration or CR-03-diverged branch can double-decrement an origin campaign's count, silently under-reporting it (compounded by WR-01's missing floor).
**Fix:** Out of scope to fully close here (would need `SELECT ... FOR UPDATE` on the `reports` row inside the transaction, or an advisory lock keyed on `reportId`), but worth a follow-up given the new decrement path raises the impact of the existing race. At minimum, consider re-reading `reports.campaign_id` with `FOR UPDATE` as the first statement inside the transaction (`:176-181`) so a concurrent second call blocks until the first commits, instead of relying solely on the non-transactional outer pre-check.
## Info
### IN-01: `limit=0` or negative is silently clamped to `0` (empty page) rather than falling back to the documented default of 50
### IN-01: Unchecked cast from DB `TEXT` to the `groupMethod` union
**File:** `app/api/phishing/campaigns/route.ts:32`
**Issue:** `Math.min(Math.max(Number.isFinite(rawLimit) ? rawLimit : 50, 0), 200)`
correctly prevents the Postgres 500 that the prior WR-02 flagged, but a
request like `?limit=-5` now silently returns an empty `items` array with
`limit: 0` in the response rather than an error or the documented default.
Callers may interpret an empty page as "no campaigns" instead of "your
`limit` param was invalid." Not a crash, just a silent-success footgun.
**Fix:** Consider treating `<= 0` the same as "unparseable" and falling back
to `50`, or explicitly document that `limit=0` is a valid "return nothing"
request.
**File:** `lib/services/campaign-grouping-service.ts:414, 426`
**Issue:** `ownCampaignRes` is typed `{ campaign_key: string; group_method: string }`, and the return value casts it with `ownCampaign.group_method as GroupReportResult['groupMethod']` — no runtime check that the stored value is actually one of `'message_id' | 'attachment_or_url' | 'sender_subject_client'`. In practice the column is only ever written by this same function's own `INSERT` (`:460-465`) with a value drawn from the union, so it's safe today, but it's an unchecked type assertion rather than a validated narrowing — the kind of blind cast CLAUDE.md's "don't use `any` — use specific types" guidance is meant to discourage in spirit.
**Fix:**
```ts
const VALID_GROUP_METHODS = new Set(['message_id', 'attachment_or_url', 'sender_subject_client']);
function asGroupMethod(v: string): GroupReportResult['groupMethod'] {
if (!VALID_GROUP_METHODS.has(v)) throw new Error(`Unexpected group_method value: ${v}`);
return v as GroupReportResult['groupMethod'];
}
```
### IN-02: `status` query param is not validated against known campaign statuses
### IN-02: CR-02 comment describes an "own campaign row is gone" scenario that the current schema makes unreachable
**File:** `app/api/phishing/campaigns/route.ts:34`
**Issue:** `status` is read straight from the query string and used directly
in the parameterized `WHERE` clause (safe from injection) but never checked
against the actual `campaigns.status` enum/values. A typo (`?status=opne`)
silently returns zero rows with a `200` rather than a `400`. Consistent with
the project's "no Zod in route handlers unless it matters" convention, but
worth a one-line comment noting it's intentional.
**File:** `lib/services/campaign-grouping-service.ts:430-436`
**Issue:** The comment states "own campaign row is gone, or the report's signal has genuinely diverged" as two alternative reasons the guard's `if (ownCampaign && ...)` can fail. Checked against the schema: `reports.campaign_id` is `UUID REFERENCES campaigns(id)` with no `ON DELETE` clause (`migrations/097_phishing_triage_schema.sql:67`), and there is no `DELETE FROM campaigns` anywhere in the codebase today (confirmed via repo-wide grep) — so a referenced `campaigns` row cannot currently be deleted out from under a report, and `ownCampaignRes.rows[0]` will always be present whenever `ownReport.campaign_id` is non-null. The defensive `ownCampaign &&` check is harmless to keep (cheap insurance against a future admin/cleanup script), but the comment overstates a currently-live code path as an expected runtime scenario.
**Fix:** Tighten the comment to note the "row is gone" branch is currently unreachable defensive code (no cascade delete, nothing deletes `campaigns` rows yet) rather than implying there's a live repro path for it — avoids a future reader assuming this needs its own test/repro.
### IN-03: Documented concurrency gap (`campaign_key` has no UNIQUE constraint) remains unaddressed — pre-existing, not touched by this fix
### IN-03: Test suite doesn't cover the CR-01/CR-03 guards via a Tier 2 match, only Tier 1 and Tier 3
**File:** `lib/services/campaign-grouping-service.ts:125-126`
**Issue:** The file's own comment ("Pitfall 2 — `campaigns.campaign_key` has
no UNIQUE constraint") acknowledges that concurrent calls to
`groupReportIntoCampaign` (e.g. webhook fire + cron sweep landing at nearly
the same time for two reports that would both create/join the same new
campaign) can race and create duplicate campaign rows or double-apply
increments, since there's no row-level locking (`SELECT ... FOR UPDATE`) or
DB-level uniqueness backing the find-or-create. Unrelated to the CR-01/WR-02
fixes verified in this pass — listed here only to confirm it's still open,
not silently resolved.
**File:** `lib/services/campaign-grouping-service.test.ts:342-424`
**Issue:** Test G (CR-01/CAMP-02 same-campaign no-op guard) and Tests A/B (CR-03 own-campaign revalidation) are covered for the Tier-3 same-campaign case (Test G) and the Tier-1/Tier-3 stored-key cases (A/B) respectively, but no test exercises either guard when the match/stored key is the Tier-2 (`attachment_or_url`) key. Both guards are generic (plain string/reference equality independent of which tier produced the value), so this is a low-risk coverage gap rather than a suspected bug — the existing 12-test suite simply doesn't touch this one branch combination.
**Fix:** Optional: add a Tier-2 variant of Test G and Test A for completeness, reusing the Tier-2 indicator fixture shape already present in the "extracts URL-domain" test.
---
_Reviewed: 2026-07-16T02:32:12Z_
_Reviewed: 2026-07-16T10:54:20Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_