diff --git a/.planning/phases/15-data-model-detection-ticket-evidence/15-VERIFICATION.md b/.planning/phases/15-data-model-detection-ticket-evidence/15-VERIFICATION.md new file mode 100644 index 0000000..c789fe1 --- /dev/null +++ b/.planning/phases/15-data-model-detection-ticket-evidence/15-VERIFICATION.md @@ -0,0 +1,160 @@ +--- +phase: 15-data-model-detection-ticket-evidence +verified: 2026-07-15T12:20:40Z +status: passed +score: 4/4 must-haves verified +overrides_applied: 0 +--- + +# Phase 15: Data Model, Detection & Ticket Evidence Verification Report + +**Phase Goal:** The durable phishing-triage schema exists in Postgres, and Pulse can scan +Autotask/Pulse tickets for known phishing/spam-report patterns idempotently, capturing base +ticket-level evidence for each candidate. +**Verified:** 2026-07-15T12:20:40Z +**Status:** passed +**Re-verification:** No — initial verification + +## Goal Achievement + +### Observable Truths (ROADMAP Success Criteria) + +| # | Truth | Status | Evidence | +|---|-------|--------|----------| +| 1 | Migration `097_*.sql` creates `campaigns`, `reports`, `messages`, `indicators`, `classifications`, `remediation_actions`, `audit_events` tables with `IF NOT EXISTS`, ready for later phases | VERIFIED | `migrations/097_phishing_triage_schema.sql` contains exactly 7 `CREATE TABLE IF NOT EXISTS` statements, no DROP/TRUNCATE. **Confirmed applied to live dev Postgres** — `docker exec pulse-postgres psql` returned `7` for `information_schema.tables` count on all 7 names. | +| 2 | Running the ticket scanner flags candidates matching the 8 known patterns and persists a `reports` row per candidate | VERIFIED | `matchesPhishingPatterns` in `lib/services/phishing-detector.ts:42-51` implements case-insensitive substring match over `KNOWN_PHISHING_PATTERNS` (exactly 8 entries, verbatim match to DETECT-01 list). `detectPhishingTicket` (lines 176-249) upserts one `reports` row via `INSERT ... ON CONFLICT (ticket_id) DO UPDATE`. 17/17 unit tests pass (`npx vitest run lib/services/phishing-detector.test.ts`), covering all 8 patterns individually plus negative/case-insensitive cases. | +| 3 | Re-scanning unchanged tickets does not reprocess/duplicate `reports` rows; a ticket whose data changed IS reprocessed | VERIFIED | `content_hash` (sha256 over title+description only, `computePhishingContentHash` lines 58-65) is compared against the stored value before any write (lines 187-206). Unchanged hash → skip full rewrite (only evidence refreshed, see below). Changed hash → full upsert via `ON CONFLICT (ticket_id)` (DB-enforced by `uq_reports_ticket_id` — confirmed present via `pg_constraint` query against live DB: `uq_reports_ticket_id\|u`). Hash tests (stability, title-change, description-change, null-normalization) pass. | +| 4 | Each flagged ticket's evidence includes ticket ID/number, company, requester/reporter, title, description, notes, time entries, attachment metadata (EVID-01) | VERIFIED | `gatherTicketEvidence` (lines 117-163) queries `companies.company_name`, `ticket_notes`, `time_entries`, and Autotask attachment metadata (fullPath/title/contentType, no base64 `data`). `reports` row also carries `ticket_id`, `ticket_number`, `requester_contact_id` (from `ticket.contact_id`), `created_by_contact_id`, `title`, `description` directly. Confirmed live DB `reports` table has all these columns. | + +**Score:** 4/4 truths verified + +### Required Artifacts + +| Artifact | Expected | Status | Details | +|----------|----------|--------|---------| +| `migrations/097_phishing_triage_schema.sql` | 7-table schema, `reports` fully designed | VERIFIED | Exists, 7 `CREATE TABLE IF NOT EXISTS`, `reports` has `ticket_id BIGINT NOT NULL REFERENCES tickets(id)`, `content_hash TEXT NOT NULL`, `matched_patterns JSONB`, `evidence JSONB`, `CONSTRAINT uq_reports_ticket_id UNIQUE (ticket_id)`. Applied to live dev DB (verified via docker exec). | +| `migrations/098_phishing_sweep_schedule.sql` | phishing-sweep `sync_schedules` seed row | VERIFIED | Exists, `INSERT ... ON CONFLICT (id) DO NOTHING`. Applied to live dev DB — `SELECT * FROM sync_schedules WHERE id='phishing-sweep'` returns `phishing-sweep\|phishing-sweep\|f\|0 5 * * *`. | +| `lib/services/phishing-detector.ts` | `KNOWN_PHISHING_PATTERNS`, `matchesPhishingPatterns`, `computePhishingContentHash`, `gatherTicketEvidence`, `detectPhishingTicket` | VERIFIED | All 5 exports present (249 lines), wired, tested. | +| `lib/services/phishing-detector.test.ts` | Unit tests for matcher + hash, all 8 patterns + negative + stability | VERIFIED | 17 tests, all passing, one assertion per locked pattern by name plus negative/case-insensitive/hash cases. | +| `lib/services/phishing-sweep-service.ts` | `sweepPhishingTickets()` bounded reconciliation loop | VERIFIED | 102 lines. Bounded `LIMIT $1` (500), `is_deleted = false` filter, 7-day window, per-row try/catch (no rethrow), calls shared `detectPhishingTicket` only — no duplicated logic. | +| `lib/services/webhook-service.ts` (modified) | Fire-and-forget phishing trigger on ticket CREATE | VERIFIED (post-fix) | `triggerPhishingDetection` reads the ticket back from Postgres (not `payload.entity`) after `handleCreateOrUpdate()` has upserted it in the same synchronous flow. See Fix Verification below. | +| `lib/services/sync-scheduler.ts` (modified) | `phishing-sweep` sync_type, default schedule, dispatch branch | VERIFIED | Union includes `'phishing-sweep'` (line 25), `defaultSchedules` entry with `is_enabled: false` (lines 302-308), dispatch branch dynamically imports and calls `sweepPhishingTickets`, logs scanned/flagged/skipped/errors summary (lines 472-477). | + +### Key Link Verification + +| From | To | Via | Status | Details | +|------|-----|-----|--------|---------| +| `reports.ticket_id` | `tickets.id` | foreign key | VERIFIED | `reports_ticket_id_fkey` present in live DB `pg_constraint`. | +| `reports.content_hash` | phishing-detector idempotency | unique key on ticket_id + stored hash | VERIFIED | `uq_reports_ticket_id` unique constraint present in live DB; hash comparison gate in `detectPhishingTicket`. | +| webhook `ticket.created` handler | `detectPhishingTicket` | fire-and-forget `.catch()` | VERIFIED | `webhook-service.ts:118-120`, not awaited in the request path. | +| `sync-scheduler.ts` dispatch | `sweepPhishingTickets` | dynamic `await import()` on `sync_type === 'phishing-sweep'` | VERIFIED | `sync-scheduler.ts:472-477`. | + +### Fix Verification (Critical Bug + Follow-up Warnings) + +The phase 15 code review (`15-REVIEW.md`) found one critical bug (webhook detection never fired +against real data) and two warnings, fixed in commits `ecc34b4`, `c875081`, `7c63c5f`. A +subsequent re-review (`15-REVIEW.md`, re-review section) found one more warning fixed in +`9c4584d`. All four were independently re-verified against the current code (not just SUMMARY +claims): + +| Fix | Commit | Verified? | Evidence | +|-----|--------|-----------|----------| +| CR-01: webhook detection reads real ticket data, not unpopulated `payload.entity` | ecc34b4 | VERIFIED | `triggerPhishingDetection` (webhook-service.ts:457-490) queries `SELECT ... FROM tickets WHERE id = $1` using `payload.entityId`, not `payload.entity`. Guard: if row not found, `console.warn` + early `return` (no throw, no crash). Ordering confirmed: `handleCreateOrUpdate()` is `await`ed at line 95, `triggerPhishingDetection` fires at line 118 — strictly after the ticket upsert on the happy path. | +| WR-01: shared `getAutotaskClient` factory replaces private duplicate | c875081 | VERIFIED | `phishing-detector.ts:17` imports `getAutotaskClient` from `./autotask-factory`; no private `_autotaskClient`/duplicate singleton remains in the file (grep confirms only the import + one call site). `autotask-factory.ts:16-20` throws a clear config-validation error when Autotask env vars are missing; that error is caught by the existing try/catch in `gatherTicketEvidence` (lines 145-155), degrading to `attachments: []`. | +| WR-02: evidence snapshot refreshed even when content_hash unchanged | 7c63c5f | VERIFIED | `phishing-detector.ts:192-206` — on hash match, still calls `gatherTicketEvidence` and issues a targeted `UPDATE reports SET evidence = $1::jsonb, updated_at = NOW() WHERE ticket_id = $2`, leaving `title`/`description`/`matched_patterns`/`content_hash` untouched. Confirmed the webhook path only fires on CREATE (line 114), so status/assignee churn on an already-detected ticket doesn't reach this branch via webhook; the sweep is the repeated caller and is itself idempotent/hash-gated. | +| WR-01 (new, post-re-review): soft-deleted notes/time entries excluded from evidence | 9c4584d | VERIFIED | `gatherTicketEvidence` (phishing-detector.ts:126-142) now filters both the `ticket_notes` and `time_entries` queries with `AND is_deleted = false`. Confirmed both columns actually exist on the live DB (`information_schema.columns` query for both tables returned `is_deleted`). | + +No regressions introduced by any of the four fixes — `npx tsc --noEmit --pretty` is clean and +`npx vitest run lib/services/phishing-detector.test.ts` passes 17/17 after all fixes are applied +(current HEAD). + +### Database Verification (Live Dev Postgres) + +Both migrations were confirmed **applied to the running `pulse-postgres` container**, not just +present as files: + +``` +docker exec pulse-postgres psql -U pulse_user -d pulse_autotask -c \ + "SELECT count(*) FROM information_schema.tables WHERE table_name IN + ('campaigns','reports','messages','indicators','classifications','remediation_actions','audit_events');" +→ 7 + +docker exec pulse-postgres psql -U pulse_user -d pulse_autotask -c "\d reports" +→ all expected columns present (ticket_id, content_hash, matched_patterns, evidence, etc.) + +docker exec pulse-postgres psql -U pulse_user -d pulse_autotask -c \ + "SELECT id, sync_type, is_enabled, cron_expression FROM sync_schedules WHERE id='phishing-sweep';" +→ phishing-sweep | phishing-sweep | f | 0 5 * * * + +pg_constraint on reports → uq_reports_ticket_id (unique), reports_ticket_id_fkey, + reports_campaign_id_fkey all present +``` + +### Requirements Coverage + +| Requirement | Source Plan | Description | Status | Evidence | +|-------------|------------|-------------|--------|----------| +| DETECT-01 | 15-01, 15-02, 15-03 | Scan recent tickets, flag candidates matching known patterns | SATISFIED | `matchesPhishingPatterns` + webhook/sweep wiring, all 8 patterns tested | +| DETECT-02 | 15-01, 15-02, 15-03 | Re-scanning doesn't reprocess unchanged ticket; reprocesses on change | SATISFIED | content-hash gate + `uq_reports_ticket_id` upsert, tested | +| EVID-01 | 15-01, 15-02 | Extract ticket ID/number, company, requester/reporter, title, description, notes, time entries, attachment metadata | SATISFIED | `gatherTicketEvidence` + `reports` schema columns, confirmed in live DB | + +No orphaned requirements — REQUIREMENTS.md's Traceability table maps only DETECT-01, DETECT-02, +EVID-01 to Phase 15, and all three appear in the plan frontmatters and are satisfied above. + +### Anti-Patterns Found + +None blocking. No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers in any file modified by +this phase. No empty stub implementations. No hardcoded empty evidence. + +Two informational items, both explicitly accepted as backlog in `15-REVIEW.md`'s "Post-Review Fix +Note" (not blocking phase 15's goal, which is about idempotent detection + evidence capture, not +100% webhook delivery guarantees): + +- If Autotask's entity-fetch fails during `handleCreateOrUpdate` (rate limit/transient outage), + a genuinely new ticket's row never lands in Postgres before `triggerPhishingDetection` reads it + back — detection is silently skipped with only a `console.warn`, and the only backstop is the + `phishing-sweep` cron, which ships `is_enabled: false` by default (matches the existing + `tickets-reconcile`/`pax8-daily` disabled-by-default convention in this codebase). +- The webhook path only triggers detection on ticket CREATE (D-01 explicit decision in + `15-CONTEXT.md`), not UPDATE — a ticket retitled into a matching pattern after creation is only + caught by the (disabled-by-default) sweep. + +These are real operational caveats worth an admin enabling `phishing-sweep` in production, but +they do not block Phase 15's stated goal — the schema exists, detection is idempotent on the +paths that do fire, and evidence capture is complete for every ticket that is detected. + +### Behavioral Spot-Checks + +| Behavior | Command | Result | Status | +|----------|---------|--------|--------| +| Phishing detector pure-function + orchestration test suite | `npx vitest run lib/services/phishing-detector.test.ts` | 17/17 passed | PASS | +| Type check across modified files | `npx tsc --noEmit --pretty` | 0 errors | PASS | +| All 7 tables present in live dev Postgres | `docker exec pulse-postgres psql ... information_schema.tables` | 7 | PASS | +| `reports` unique constraint present in live dev Postgres | `docker exec pulse-postgres psql ... pg_constraint` | `uq_reports_ticket_id\|u` | PASS | +| `phishing-sweep` schedule row present in live dev Postgres | `docker exec pulse-postgres psql ... sync_schedules` | row found, `is_enabled=f` | PASS | +| Commits referenced in review/fix reports actually exist | `git show --stat ` for ecc34b4, c875081, 7c63c5f, 9c4584d | all 4 found with matching diffs | PASS | + +### Probe Execution + +No `scripts/*/tests/probe-*.sh` files declared or found for this phase; no probe-based +verification was specified in the PLAN/SUMMARY files. Step 7c: SKIPPED (no probes declared). + +### Human Verification Required + +None. All must-haves are verifiable via code inspection, unit tests, type-check, and direct +querying of the live dev database — no visual/UX/real-time behavior requiring human judgment in +this phase (no UI, per `**UI hint**: no` in ROADMAP.md). + +### Gaps Summary + +No gaps. All 4 ROADMAP success criteria are verified against the actual codebase and the live +dev database (not just SUMMARY claims). The critical bug found in code review (webhook detection +reading an unpopulated field) and all three follow-up warnings were independently re-verified as +correctly fixed, with no regressions. Two known operational caveats (sweep disabled by default; +CREATE-only webhook trigger) are explicitly accepted as intentional/backlog per the phase's own +review trail and do not block the phase goal. + +--- + +_Verified: 2026-07-15T12:20:40Z_ +_Verifier: Claude (gsd-verifier)_