docs(15): add code review fix report

.planning/phases/15-data-model-detection-ticket-evidence/15-REVIEW-FIX.md

@@ -0,0 +1,53 @@
+---
+phase: 15-data-model-detection-ticket-evidence
+fixed_at: 2026-07-15T12:08:31Z
+review_path: .planning/phases/15-data-model-detection-ticket-evidence/15-REVIEW.md
+iteration: 1
+findings_in_scope: 3
+fixed: 3
+skipped: 0
+status: all_fixed
+---
+
+# Phase 15: Code Review Fix Report
+
+**Fixed at:** 2026-07-15T12:08:31Z
+**Source review:** .planning/phases/15-data-model-detection-ticket-evidence/15-REVIEW.md
+**Iteration:** 1
+
+**Summary:**
+- Findings in scope: 3 (CR-01, WR-01, WR-02 — critical_warning scope; IN-01/IN-02 left unfixed as out of scope)
+- Fixed: 3
+- Skipped: 0
+
+## Fixed Issues
+
+### CR-01: Webhook-triggered phishing detection never actually detects anything (`payload.entity` is always `undefined`)
+
+**Files modified:** `lib/services/webhook-service.ts`
+**Commit:** ecc34b4
+**Applied fix:** Rewrote `triggerPhishingDetection` to stop branching on `payload.entity` (which is never populated by the real Autotask webhook flow — confirmed by tracing `normalizeWebhookPayload` in `lib/types/webhook.ts`, which never assigns `.entity`). It now queries the `tickets` table in Postgres for the row by `payload.entityId`, which is guaranteed to exist and hold current `title`/`description` because `handleCreateOrUpdate()` runs earlier in the same `processWebhook` flow and has already upserted the ticket. Added a guard that logs a warning and returns early if the row isn't found yet, instead of silently falling through with null title/description. Verified via `npx tsc --noEmit -p tsconfig.json` (no new errors attributed to this file) and manual re-read of the affected function.
+
+### WR-01: `phishing-detector.ts` duplicates the Autotask client factory and drops its config validation
+
+**Files modified:** `lib/services/phishing-detector.ts`
+**Commit:** c875081
+**Applied fix:** Removed the hand-rolled module-level `_autotaskClient`/`getAutotaskClient()` singleton (no env-var validation) and replaced it with the shared `getAutotaskClient()` from `lib/services/autotask-factory.ts`, which throws a clear error when Autotask isn't configured. `gatherTicketEvidence`'s existing try/catch around `getAttachments()` still swallows the error to `attachments = []`, but the logged error now carries the factory's explicit "Missing Autotask API configuration" message instead of silently producing an empty-but-indistinguishable result. Verified via `npx tsc --noEmit -p tsconfig.json` (no new errors) and `npx vitest run lib/services/phishing-detector.test.ts` (17/17 passing).
+
+### WR-02: Evidence snapshot goes stale — the content-hash gate that (correctly) excludes status/assignee churn also silently skips re-gathering evidence
+
+**Files modified:** `lib/services/phishing-detector.ts`
+**Commit:** 7c63c5f
+**Applied fix:** Decoupled "should we rewrite content_hash / title / description / matched_patterns" from "should we refresh the evidence snapshot," per the reviewer's suggested approach. When `content_hash` is unchanged, `detectPhishingTicket` now still calls `gatherTicketEvidence` and issues a targeted `UPDATE reports SET evidence = $1::jsonb, updated_at = NOW() WHERE ticket_id = $2` before returning `skippedUnchanged: true` (now also returning the existing `reportId`). This means the sweep's periodic re-scan of still-open reports will keep the evidence snapshot (ticket notes, time entries, attachment metadata) current even when title/description text never changes, while still avoiding the more expensive full upsert/rewrite on pure status/assignee churn. Verified via `npx tsc --noEmit -p tsconfig.json` (no new errors) and `npx vitest run lib/services/phishing-detector.test.ts` (17/17 passing).
+
+**Note for reviewer:** This is a behavioral/logic change (not just a structural refactor) — the pure-function test suite (`matchesPhishingPatterns`, `computePhishingContentHash`) still passes, but there is no automated coverage of the orchestration path itself (this gap is called out separately in IN-02, left unfixed per scope). Recommend a human sanity-check of the new "unchanged content_hash still refreshes evidence" branch before this ships, per the verification_strategy's logic-bug handling — treat as **fixed: requires human verification**.
+
+## Skipped Issues
+
+None — all in-scope findings (CR-01, WR-01, WR-02) were fixed. IN-01 and IN-02 were explicitly out of scope for this fix pass (Info severity, `fix_scope: critical_warning`) and were left untouched.
+
+---
+
+_Fixed: 2026-07-15T12:08:31Z_
+_Fixer: Claude (gsd-code-fixer)_
+_Iteration: 1_