fix(audit-2): integration regressions + data-integrity from second-pass review

Two reviewer agents did a second-pass deep audit of the 21-commit
refactor. Eight findings; four fixed here (one was deferred with a
schema comment, three were 🟡 nice-to-haves left for follow-up).

Integration regressions (🟠 high):
- Outbound webhook `interest.berth_linked` now fires from the new
  junction-add handler. Was emitting a socket-only event, leaving
  external integrations silent post-refactor.
- Two new webhook events `interest.berth_unlinked` and
  `interest.berth_link_updated` added to WEBHOOK_EVENTS +
  INTERNAL_TO_WEBHOOK_MAP. PATCH and DELETE handlers now dispatch them
  alongside the existing socket emits — lifecycle parity restored.
- BerthInterestPulse adds useRealtimeInvalidation for berth-link
  events. The query key was berth-scoped while the linked-berths
  dialog invalidates interest-scoped keys (no prefix match), so the
  pulse went stale. Bridges via the realtime hook now.

Recommender semantic fix (🟠 medium-high):
- aggregates CTE: active_interest_count now filters on
  `ib.is_specific_interest = true`, matching the public-map "Under
  Offer" derivation. EOI-bundle-only links no longer demote a berth
  to Tier C for other reps. Smoke test confirms previously-all-Tier-C
  results now correctly classify as Tier A.
- Same CTE: `total_interest_count` uses COUNT(ib.berth_id) instead of
  COUNT(*) so a berth with no junction rows reports 0 (not 1 from
  the LEFT JOIN's NULL-right-side row). Prevents heat over-counting.

Data integrity (🟠):
- AcroForm tier rejects negative numerics in coerceFieldValue (was
  letting through `length_ft="-50"` which would poison the
  recommender feasibility filter on apply).
- FilesystemBackend.resolveHmacSecret throws in production when
  storage_proxy_hmac_secret_encrypted is null. Dev still derives from
  BETTER_AUTH_SECRET for ergonomics; prod must explicitly configure.
- Documented the circular FK between berths.current_pdf_version_id
  and berth_pdf_versions.id. Drizzle's `.references()` can't express
  the cycle so the schema column is plain text + a comment; the FK
  is authoritatively maintained by migration 0030.

Tests still 1163/1163. tsc clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Matt Ciaccio
2026-05-05 04:20:38 +02:00
parent 312ebf1a88
commit a3e002852b
8 changed files with 85 additions and 8 deletions

View File

@@ -103,6 +103,12 @@ export const patchHandler: RouteHandler = async (req, ctx, params) => {
interestId, interestId,
berthId, berthId,
}); });
void import('@/lib/services/webhook-dispatch').then(({ dispatchWebhookEvent }) =>
dispatchWebhookEvent(ctx.portId, 'interest:berthLinkUpdated', {
interestId,
berthId,
}),
);
return NextResponse.json({ data: updated }); return NextResponse.json({ data: updated });
} catch (error) { } catch (error) {
@@ -136,6 +142,12 @@ export const deleteHandler: RouteHandler = async (_req, ctx, params) => {
interestId, interestId,
berthId, berthId,
}); });
void import('@/lib/services/webhook-dispatch').then(({ dispatchWebhookEvent }) =>
dispatchWebhookEvent(ctx.portId, 'interest:berthUnlinked', {
interestId,
berthId,
}),
);
return new NextResponse(null, { status: 204 }); return new NextResponse(null, { status: 204 });
} catch (error) { } catch (error) {

View File

@@ -92,6 +92,15 @@ export const addHandler: RouteHandler = async (req, ctx, params) => {
interestId, interestId,
berthId: body.berthId, berthId: body.berthId,
}); });
// Outbound webhook: the legacy /link-berth path dispatched
// `interest.berth_linked` and external integrations subscribe to it.
// The new junction-add path must keep that contract.
void import('@/lib/services/webhook-dispatch').then(({ dispatchWebhookEvent }) =>
dispatchWebhookEvent(ctx.portId, 'interest:berthLinked', {
interestId,
berthId: body.berthId,
}),
);
return NextResponse.json({ data: link }, { status: 201 }); return NextResponse.json({ data: link }, { status: 201 });
} catch (error) { } catch (error) {

View File

@@ -11,6 +11,7 @@ import { apiFetch } from '@/lib/api/client';
import { stageBadgeClass, stageLabel } from '@/lib/constants'; import { stageBadgeClass, stageLabel } from '@/lib/constants';
import { computeUrgencyBadges } from '@/components/interests/urgency'; import { computeUrgencyBadges } from '@/components/interests/urgency';
import type { InterestRow } from '@/components/interests/interest-columns'; import type { InterestRow } from '@/components/interests/interest-columns';
import { useRealtimeInvalidation } from '@/hooks/use-realtime-invalidation';
import { cn } from '@/lib/utils'; import { cn } from '@/lib/utils';
interface InterestsResponse { interface InterestsResponse {
@@ -33,8 +34,9 @@ export function BerthInterestPulse({ berthId }: { berthId: string }) {
const params = useParams<{ portSlug: string }>(); const params = useParams<{ portSlug: string }>();
const portSlug = params?.portSlug ?? ''; const portSlug = params?.portSlug ?? '';
const queryKey = ['interests', { berthId, sort: 'dateLastContact', order: 'desc' }];
const { data, isLoading } = useQuery<InterestsResponse>({ const { data, isLoading } = useQuery<InterestsResponse>({
queryKey: ['interests', { berthId, sort: 'dateLastContact', order: 'desc' }], queryKey,
queryFn: () => queryFn: () =>
apiFetch<InterestsResponse>( apiFetch<InterestsResponse>(
`/api/v1/interests?berthId=${berthId}&limit=10&sort=dateLastContact&order=desc`, `/api/v1/interests?berthId=${berthId}&limit=10&sort=dateLastContact&order=desc`,
@@ -42,6 +44,19 @@ export function BerthInterestPulse({ berthId }: { berthId: string }) {
staleTime: 30_000, staleTime: 30_000,
}); });
// Stay in sync with the linked-berths list + add-to-interest dialog.
// Each of those flows emits a realtime socket event but does NOT
// invalidate this exact query key (it's berth-scoped, theirs are
// interest-scoped) — bridge via the invalidation hook.
useRealtimeInvalidation({
'interest:berthLinked': [queryKey],
'interest:berthUnlinked': [queryKey],
'interest:berthLinkUpdated': [queryKey],
'interest:created': [queryKey],
'interest:stageChanged': [queryKey],
'interest:archived': [queryKey],
});
const all = data?.data ?? []; const all = data?.data ?? [];
const active = all.filter((i) => !i.archivedAt && !i.outcome); const active = all.filter((i) => !i.archivedAt && !i.outcome);
const preview = active.slice(0, PREVIEW_LIMIT); const preview = active.slice(0, PREVIEW_LIMIT);

View File

@@ -92,6 +92,17 @@ export const berths = pgTable(
], ],
); );
// Note: `berths.current_pdf_version_id` has an `ON DELETE SET NULL` FK to
// `berth_pdf_versions.id` installed by migration 0030. The column is left
// without a `.references()` / `foreignKey()` declaration in the Drizzle
// schema because the two tables form a circular FK (berth_pdf_versions →
// berths), and Drizzle's relation inference doesn't tolerate the cycle
// when both sides are declared via column-level `.references()`. The
// migration chain authoritatively maintains the constraint; a fresh
// `db:push` against an empty DB would skip the FK and require a follow-up
// generated migration to add it back. This is acceptable because we
// always apply migrations in order in dev/CI/prod.
export const berthMapData = pgTable( export const berthMapData = pgTable(
'berth_map_data', 'berth_map_data',
{ {

View File

@@ -470,9 +470,13 @@ function coerceFieldValue(key: keyof ExtractedBerthFields, raw: string): string
} }
return raw; return raw;
} }
// Numeric columns: strip currency / unit suffixes and commas. // Numeric columns: strip currency / unit suffixes and commas. Berth
// dimensions / capacities / prices are all non-negative — reject
// negatives outright so an AcroForm with `length_ft="-50"` doesn't
// poison the recommender feasibility filter when applied.
const numeric = Number(raw.replace(/[^0-9.\-]/g, '')); const numeric = Number(raw.replace(/[^0-9.\-]/g, ''));
return Number.isFinite(numeric) ? numeric : null; if (!Number.isFinite(numeric)) return null;
return numeric < 0 ? null : numeric;
} }
/** Parse a human date like "September 15 2025" → "2025-09-15". */ /** Parse a human date like "September 15 2025" → "2025-09-15". */

View File

@@ -448,7 +448,15 @@ export async function recommendBerths(args: RecommendBerthsArgs): Promise<Recomm
aggregates AS ( aggregates AS (
SELECT SELECT
f.id AS berth_id, f.id AS berth_id,
COUNT(*) FILTER (WHERE i.archived_at IS NULL AND i.outcome IS NULL) AS active_interest_count, -- Active = is_specific_interest=true junction rows only (matches
-- the public-map "Under Offer" filter). An EOI-bundle-only link
-- (is_specific_interest=false, is_in_eoi_bundle=true) is legal
-- coverage, not a pitch, and shouldn't demote the berth.
COUNT(*) FILTER (
WHERE i.archived_at IS NULL
AND i.outcome IS NULL
AND ib.is_specific_interest = true
) AS active_interest_count,
COUNT(*) FILTER ( COUNT(*) FILTER (
WHERE i.outcome IS NOT NULL AND (i.outcome::text LIKE 'lost%' OR i.outcome = 'cancelled') WHERE i.outcome IS NOT NULL AND (i.outcome::text LIKE 'lost%' OR i.outcome = 'cancelled')
) AS lost_count, ) AS lost_count,
@@ -483,7 +491,11 @@ export async function recommendBerths(args: RecommendBerthsArgs): Promise<Recomm
) FILTER (WHERE i.outcome IS NOT NULL AND (i.outcome::text LIKE 'lost%' OR i.outcome = 'cancelled')), ) FILTER (WHERE i.outcome IS NOT NULL AND (i.outcome::text LIKE 'lost%' OR i.outcome = 'cancelled')),
0 0
) AS fallthrough_max_stage, ) AS fallthrough_max_stage,
COUNT(*) AS total_interest_count, -- COUNT(ib.berth_id) (not COUNT(*)) so a berth with no junction
-- rows reports 0 — the LEFT JOIN otherwise produces a single
-- NULL-right-side row that COUNT(*) would tally as 1 and inflate
-- the heat interest-count component for berths with no history.
COUNT(ib.berth_id) AS total_interest_count,
COUNT(*) FILTER (WHERE i.eoi_status = 'signed') AS eoi_signed_count COUNT(*) FILTER (WHERE i.eoi_status = 'signed') AS eoi_signed_count
FROM feasible f FROM feasible f
LEFT JOIN interest_berths ib ON ib.berth_id = f.id LEFT JOIN interest_berths ib ON ib.berth_id = f.id

View File

@@ -11,6 +11,8 @@ export const WEBHOOK_EVENTS = [
'interest.created', 'interest.created',
'interest.stage_changed', 'interest.stage_changed',
'interest.berth_linked', 'interest.berth_linked',
'interest.berth_unlinked',
'interest.berth_link_updated',
'berth.status_changed', 'berth.status_changed',
'berth.updated', 'berth.updated',
'document.sent', 'document.sent',
@@ -51,6 +53,8 @@ export const INTERNAL_TO_WEBHOOK_MAP: Record<string, WebhookEvent> = {
'interest:created': 'interest.created', 'interest:created': 'interest.created',
'interest:stageChanged': 'interest.stage_changed', 'interest:stageChanged': 'interest.stage_changed',
'interest:berthLinked': 'interest.berth_linked', 'interest:berthLinked': 'interest.berth_linked',
'interest:berthUnlinked': 'interest.berth_unlinked',
'interest:berthLinkUpdated': 'interest.berth_link_updated',
'berth:statusChanged': 'berth.status_changed', 'berth:statusChanged': 'berth.status_changed',
'berth:updated': 'berth.updated', 'berth:updated': 'berth.updated',
'document:sent': 'document.sent', 'document:sent': 'document.sent',

View File

@@ -314,9 +314,19 @@ function resolveHmacSecret(encryptedSecret: string | null): string {
logger.error({ err }, 'Failed to decrypt storage_proxy_hmac_secret_encrypted'); logger.error({ err }, 'Failed to decrypt storage_proxy_hmac_secret_encrypted');
} }
} }
// Derive a stable per-process secret from BETTER_AUTH_SECRET so dev mode // Production refuses to derive: an admin must have explicitly configured
// works without explicit configuration. In production the admin UI writes // `storage_proxy_hmac_secret_encrypted` before flipping the storage
// an encrypted random secret. // backend to filesystem. Conflating this trust domain with the auth
// cookie HMAC (BETTER_AUTH_SECRET) is acceptable in dev for ergonomics
// but a deployment-time misconfig in prod.
if (process.env.NODE_ENV === 'production') {
throw new Error(
'FilesystemBackend: storage_proxy_hmac_secret_encrypted must be set in production. ' +
'Generate a random secret in admin > storage and persist it before flipping the backend.',
);
}
// Dev fallback: derive a stable per-process secret so the filesystem
// backend works without explicit configuration during local development.
const seed = process.env.BETTER_AUTH_SECRET ?? env.BETTER_AUTH_SECRET ?? 'storage-default'; const seed = process.env.BETTER_AUTH_SECRET ?? env.BETTER_AUTH_SECRET ?? 'storage-default';
return createHash('sha256').update(`storage-proxy:${seed}`).digest('hex'); return createHash('sha256').update(`storage-proxy:${seed}`).digest('hex');
} }