diff --git a/.planning/phases/23-classification-disposition-per-client-automation-gate/23-06-REVIEW.md b/.planning/phases/23-classification-disposition-per-client-automation-gate/23-06-REVIEW.md new file mode 100644 index 0000000..90b27af --- /dev/null +++ b/.planning/phases/23-classification-disposition-per-client-automation-gate/23-06-REVIEW.md @@ -0,0 +1,191 @@ +--- +phase: 23-classification-disposition-per-client-automation-gate +reviewed: 2026-07-16T23:30:00Z +depth: standard +files_reviewed: 3 +files_reviewed_list: + - lib/services/remediation-service.ts + - lib/services/remediation-service.test.ts + - lib/services/webhook-service.ts +findings: + critical: 2 + warning: 1 + info: 1 + total: 4 +status: issues_found +--- + +# Phase 23: Code Review Report (gap-closure plan 23-06) + +**Reviewed:** 2026-07-16T23:30:00Z +**Depth:** standard +**Files Reviewed:** 3 +**Status:** issues_found + +## Summary + +Scope: commits c79af9b and 13bf851 on top of `a918b72d`, adding `autoPostAcknowledgment()` to +`remediation-service.ts` and wiring it into `webhook-service.ts`'s `auto_report` gated stage, +replacing the previously-unguarded `generateAndPostAcknowledgment(campaignId)` call (CR-01 from +23-REVIEW.md). + +The core idempotency mechanism is sound: `autoPostAcknowledgment` takes a `SELECT ... FOR UPDATE` +lock on the `campaigns` row, checks for an existing `acknowledge_user` remediation_actions row +before inserting/posting, and correctly stops the specific repeat-webhook scenario described in +CR-01 (multiple reports joining the same campaign, each re-triggering `runGatedPhishingStages` +with the same `campaignId`). `npm test` (16/16) and `npx tsc --noEmit` both pass for the changed +files. + +However, tracing the new function against the rest of the (unchanged) remediation/audit/UI code +that it now has to coexist with surfaces two real defects: the audit payload it writes doesn't +match the shape the campaign-detail API depends on to compute `completedAt`, and the manual +approve→remediate path has no server-side guard against re-triggering the exact same +duplicate-note class of bug this plan set out to close. Both are provable by reading the +consuming code, not speculative. + +## Critical Issues + +### CR-01: `autoPostAcknowledgment`'s audit payload breaks `completedAt` derivation for the row it creates + +**File:** `lib/services/remediation-service.ts:361-369` +**Issue:** `remediateApprovedActions` (the pre-existing manual path, lines 197-205) writes its +`remediation_completed` audit event with payload `{ actionId: row.id, actionType: row.action_type }`. +`app/api/phishing/campaigns/[id]/route.ts:225-231` explicitly depends on that `actionId` field to +build `completedAtByActionId`, which is the *only* source of `RemediationActionSummary.completedAt` +(remediation_actions has no dedicated completion-timestamp column — see the comment at +`route.ts:221-224`). + +`autoPostAcknowledgment`'s new audit event payload is `{ actionType: 'acknowledge_user', auto: true }` +— no `actionId`. The INSERT it issues also doesn't `RETURNING id`, so the id isn't even available +to include. As a result, every auto-posted `acknowledge_user` row will always resolve to +`completedAt: null` in the campaign-detail API response. + +This isn't cosmetic: `components/phishing/action-area-card.tsx:411-421` uses +`completedAction.completedAt` to render the "resolved" tooltip ("Already remediated on +{date} by {actor}"). For every auto-acknowledged campaign, this will silently fall back to the +vague "Already remediated on an earlier date" instead of the real date — a concrete, reproducible +regression in a UI surface this same gap-closure plan explicitly claims to keep consistent (see the +docstring at `remediation-service.ts:328-334`, which asserts the inserted row is what "the Action +Area / Timeline UI already render from"). + +**Fix:** +```ts +await client.query( + `INSERT INTO remediation_actions (campaign_id, action_type, status, approved_by, approved_at) + VALUES ($1, 'acknowledge_user', 'completed', $2, NOW()) + RETURNING id::text AS id`, + [campaignId, actor] +); +// capture the returned id, e.g.: +const insertRes = await client.query<{ id: string }>( + `INSERT INTO remediation_actions (campaign_id, action_type, status, approved_by, approved_at) + VALUES ($1, 'acknowledge_user', 'completed', $2, NOW()) + RETURNING id::text AS id`, + [campaignId, actor] +); +await writeAuditEvent( + { + campaignId, + actor, + eventType: 'remediation_completed', + payload: { actionId: insertRes.rows[0].id, actionType: 'acknowledge_user', auto: true }, + }, + client +); +``` +Add a test asserting the audit payload includes `actionId` matching the inserted row's id. + +### CR-02: Manual approve/remediate path can still re-trigger a duplicate acknowledgment note after auto-post + +**File:** `lib/services/remediation-service.ts:92-147` (`approveRemediationActions`, unchanged by +this diff) and `app/api/phishing/campaigns/[id]/approve/route.ts:52-60` +**Issue:** `autoPostAcknowledgment`'s idempotency guard ("does a `remediation_actions` row with +`action_type='acknowledge_user'` already exist for this campaign?") lives *only* inside +`autoPostAcknowledgment`. It is not consulted by, and has no counterpart in, the pre-existing manual +path: + +- `campaign-classifier.ts`'s `mapVerdictToActions()` unconditionally returns + `['acknowledge_user']` for verdict `USER_AWARENESS` — every re-classification of an + already-auto-acknowledged campaign still recommends `acknowledge_user` again. +- `approveRemediationActions` only validates the requested action type against + `recommendedActions`; it never checks whether an `acknowledge_user` row already exists for the + campaign, so it will happily insert a second `approved` row. +- `remediateApprovedActions` (lines 217-233) then unconditionally calls + `generateAndPostAcknowledgment(campaignId)` for any newly-transitioned `acknowledge_user` row — + it has no awareness that the auto path already posted once. +- `POST /api/phishing/campaigns/[id]/approve` (`route.ts:52-60`) only checks that the campaign + exists — no check for existing completed remediation of the same action type. + +The only thing that currently prevents this in practice is a **client-side** gate: +`action-area-card.tsx:411-412` computes `resolved = ... || completedAction != null` and disables +the Approve/Remediate buttons once any `remediation_actions` row is `status='completed'`. That's +UI-only — a direct API call (stale client tab, script, or race between the auto-post committing +and a manual approval already in flight) reaches `approveRemediationActions` → `remediateApprovedActions` +with zero server-side defense, reintroducing exactly the "duplicate customer-visible note" bug +this plan (23-06) was chartered to close, just via the manual path instead of the repeat-webhook +path. +**Fix:** Add a server-side guard, e.g. in `approveRemediationActions` (or as a DB constraint): +```ts +// inside approveRemediationActions, before the per-action insert loop: +const existingAckRes = await client.query( + `SELECT id FROM remediation_actions WHERE campaign_id = $1 AND action_type = 'acknowledge_user' LIMIT 1`, + [campaignId] +); +const alreadyAcknowledged = existingAckRes.rows.length > 0; +for (const action of actions) { + if (action.actionType === 'acknowledge_user' && alreadyAcknowledged) { + throw new RemediationValidationError('acknowledge_user has already been posted for this campaign'); + } + ... +} +``` +Alternatively (defense-in-depth, catches both paths at once): a partial unique index +`CREATE UNIQUE INDEX ... ON remediation_actions (campaign_id) WHERE action_type = 'acknowledge_user'` +and translate the resulting unique-violation into `RemediationConflictError` in both +`approveRemediationActions` and `autoPostAcknowledgment`. + +## Warnings + +### WR-01: `autoPostAcknowledgment` has no explicit "campaign not found" guard, unlike its sibling functions + +**File:** `lib/services/remediation-service.ts:344-345` +**Issue:** `markCampaignFalsePositive` (lines 280-287) explicitly throws +`RemediationValidationError('Campaign not found')` when the campaign row doesn't exist. +`autoPostAcknowledgment`'s `SELECT id FROM campaigns WHERE id = $1 FOR UPDATE` silently returns zero +rows for a bad/stale `campaignId` and the function proceeds anyway — the subsequent `INSERT INTO +remediation_actions` would fail via the `campaign_id REFERENCES campaigns(id)` FK constraint, +throwing an opaque Postgres FK-violation error instead of a clear, typed `RemediationValidationError`. +In practice this is caught by `webhook-service.ts`'s surrounding try/catch (`'[WEBHOOK] auto_report +stage error'`) so it doesn't crash the request, but it's an inconsistency with the file's own +established pattern and makes the failure mode harder to diagnose from logs alone. +**Fix:** Mirror the existing pattern: +```ts +const campaignRes = await client.query<{ id: string }>( + `SELECT id FROM campaigns WHERE id = $1 FOR UPDATE`, + [campaignId] +); +if (campaignRes.rows.length === 0) { + throw new RemediationValidationError('Campaign not found'); +} +``` + +## Info + +### IN-01: `AutoPostAcknowledgmentResult.posted` can be `true` even when the note post itself failed + +**File:** `lib/services/remediation-service.ts:340-383` +**Issue:** `posted: true` is returned whenever the DB insert/audit succeeded, regardless of whether +the subsequent `generateAndPostAcknowledgment` call actually succeeded (its failure is caught and +only logged, per the D-04 carve-out — this mirrors `remediateApprovedActions`'s existing behavior, +so it's intentional design, not a new bug). The current sole caller (`webhook-service.ts:561`) +discards the return value, so there's no live impact today, but the field name is misleading for +any future caller that trusts `posted: true` to mean "the customer note was actually delivered." +**Fix:** Consider renaming to something like `acknowledged` (state-transition succeeded) and +tracking actual note-delivery success separately, or document the distinction inline at the +interface definition. + +--- + +_Reviewed: 2026-07-16T23:30:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_