docs(23-06): add code review report

.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_