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>
AWS Route 53 rejects an unquoted TXT Value with:
'InvalidCharacterString (Value should be enclosed in quotation marks)'
— discovered during plan 24-07's live checkpoint (step 2, create) against
a real hosted zone. buildChangeBatch now wraps TXT values in escaped
double quotes, splitting into 255-character segments per RFC 1035's
character-string limit. A/AAAA/CNAME/MX/SRV values pass through
unchanged (only TXT uses the quoted-string wire format).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add route53-incremental and route53-full to the sync_type union
- Seed both schedules disabled (*/15 * * * * incremental, 0 1 * * * full)
- Dispatch branches gate on isRoute53Configured() only (D-10 — no
integration_settings check, unlike the pax8-daily exception)
- Both branches use dynamic import to keep the AWS SDK out of the
scheduler's eager module graph
Plan 24-04's isolated worktree didn't have plan 24-03's
route53-record-validation.ts available (parallel wave, no direct
dependency), so it carried a local copy of the identical AWS error
redaction logic — flagged in its own SUMMARY for consolidation once
24-03 merged. Both plans are now merged; importing the shared
implementation instead of keeping two copies in sync.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- checkRoute53(): config gate + ListHostedZonesCommand auth probe, mirrors
checkDattoRmm()'s custom-body shape (key: 'route53', category: 'network')
- Auth-probe errors classified via isAwsAuthError (InvalidClientTokenId,
SignatureDoesNotMatch, AccessDenied, UnrecognizedClientException, 401/403)
and redacted through a local sanitizeAwsError before reaching IntegrationHealth.error
- D-12: queries route53_zones (is_deleted=false, capped at 50 by name),
feeds checkAllZoneDelegations(); mismatches downgrade status to a new
'degraded' HealthStatus member; lookup failures reported separately via
nsDelegationErrors, never counted as mismatches
- Whole delegation step wrapped in try/catch so a Postgres failure or
blocked resolver can never abort checkIntegrationHealth()'s Promise.all (T-24-16)
- summarize() updated so 'degraded' counts toward failed/hasIssues instead
of falling through uncounted (Rule 1 fix)
- No changes to applyDisableOverlay() — route53 covered by the existing
generic by-key overlay (D-10)
- 6 unit tests exercising the pure drift-classification wiring in Route53SyncService
- Confirms whole-recordset before/after payloads, not per-field deltas
- Confirms CRUD-originated changes still get tagged sync_detected_drift (sync cannot distinguish actor)
- Full suite green aside from 2 pre-existing itglue-search.test.ts failures logged in deferred-items.md (unrelated to this plan)
- fullSync/incrementalSync + getRoute53SyncService() singleton
- Paginated ListHostedZonesCommand + GetHostedZoneCommand (delegation set for D-12)
- Paginated ListResourceRecordSetsCommand per live zone
- Soft-delete reconciliation for zones and records (never hard-delete)
- sync_history bookkeeping with entity_type='route53', literal full/incremental sync_type
- Drift detection wired via buildDriftHistoryRows, writing sync_detected_drift history rows
- No integration_settings gating anywhere (D-10)
- createPendingAuditLog/markAuditCommitted/markAuditFailed implement the pending -> committed/failed lifecycle (D-07, SC-3); markAuditFailed always sanitizes via sanitizeAwsError
- insertPulseCrudHistory writes 'pulse_crud' history rows, documented as callable only after a committed write
- upsertMirrorRecord/softDeleteMirrorRecord/loadMirrorRecord manage the route53_records mirror; mirror writes are best-effort and soft-delete only (D-08), audit/history writes are not best-effort
- log same pre-existing itglue-search.test.ts failures (unrelated, out of scope) in deferred-items.md
- buildRecordKey, normalizeRecordSet, recordSetsEqual, classifyDrift, toHistoryPayload
- Pure, dependency-free module (no pg, no AWS client construction)
- 16 unit tests covering every behavior bullet from the plan
- lib/services/route53-factory.ts: isRoute53Configured() / getRoute53Client()
/ resetRoute53Client(), following the veeam-factory.ts singleton shape
- No explicit credentials option passed to Route53Client — relies on the AWS
SDK's default credential chain reading AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY
from process.env, exactly how BWS injects them at the container entrypoint
- CLAUDE.md: document the AWS_* env-prefix exception in the integration table
- All 7 route53-factory.test.ts assertions pass; npx tsc --noEmit clean
Tier 3 is the only tier automatic (webhook-triggered) grouping ever
reaches, since grouping runs before message parsing. It was scoped to
reports.requester_contact_id, so the same campaign reported by
different employees at the same company never consolidated into one
campaign — each report's evidence/blast-radius view silently
under-reported the campaign's true recipients.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add mimecast_tenants routing branch to stageQueries and company_id to
report fixtures
- New test: enabled mimecast_tenants row -> getBlastRadius called with
{ client, cacheScope: companyId } and the tenant SQL is issued
- New parameterized test: null companyId and companyId-with-no-enabled-row
-> getBlastRadius called with no second argument (global fallback)
- Thread company_id through reports query and CampaignReportSummary
- Mirror the route's Bug 2 (D-05) tenant-resolution block: query
mimecast_tenants for an enabled row, build a tenant-scoped client via
getMimecastClientForTenant, and pass { client, cacheScope } to
getBlastRadius() when one exists
- Preserve global env fallback unchanged when no companyId or no
enabled tenant row is present
- Updated the getHeldMessages window test to assert no `recipient` key and
added an explicit no-`to` assertion for searchDeliveredMessages
- Added a new test driving searchDeliveredMessages with 3 distinct `to`
addresses, asserting all appear in perRecipient and count toward
matched/delivered
- searchDeliveredMessages now called with from+subject+start+end only
(no `to`) so it returns every delivered/rejected message matching the
campaign across all recipients, not just the reporter's mailbox
- getHeldMessages now called with start+end only (no `recipient`) —
domainsMatch() post-filter is the sole scoping mechanism for held rows
- Updated inline comments to document the tenant-wide fan-out and the
per-recipient merge behavior it now produces
- Extend ScheduleConfig.sync_type union with 'mimecast-sync'
- Add mimecast-sync branch calling runMimecastIncrementalSync() behind
isMimecastConfigured(), mirroring the engagement/zoom configured-gate pattern
- Add qbo branch calling getQboSyncService().incrementalSync('scheduled')
behind an integration_settings disabled check, mirroring the pax8-daily
disable-check pattern
- Both branches previously fell through to the generic Autotask fullSync()
catch-all, which also contended for the SyncService singleton mutex
Bundles several in-progress efforts that were sitting uncommitted:
- User queue-preferences (migration 087, API route, popover component)
- QBO invoice soft-delete (migration 088) and AR diagnostics route
- Dashboard/mobile engagement route and page adjustments
- Docker Compose log-rotation config
- One-off ticket/RMM investigation scripts (scripts/)
- Planning docs: phase verification/pattern notes, mobile shell design spec
- .gitignore: exclude local scratch financial/inventory data and Claude Code
worktree/local-settings runtime state (never meant for version control)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6RuWdiUiXrPK6FLBHjtpY
- blast-radius: getHeldMessages called with same start/end window as
searchDeliveredMessages
- blast-radius: unrelated-sender held row excluded from held/matched and
perRecipient; matching-sender held row still counts and overrides
- client: getHeldMessages threads start/end into POST body data[0] when
provided, omits them when not
- getHeldMessages() accepts optional start/end, threaded into data[0] as
siblings of admin/searchBy (backward compatible when omitted; 403
fallback body inherits them automatically via the existing spread)
- getBlastRadius() passes the same startStr/endStr window already
computed for searchDeliveredMessages into getHeldMessages()
- Added domainsMatch() sender-relevance guard: held rows whose sender
domain doesn't match input.sender (exact-or-proper-subdomain) are
filtered out before counting/merging, so unrelated same-window holds
never inflate held/matched or override a delivered recipient
- parseAndStoreMessage (Defect 3): short-circuit with
{ stored: false, reason: 'already-parsed' } when a messages row already
exists for the report, before any Autotask attachment fetch
- webhook-service (Defect 2): new retryPhishingParseOnUpdate wired into
ticket.update fire-and-forget path; retries the missing-EML parse for a
flagged, unparsed, auto_parse-gated report — no new cron/polling, reuses
existing update traffic, safe to fire repeatedly thanks to the new
idempotency guard
- Adjust eml-service test mock default so the new leading existence-check
query doesn't short-circuit existing happy-path tests; add new test for
the already-parsed short-circuit
23-06-REVIEW.md found two real defects in the just-merged idempotency fix:
- CR-01: autoPostAcknowledgment's audit payload omitted actionId, which the
campaign-detail API requires to derive completedAt — every auto-posted
acknowledge_user row rendered a null completion date in the Action Area UI.
- CR-02: the manual approve/remediate path had no server-side guard against
re-approving acknowledge_user for a campaign that already got auto-posted —
only a client-side UI check prevented the exact duplicate-note bug 23-06
was chartered to close, reachable via a direct API call.
Fixes both: capture RETURNING id from the insert and include it in the audit
payload; add an existence check in approveRemediationActions that rejects
acknowledge_user when already posted for the campaign.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6RuWdiUiXrPK6FLBHjtpY
Replace the unguarded generateAndPostAcknowledgment(campaignId) direct
call in runGatedPhishingStages' auto_report branch with the idempotent
autoPostAcknowledgment(campaignId, 'system:auto_report'). Closes CR-01
(23-REVIEW.md) / Truth #18 (23-VERIFICATION.md): a repeat ticket-create
webhook joining an already-acknowledged USER_AWARENESS campaign now
finds the persisted remediation_actions row and skips the re-post
instead of re-sending the customer-visible thank-you note on every
additional report.
- New autoPostAcknowledgment(campaignId, actor) mirrors the VERIFIED
remediateApprovedActions shape: campaign row lock, idempotency check
against an existing acknowledge_user remediation_actions row, insert +
audit inside one transaction, note post after commit (non-fatal on
failure)
- Closes CR-01/WR-01: gives the auto_report webhook path a persisted
record so a repeat ticket-create webhook joining an already-acked
campaign does not re-insert/re-audit/re-post
- Adds Test A/B/C in remediation-service.test.ts covering first-pass
insert, idempotent skip, and non-fatal note-post failure
- triggerPhishingDetection now captures groupReportIntoCampaign's result and,
when a campaignId exists, calls new runGatedPhishingStages
- runGatedPhishingStages reads the per-company automation gate and
conditionally runs parseAndStoreMessage, classifyCampaign, and (only for
USER_AWARENESS verdicts) generateAndPostAcknowledgment
- each stage isolated in its own try/catch (T-23-09); detection + grouping
remain unconditional (D-07); auto_report never posts any other action
(D-04, T-23-08)
- remediateApprovedActions now captures the transaction's RemediateResult,
then post-commit checks whether an approved acknowledge_user row was
transitioned this pass (alreadyCompleted === false) and, if so, calls
generateAndPostAcknowledgment(campaignId) exactly once
- Call happens outside the DB transaction (network I/O hazard) and is
wrapped in its own try/catch that logs and swallows failures -- the DB
transition has already committed
- Every other action type (block_sender, purge_message, warn_user,
reset_password, isolate_endpoint, disable_forwarding_rule, quarantine)
remains a simulated status-only transition, unchanged
- Updated top-of-file D-01 doc comment to record the narrow D-04 carve-out
- Tests: acknowledge_user IS posted once when remediated, NOT called for
block_sender/warn_user-only remediation, NOT called on idempotent re-run
of an already-completed acknowledge_user row, and a post rejection does
not propagate out of remediateApprovedActions
- New generateAndPostAcknowledgment(campaignId) posts a short, appreciative
thank-you note to every ticket linked to a campaign, using noteType 18
(Client Portal Note, verified live against tenant's TicketNotes field
metadata) so the note is customer-visible; publish stays 1 unchanged
- Body is a fixed template with zero evidence/URL/classification
interpolation (T-23-01) -- not the evidence-dump formatTriageNote() template
- Mirrors generateAndPostTriageNote's per-ticket try/catch-in-loop error
isolation and { noteText, tickets } return shape
- Tests: noteType 18 + publish 1 payload assertion, per-ticket failure
isolation, and zero-linked-reports case
- Add USER_AWARENESS to the Verdict union in campaign-classifier.ts
- mapVerdictToActions('USER_AWARENESS') returns ['acknowledge_user']; not added to DESTRUCTIVE_ACTIONS so requires_approval computes false
- classifyCampaign's simulation branch now assigns verdict = 'USER_AWARENESS' directly instead of falling through to evaluateSpamVsUnwanted
- deriveDefaultParams('acknowledge_user') returns {} (no operator-editable params)
- Widen TriageNoteEvidence.verdict to admit 'USER_AWARENESS' (pure type widen, no formatting change)
- Tests: classifier simulation fixtures now assert USER_AWARENESS/acknowledge_user/requiresApproval=false; new mapVerdictToActions/computeRequiresApproval/deriveDefaultParams cases
Covers the already-implemented per-tenant factory: returns a MimecastClient
instance, builds a new independent instance per call (never the cached
global), doesn't affect getMimecastClient()'s singleton, and defaults
base_url when omitted. Uses fake credentials only.
- getBlastRadius(input, options?) accepts an optional injected MimecastClient
and cacheScope; an injected client bypasses the global isMimecastConfigured()
gate since it carries its own credentials
- cache key namespaced by cacheScope to prevent cross-tenant collisions
- deliveredResult.error (previously swallowed) now rethrown so the outer
catch converts it to status: unavailable / reason: lookup_failed --
defense-in-depth against Bug 1 (future end-date rejected by Mimecast)
- test mock hygiene: getMimecastClientMock now cleared in beforeEach
- Fake tenant client via options.client bypasses getMimecastClient
- Injected tenant client runs fan-out even when global env unconfigured
- searchDeliveredMessages error field now expected to degrade to unavailable/lookup_failed
- Discriminated union TimelineEntry with report/classification/audit variants
- Ascending sort by createdAt with stable report<classification<audit tie-break
- Covers ascending chronological sort across reports/classifications/audit
- Asserts discriminant kind + source fields per variant, plus stable tie-break order
- Pure switch over no_action/warn_user/block_sender/purge_message/
reset_password/isolate_endpoint/disable_forwarding_rule
- Unknown/future action types fall through to {} rather than throwing
- Pure lookup: reports row for a ticket id -> found/reportId/campaignId/ticketNumber
- Parameterized query only (WHERE ticket_id = $1), no requirePermission/NextResponse
