fix(24): address code-review findings — PATCH identity guard, empty-array tombstone, health-check timeout, record-key normalization

Two critical issues from the post-phase code review:

- PATCH /api/route53/zones/[zoneId]/records/[recordId] never verified the
  request body's name/type/setIdentifier matched the record identified by
  the URL. A mismatch would silently UPSERT a brand-new AWS recordset
  (leaving the original live and untouched) while corrupting the mirror's
  record_key invariant. Now rejects with 400 if any of those three fields
  differ from the existing record — renaming/retyping is delete-plus-create,
  not an update.

- route53-sync-service.ts's syncZones()/syncRecords() tombstone queries used
  "id <> ALL(seenIds)" style queries with no empty-array guard — a
  successful-but-empty AWS response would soft-delete every previously
  synced zone/record in one shot. Same bug class already fixed in
  pax8-sync-service.ts; now guarded the same way here.

Two smaller fixes:

- checkRoute53()'s AWS auth probe had no timeout, unlike every other
  integration's liveCheck() (8s AbortController). Added the same bound via
  the SDK's abortSignal option.
- buildRecordKey() relied on every caller to pre-normalize name/type case
  before calling it. Now normalizes internally (lowercase name, uppercase
  type) so the record_key invariant holds regardless of caller discipline.

Full REVIEW.md findings in 24-REVIEW.md. Two remaining Warnings (alias
records un-editable/undeletable, no admin-UI surface for route53_audit_log)
deliberately left as backlog items for a follow-up phase — out of scope for
a post-execution fix pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

app/api/route53/zones/[zoneId]/records/[recordId]/route.ts

@@ -91,6 +91,26 @@ export async function PATCH(
   if (!existing) {
     return NextResponse.json({ error: 'Record not found' }, { status: 404 });
   }
+
+  // name/type/setIdentifier are immutable via PATCH — Route 53 identifies a
+  // recordset by Name+Type+SetIdentifier, not by any Pulse-internal id, so a
+  // mismatch here would UPSERT a brand-new AWS recordset (leaving the one
+  // `recordId` actually denotes untouched) while corrupting the mirror's
+  // record_key = zoneId:name:type:setIdentifier invariant. The UI disables
+  // these fields in edit mode, but per T-24-02 the server never relies on
+  // the UI hiding a control — reject any attempt to rename/retype instead of
+  // silently targeting a different record set.
+  const existingSetIdentifier = existing.setIdentifier ?? null;
+  if (name !== existing.name || type !== existing.type || setIdentifier !== existingSetIdentifier) {
+    return NextResponse.json(
+      {
+        error: 'Cannot change name/type/setIdentifier via update',
+        message: 'Renaming or retyping a record is a delete-plus-create, not an update.',
+      },
+      { status: 400 }
+    );
+  }
+
   const beforeValue = {
     name: existing.name,
     type: existing.type,

lib/services/integration-health.ts

@@ -218,6 +218,7 @@ function isAwsAuthError(err: unknown): boolean {
 }
 
 const ROUTE53_ZONE_CHECK_LIMIT = 50;
+const ROUTE53_AUTH_PROBE_TIMEOUT_MS = 8000;
 
 async function checkRoute53(): Promise<IntegrationHealth> {
   const checkedAt = new Date().toISOString();
@@ -232,7 +233,19 @@ async function checkRoute53(): Promise<IntegrationHealth> {
   let status: HealthStatus;
   let error: string | null = null;
   try {
-    await getRoute53Client().send(new ListHostedZonesCommand({ MaxItems: 1 }));
+    // Bounded the same way liveCheck() bounds every other integration's
+    // fetch() (8s) — the AWS SDK's own retry policy has no caller-supplied
+    // deadline, and checkIntegrationHealth() fans out via Promise.all, so an
+    // unbounded call here would extend the whole aggregate's latency.
+    const ctrl = new AbortController();
+    const timeout = setTimeout(() => ctrl.abort(), ROUTE53_AUTH_PROBE_TIMEOUT_MS);
+    try {
+      await getRoute53Client().send(new ListHostedZonesCommand({ MaxItems: 1 }), {
+        abortSignal: ctrl.signal,
+      });
+    } finally {
+      clearTimeout(timeout);
+    }
     status = 'ok';
   } catch (err) {
     status = isAwsAuthError(err) ? 'auth_failed' : 'unreachable';

lib/services/route53-record-key.ts

@@ -24,6 +24,13 @@ export interface NormalizedRecordSet {
 /**
  * Build the `route53_records.record_key` primary key / `recordId` URL
  * segment: `${zoneId}:${name}:${type}:${setIdentifier ?? ''}`.
+ *
+ * Normalizes `name` (lowercase) and `type` (uppercase) itself rather than
+ * trusting every caller to pre-normalize — every current call site happens
+ * to normalize before calling this, but that's a fragile invariant with no
+ * guard at the point where it actually matters: a mismatched-case call would
+ * silently produce a different `record_key` than the canonical one, splitting
+ * a single AWS record across two mirror rows.
  */
 export function buildRecordKey(input: {
   zoneId: string;
@@ -31,7 +38,7 @@ export function buildRecordKey(input: {
   type: string;
   setIdentifier?: string | null;
 }): string {
-  return `${input.zoneId}:${input.name}:${input.type}:${input.setIdentifier ?? ''}`;
+  return `${input.zoneId}:${input.name.toLowerCase()}:${input.type.toUpperCase()}:${input.setIdentifier ?? ''}`;
 }
 
 /**

lib/services/route53-sync-service.ts

@@ -279,12 +279,19 @@ export class Route53SyncService {
       );
     }
 
-    // Soft-delete zones no longer returned by AWS.
-    await postgresClient.query(
-      `UPDATE route53_zones SET is_deleted = true, deleted_at = NOW(), updated_at = NOW()
-       WHERE is_deleted = false AND id <> ALL($1)`,
-      [seenIds]
-    );
+    // Soft-delete zones no longer returned by AWS. Guard against an empty
+    // seenIds — `id <> ALL('{}')` is vacuously true for every row, so a
+    // successful-but-empty AWS response (transient API quirk, not "the
+    // account has zero zones") would otherwise soft-delete every previously
+    // synced zone in one shot. Same bug class already fixed in
+    // pax8-sync-service.ts.
+    if (seenIds.length > 0) {
+      await postgresClient.query(
+        `UPDATE route53_zones SET is_deleted = true, deleted_at = NOW(), updated_at = NOW()
+         WHERE is_deleted = false AND id <> ALL($1)`,
+        [seenIds]
+      );
+    }
 
     return seenIds.length;
   }
@@ -396,12 +403,17 @@ export class Route53SyncService {
       }
 
       // Soft-delete this zone's records no longer present in AWS. Never
-      // hard-delete — the history ledger references record_key.
-      await postgresClient.query(
-        `UPDATE route53_records SET is_deleted = true, deleted_at = NOW(), updated_at = NOW()
-         WHERE zone_id = $1 AND is_deleted = false AND record_key <> ALL($2)`,
-        [zoneId, seenKeys]
-      );
+      // hard-delete — the history ledger references record_key. Guard
+      // against an empty seenKeys for the same reason as syncZones() above —
+      // every real zone has at least apex NS/SOA records, so an empty page
+      // here is a transient AWS API quirk, not "this zone has zero records."
+      if (seenKeys.length > 0) {
+        await postgresClient.query(
+          `UPDATE route53_records SET is_deleted = true, deleted_at = NOW(), updated_at = NOW()
+           WHERE zone_id = $1 AND is_deleted = false AND record_key <> ALL($2)`,
+          [zoneId, seenKeys]
+        );
+      }
     }
 
     return totalUpserted;