From cb9d5af784e0717d105b91ee97fc723ba09a3561 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 08:09:49 -0400 Subject: [PATCH] docs(15): add code review fix report --- .../15-REVIEW-FIX.md | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 .planning/phases/15-data-model-detection-ticket-evidence/15-REVIEW-FIX.md diff --git a/.planning/phases/15-data-model-detection-ticket-evidence/15-REVIEW-FIX.md b/.planning/phases/15-data-model-detection-ticket-evidence/15-REVIEW-FIX.md new file mode 100644 index 0000000..efea733 --- /dev/null +++ b/.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_