docs(15): re-review after code-review fixes
This commit is contained in:
parent
9c4584d428
commit
502e0b95a7
1 changed files with 127 additions and 143 deletions
|
|
@ -1,184 +1,168 @@
|
|||
---
|
||||
phase: 15-data-model-detection-ticket-evidence
|
||||
reviewed: 2026-07-15T00:00:00Z
|
||||
reviewed: 2026-07-15T12:14:30Z
|
||||
depth: standard
|
||||
files_reviewed: 7
|
||||
files_reviewed: 3
|
||||
files_reviewed_list:
|
||||
- migrations/097_phishing_triage_schema.sql
|
||||
- migrations/098_phishing_sweep_schedule.sql
|
||||
- lib/services/webhook-service.ts (triggerPhishingDetection method and its call site only)
|
||||
- lib/services/phishing-detector.ts
|
||||
- lib/services/phishing-detector.test.ts
|
||||
- lib/services/phishing-sweep-service.ts
|
||||
- lib/services/webhook-service.ts
|
||||
- lib/services/sync-scheduler.ts
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 2
|
||||
info: 2
|
||||
critical: 0
|
||||
warning: 4
|
||||
info: 1
|
||||
total: 5
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 15: Code Review Report
|
||||
# Phase 15: Code Review Report (re-review)
|
||||
|
||||
**Reviewed:** 2026-07-15T00:00:00Z
|
||||
**Reviewed:** 2026-07-15T12:14:30Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 7
|
||||
**Files Reviewed:** 3 (scoped subset per config)
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed the new phishing-triage schema (7 tables, `reports` fully designed and 6 stubs for later phases), the shared `phishing-detector.ts` core (pattern matcher + content-hash idempotency + evidence gathering + upsert), the `phishing-sweep-service.ts` cron reconciliation, and the two hook-in points (webhook-service.ts's new `triggerPhishingDetection`, sync-scheduler.ts's new `phishing-sweep` dispatch branch + default schedule).
|
||||
Re-review of three prior findings (CR-01, WR-01, WR-02) against fix commits `ecc34b4`,
|
||||
`c875081`, `7c63c5f`. All three are verified fixed and correctly implemented — no evidence
|
||||
of a regression or a superficial/partial fix in any of them.
|
||||
|
||||
The good news: the pattern matcher is exactly what it claims to be (plain `.toLowerCase()` + `.includes()`, no regex, no ReDoS surface), the content hash is scoped to `title`+`description` only (correctly excludes status/assignee churn per D-04), all `reports`/`companies`/`ticket_notes`/`time_entries` queries are fully parameterized, and the sync-scheduler dispatch branch is fire-and-forget-safe and wrapped in the same try/catch/status-update scaffolding as every other schedule type.
|
||||
- **CR-01 (fixed correctly):** `triggerPhishingDetection` no longer reads `payload.entity`
|
||||
(confirmed still structurally never populated — `normalizeWebhookPayload` in
|
||||
`lib/types/webhook.ts:137-157` never sets `.entity`). It now queries Postgres by
|
||||
`payload.entityId`, and the not-found case is handled safely: `console.warn` + early
|
||||
`return`, no throw. Traced the ordering: `handleCreateOrUpdate()` is `await`ed
|
||||
synchronously (webhook-service.ts:95) before `triggerPhishingDetection()` is invoked
|
||||
(webhook-service.ts:118) inside the same `processWebhook` call — so on the happy path the
|
||||
ticket row is guaranteed to exist with current `title`/`description` by the time it's read
|
||||
back. Confirmed correct.
|
||||
- **WR-01 (fixed correctly):** `phishing-detector.ts` now imports `getAutotaskClient` from
|
||||
`./autotask-factory` (line 17) instead of the private duplicate that was removed. Verified
|
||||
`autotask-factory.ts` throws a clear config-validation error when Autotask env vars are
|
||||
missing (lines 16-20), and that error is caught by `gatherTicketEvidence`'s existing
|
||||
`try/catch` around the attachments call (phishing-detector.ts:143-153), degrading
|
||||
gracefully to `attachments: []` rather than crashing detection. No unused imports left
|
||||
behind from the removed duplicate.
|
||||
- **WR-02 (fixed correctly):** The unchanged-content-hash branch (phishing-detector.ts:190-204)
|
||||
now issues `UPDATE reports SET evidence = ...` even when `content_hash` matches, touching
|
||||
only the `evidence`/`updated_at` columns — `title`, `description`, `matched_patterns`, and
|
||||
`content_hash` are left untouched, so this does not reintroduce reprocessing on pure
|
||||
status/assignee churn. Confirmed via `computePhishingContentHash`, which hashes only
|
||||
`title`+`description` (phishing-detector.ts:58-65), combined with the fact that the webhook
|
||||
trigger only fires on `CREATE` events (webhook-service.ts:114) — so status/assignee changes
|
||||
on an already-detected ticket never even reach `detectPhishingTicket` via the webhook path.
|
||||
The only caller that repeatedly exercises this branch is the cron sweep
|
||||
(`phishing-sweep-service.ts`), which is itself idempotent and hash-gated.
|
||||
|
||||
The bad news: tracing the webhook hook-in across `app/api/webhooks/autotask/route.ts` → `lib/types/webhook.ts` → `lib/services/webhook-service.ts` shows `triggerPhishingDetection` reads from `payload.entity`, a field that is **never populated** anywhere in the real webhook flow (Autotask's actual payload only carries `Action/Guid/EntityType/Id/Fields/EventTime`, and `normalizeWebhookPayload` never sets `.entity`). As a result the "primary near-real-time trigger" described in `phishing-sweep-service.ts`'s own docstring never actually flags a ticket — every webhook-triggered call to `detectPhishingTicket` runs with `title: null, description: null`, which can never match any pattern. This is a functional regression that should block ship; see CR-01.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Webhook-triggered phishing detection never actually detects anything (`payload.entity` is always `undefined`)
|
||||
|
||||
**File:** `lib/services/webhook-service.ts:449-480` (root cause spans `lib/types/webhook.ts:97-157` and `app/api/webhooks/autotask/route.ts:41-53`)
|
||||
|
||||
**Issue:**
|
||||
`triggerPhishingDetection` branches on `payload.entity`:
|
||||
|
||||
```ts
|
||||
if (payload.entity) {
|
||||
ticket = {
|
||||
id: payload.entityId,
|
||||
ticket_number: payload.entity.ticketNumber || null,
|
||||
title: payload.entity.title || null,
|
||||
description: payload.entity.description || null,
|
||||
...
|
||||
};
|
||||
} else {
|
||||
ticket = { id: payload.entityId, ticket_number: null, title: null, description: null, ... };
|
||||
}
|
||||
```
|
||||
|
||||
But `payload.entity` is structurally never populated:
|
||||
- Autotask's real webhook payload (`AutotaskRawWebhookPayload`, `lib/types/webhook.ts:83-92`) only contains `Action, Guid, EntityType, Id, Fields, EventTime, SequenceNumber, PersonId` — there is no embedded entity body. `app/api/webhooks/autotask/route.ts`'s own header comment confirms this is the *actual* payload shape.
|
||||
- `normalizeWebhookPayload()` (`lib/types/webhook.ts:137-157`) constructs `AutotaskWebhookPayload` from the raw payload and never assigns `.entity` — it's left `undefined`.
|
||||
- `handleCreateOrUpdate()` (`webhook-service.ts:157-180`) fetches the full ticket separately (either from `payload.entity` — always absent — or via `client.getEntityById(...)`) and upserts it into Postgres, but never writes the fetched entity back onto `payload.entity`. By the time `triggerPhishingDetection(payload)` runs (same `payload` object, fire-and-forget after the response path), `payload.entity` is still `undefined`.
|
||||
|
||||
So every real invocation takes the `else` branch: `title: null, description: null`. `matchesPhishingPatterns(null, null)` computes `haystack = ' '` (empty + space + empty) and `.includes()` against all 8 locked patterns is always `false`. `detectPhishingTicket` therefore returns `{ flagged: false }` immediately, before ever reaching the content-hash / evidence-gathering / upsert logic. **No `reports` row is ever created via the webhook path.**
|
||||
|
||||
This is worse than a no-op: the code logs `[WEBHOOK] Triggering phishing detection for ticket ${payload.entityId}` on every ticket creation, which reads as confirmation the pipeline ran, while it silently never matches. The only thing that will ever populate `reports` is the nightly `phishing-sweep` cron (disabled by default, `0 5 * * *`), directly contradicting the stated design ("webhook path... is the primary near-real-time trigger; this sweep catches anything the webhook missed" — `phishing-sweep-service.ts:5-9`).
|
||||
|
||||
**Fix:** Don't rely on `payload.entity`. Either fetch the ticket row from Postgres (already upserted earlier in the same request by `handleCreateOrUpdate`) or reuse the entity `handleCreateOrUpdate` already fetched from Autotask, and thread it through to `triggerPhishingDetection`:
|
||||
|
||||
```ts
|
||||
private async triggerPhishingDetection(payload: AutotaskWebhookPayload): Promise<void> {
|
||||
const row = await postgresClient.query<{
|
||||
id: string; ticket_number: string | null; title: string | null;
|
||||
description: string | null; company_id: number | null;
|
||||
contact_id: number | null; created_by_contact_id: number | null;
|
||||
}>(
|
||||
`SELECT id, ticket_number, title, description, company_id, contact_id, created_by_contact_id
|
||||
FROM tickets WHERE id = $1`,
|
||||
[payload.entityId]
|
||||
);
|
||||
const r = row.rows[0];
|
||||
if (!r) {
|
||||
console.warn(`[WEBHOOK] Skipping phishing detection — ticket ${payload.entityId} not found in Postgres yet`);
|
||||
return;
|
||||
}
|
||||
const ticket: DetectableTicket = {
|
||||
id: Number(r.id),
|
||||
ticket_number: r.ticket_number,
|
||||
title: r.title,
|
||||
description: r.description,
|
||||
company_id: r.company_id,
|
||||
contact_id: r.contact_id,
|
||||
created_by_contact_id: r.created_by_contact_id,
|
||||
};
|
||||
await detectPhishingTicket(ticket);
|
||||
}
|
||||
```
|
||||
|
||||
This runs after `handleCreateOrUpdate` has already upserted the ticket, so the row is guaranteed to exist and have current `title`/`description`.
|
||||
No new bugs were introduced by the three fixes. The findings below are gaps surfaced while
|
||||
tracing the fixes (some latent in surrounding logic not touched by the fix commits, one a
|
||||
still-outstanding item from the prior review pass).
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `phishing-detector.ts` duplicates the Autotask client factory and drops its config validation
|
||||
### WR-01: `gatherTicketEvidence` doesn't filter soft-deleted notes/time entries
|
||||
|
||||
**File:** `lib/services/phishing-detector.ts:112-123`
|
||||
|
||||
**Issue:** The codebase already has a shared factory, `lib/services/autotask-factory.ts::getAutotaskClient()`, which validates all four required env vars and **throws** a clear error when Autotask isn't configured. `phishing-detector.ts` instead hand-rolls its own private module-level singleton with the same shape but no validation:
|
||||
|
||||
```ts
|
||||
let _autotaskClient: AutotaskClient | null = null;
|
||||
function getAutotaskClient(): AutotaskClient {
|
||||
if (!_autotaskClient) {
|
||||
_autotaskClient = new AutotaskClient({
|
||||
apiUrl: process.env.AUTOTASK_API_URL || '',
|
||||
username: process.env.AUTOTASK_USERNAME || '',
|
||||
password: process.env.AUTOTASK_SECRET || '',
|
||||
apiIntegrationCode: process.env.AUTOTASK_API_INTEGRATION_CODE || '',
|
||||
});
|
||||
}
|
||||
return _autotaskClient;
|
||||
}
|
||||
**File:** `lib/services/phishing-detector.ts:126-140`
|
||||
**Issue:** The notes and time-entries queries select all rows for the ticket with no
|
||||
`is_deleted = false` filter, even though both `ticket_notes` and `time_entries` have that
|
||||
column (`migrations/025_create_ticket_notes.sql:15`, `migrations/006_add_time_entries_table.sql:47`).
|
||||
A note or time entry retracted in Autotask after the fact still appears in every future
|
||||
evidence snapshot — including the WR-02 refresh path, which was specifically added so
|
||||
evidence doesn't go stale. Showing a retracted note as live evidence in a phishing-triage
|
||||
snapshot is a correctness problem for whatever later phase reads `reports.evidence`, not
|
||||
just a cosmetic one.
|
||||
**Fix:**
|
||||
```sql
|
||||
SELECT id, title, description, note_type, creator_resource_id, created_at
|
||||
FROM ticket_notes
|
||||
WHERE ticket_id = $1 AND is_deleted = false
|
||||
ORDER BY created_at
|
||||
```
|
||||
(apply the same `is_deleted = false` filter to the `time_entries` query.)
|
||||
|
||||
Since `gatherTicketEvidence`'s call to `getAttachments()` is wrapped in a try/catch that swallows all errors to `attachments = []`, a misconfigured Autotask integration will silently produce empty attachment evidence forever, with no distinguishable signal from "ticket genuinely has no attachments." It also creates a second, independently-cached `AutotaskClient` instance parallel to the one in `autotask-factory.ts` and the one duplicated again in `webhook-service.ts`.
|
||||
### WR-02: Phishing detection is silently skipped with no retry when the ticket row isn't in Postgres yet
|
||||
|
||||
**Fix:** Import and use the shared factory instead of re-implementing it:
|
||||
```ts
|
||||
import { getAutotaskClient } from './autotask-factory';
|
||||
```
|
||||
and drop the local `_autotaskClient`/`getAutotaskClient` in `phishing-detector.ts`.
|
||||
**File:** `lib/services/webhook-service.ts:457-476`
|
||||
**Issue:** If `handleCreateOrUpdate()`'s Autotask entity fetch returns `null` (API hiccup,
|
||||
rate limit, transient unavailability — see `webhook-service.ts:173-176`, which already
|
||||
warns-and-continues without upserting or retrying), a genuinely new ticket's row never lands
|
||||
in Postgres. `triggerPhishingDetection` then finds no row, logs a `console.warn`, and returns
|
||||
— no retry, no re-queue. The call is fire-and-forget
|
||||
(`.catch(err => console.error(...))` at the call site), so nothing else observes the miss.
|
||||
The only backstop is the `phishing-sweep` cron job, registered `is_enabled: false` by default
|
||||
(`sync-scheduler.ts:308`). In the default configuration, a brand-new phishing ticket that
|
||||
races the entity-fetch failure is never detected until an operator manually enables the sweep.
|
||||
**Fix:** At minimum, log this case with the same visibility as a failed webhook (not just
|
||||
`console.warn`) so ops can spot the gap, and/or surface in the admin UI that `phishing-sweep`
|
||||
should be enabled for detection reliability. Longer-term, consider re-queuing the ticket ID
|
||||
for a delayed re-check instead of relying solely on the next scheduled sweep.
|
||||
|
||||
### WR-02: Evidence snapshot goes stale — the content-hash gate that (correctly) excludes status/assignee churn also silently skips re-gathering evidence
|
||||
### WR-03: Webhook path only triggers phishing detection on ticket CREATE, never on UPDATE
|
||||
|
||||
**File:** `lib/services/phishing-detector.ts:195-207`
|
||||
**File:** `lib/services/webhook-service.ts:113-121`
|
||||
**Issue:** `triggerPhishingDetection` only fires when
|
||||
`payload.eventType === WebhookEventType.CREATE`. If a ticket's title/description changes
|
||||
*after* creation (a technician retitles a miscategorized ticket to include a locked phishing
|
||||
phrase, or a customer's subject line gets corrected), the webhook path never re-runs
|
||||
detection for that ticket — the content-hash gate in `detectPhishingTicket` never even gets a
|
||||
chance to see the change via the near-real-time path. Detection of that case depends entirely
|
||||
on the disabled-by-default nightly sweep, the same gap noted in the prior review's WR-02
|
||||
discussion of staleness, just from the opposite direction (missed detection vs. stale
|
||||
evidence).
|
||||
**Fix:** Either extend the trigger condition to also cover `WebhookEventType.UPDATE` (cheap
|
||||
and safe given the content-hash gate makes unrelated updates a no-op), or explicitly document
|
||||
that UPDATE-triggered re-detection is intentionally deferred to the cron sweep, and make sure
|
||||
that sweep ships enabled before relying on it.
|
||||
|
||||
**Issue:** `detectPhishingTicket` short-circuits before evidence gathering whenever `content_hash` (over `title`+`description` only) is unchanged:
|
||||
### WR-04: New evidence-refresh logic (WR-02 fix) still has zero test coverage
|
||||
|
||||
```ts
|
||||
if (existing.rowCount && existing.rowCount > 0 && existing.rows[0].content_hash === contentHash) {
|
||||
return { flagged: true, skippedUnchanged: true };
|
||||
}
|
||||
const evidence = await gatherTicketEvidence(ticket);
|
||||
```
|
||||
|
||||
This correctly avoids reprocessing on pure status/assignee churn (the D-04 design goal), but it also means `reports.evidence` (notes, time entries, attachment metadata) is frozen at whatever it was captured on the first detection. If an analyst adds a new ticket note, logs time, or an attachment lands on the ticket later — all common during triage — and the title/description text doesn't change, the stored evidence snapshot never refreshes. Later phases (16+) that read `reports.evidence` for message/indicator extraction will be working from a stale snapshot unless something re-triggers full reprocessing (e.g. a title/description edit).
|
||||
|
||||
**Fix:** Consider decoupling "should we re-run pattern matching + rewrite content_hash" from "should we refresh the evidence snapshot" — e.g. re-gather evidence on every sweep pass for still-open reports regardless of content_hash, or hash evidence inputs (notes/time-entries/attachment count) separately and refresh when that changes. At minimum, document this as an accepted limitation if it's intentional, since it isn't currently called out anywhere in the code comments (only the status/assignee-churn exclusion is documented).
|
||||
**File:** `lib/services/phishing-detector.test.ts` (whole file), `lib/services/phishing-detector.ts:174-247`
|
||||
**Issue:** `phishing-detector.test.ts` only exercises the two pure functions
|
||||
(`matchesPhishingPatterns`, `computePhishingContentHash`) — unchanged from the previous
|
||||
review pass (previously flagged as IN-02). `detectPhishingTicket`, including the exact branch
|
||||
this re-review was asked to verify (evidence refresh on an unchanged content hash), still has
|
||||
no test coverage. A regression to the hash-gate condition, the
|
||||
`UPDATE reports SET evidence = ...` statement, or the `ON CONFLICT (ticket_id) DO UPDATE`
|
||||
upsert would not be caught by `npm test`. Given this exact code path was the subject of two
|
||||
review cycles now, it's the highest-value place in this file to add coverage.
|
||||
**Fix:** Add tests using the `postgresClient`-mocking pattern already established in
|
||||
`lib/services/pax8-sync-service.test.ts` / `pax8-company-matcher.test.ts`, covering: new-ticket
|
||||
insert, unchanged-hash evidence-only refresh (assert `title`/`content_hash` unchanged but
|
||||
`evidence`/`updated_at` updated, and that `gatherTicketEvidence` is still called exactly once),
|
||||
and changed-hash full reprocessing.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Non-parameterized SQL interpolation of a sweep-window constant
|
||||
### IN-01: `reportId` now populated on the unchanged-hash branch (behavior improvement, not a defect)
|
||||
|
||||
**File:** `lib/services/phishing-sweep-service.ts:48`
|
||||
**File:** `lib/services/phishing-detector.ts:203`
|
||||
**Issue:** Not a bug — flagging for completeness since it changed as a side effect of the
|
||||
WR-02 fix. Previously the unchanged-hash branch returned
|
||||
`{ flagged: true, skippedUnchanged: true }` with no `reportId`. The fix now also returns
|
||||
`reportId: existing.rows[0].id`, which is strictly more useful to callers. Nothing currently
|
||||
consumes it incorrectly (`phishing-sweep-service.ts` doesn't read it), but worth noting in the
|
||||
review trail since it's an unrequested (though harmless) API surface change.
|
||||
**Fix:** None needed.
|
||||
|
||||
**Issue:** The sweep query builds part of its `WHERE` clause via template-literal interpolation rather than a bound parameter:
|
||||
```ts
|
||||
WHERE is_deleted = false
|
||||
AND last_activity_date > NOW() - INTERVAL '${SWEEP_WINDOW_DAYS} days'
|
||||
ORDER BY last_activity_date DESC
|
||||
LIMIT $1
|
||||
```
|
||||
`SWEEP_WINDOW_DAYS` is a hardcoded module constant (`7`), not attacker-controlled, so this isn't currently exploitable. But it's a pattern this phase was explicitly asked to be careful about, and it's an easy thing to regress if `SWEEP_WINDOW_DAYS` is ever made configurable (e.g. via an admin-editable setting) without someone remembering to re-parameterize it.
|
||||
## Post-Review Fix Note
|
||||
|
||||
**Fix:** Use `make_interval(days => $2)` (or similar) with a bound parameter instead of string interpolation, even though today it's a constant:
|
||||
```ts
|
||||
AND last_activity_date > NOW() - make_interval(days => $2)
|
||||
...
|
||||
const candidates = await postgresClient.query(candidatesQuery, [SCAN_LIMIT, SWEEP_WINDOW_DAYS]);
|
||||
```
|
||||
|
||||
### IN-02: No coverage for the orchestration path (`detectPhishingTicket`, `gatherTicketEvidence`) despite an established mocking precedent
|
||||
|
||||
**File:** `lib/services/phishing-detector.test.ts`, `lib/services/phishing-detector.ts:130-248`
|
||||
|
||||
**Issue:** The test file only covers the two pure functions (`matchesPhishingPatterns`, `computePhishingContentHash`). The stateful orchestration — the idempotency check, the upsert, and evidence gathering, which is where CR-01/WR-02-adjacent bugs live — has zero test coverage. This isn't out of line with most of the codebase (per CLAUDE.md, most services have no tests), but the project does have a working precedent for mocking `postgresClient` in a service test (`lib/services/pax8-sync-service.test.ts`, `lib/services/pax8-company-matcher.test.ts`), so a mocked-DB test for the idempotency branch (unchanged hash → `skippedUnchanged: true` without a second evidence fetch) would have been feasible and would have caught regressions in this exact logic cheaply.
|
||||
|
||||
**Fix:** Add a `postgresClient`-mocked test asserting that a second `detectPhishingTicket` call with an identical title/description does not re-invoke `gatherTicketEvidence`/Autotask, following the `pax8-sync-service.test.ts` mocking pattern.
|
||||
The `WR-01 (new)` finding above (missing `is_deleted = false` filter on
|
||||
`ticket_notes`/`time_entries` in `gatherTicketEvidence`) was fixed directly
|
||||
after this re-review, commit `9c4584d`. Remaining items (`WR-02 (new)`
|
||||
ticket-not-found race with the sweep disabled by default, `WR-03 (new)`
|
||||
webhook fires on CREATE only, `WR-04`/test coverage, `IN-01`) are accepted
|
||||
as follow-up/backlog items rather than blocking this phase:
|
||||
- Disabled-by-default sweep matches this codebase's established convention
|
||||
for new scheduled integrations (e.g. `pax8-daily`) — an admin opts in via
|
||||
`/admin` once ready; not unique to this phase.
|
||||
- CREATE-only webhook trigger matches the explicit `D-01` decision in
|
||||
`15-CONTEXT.md` (hook into `ticket.created`), not an oversight.
|
||||
- Orchestration-path test coverage gap is consistent with most of this
|
||||
codebase per CLAUDE.md (no tests outside `analyzer/`, `rmm/`, `b2/`).
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-07-15T00:00:00Z_
|
||||
_Reviewed: 2026-07-15T12:14:30Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue