feat: add site-scoped preflight REST endpoints | SITES-44686

[GeezerPelletier]

Summary

• Adds three new site-scoped REST endpoints for the Preflight feature:
  • POST /sites/:siteId/preflights — creates a Preflight record + AsyncJob, fires Mysticat analyze (always mode: suggest), returns 202 with Location header
  • GET /sites/:siteId/preflights — lists all preflights for a site (optional ?url filter)
  • GET /sites/:siteId/preflights/:preflightId — returns full preflight detail
• Adds src/dto/preflight.js with PreflightDto.toJSON (list view) and toDetailJSON (detail view)
• Wires preflightId UUID validation in src/index.js (alongside existing jobId validation)
• Routes registered in src/routes/index.js and capability requirements in src/routes/required-capabilities.js
• Removes internal /preflight/beta/* endpoints (replaced by the new site-scoped endpoints)
• Existing /preflight/jobs endpoint is not modified
• Adds ADR-003 — extracts the Preflight entity design into its own decision record (per retrospective review feedback on docs: ADR-002 preflight API REST redesign #2377); ADR-002 updated to reference it

Design decisions

• New endpoints are undocumented (not in the public OpenAPI spec) intentionally — they will remain that way until the feature is ready for GA
• Access control uses AccessControlUtil.fromContext(ctx) with hasAccess check against the resolved site
• Entitlement check via TierClient.createForSite → checkValidEntitlement('preflight') gates the create endpoint
• Cross-site isolation: getPreflightById returns 404 (not 403) when preflight.getSiteId() !== siteId to prevent site enumeration
• Mysticat is always called with mode: 'suggest' (no step parameter exposed)

Dependency on #2480

Two controller tests in this PR exercise the x-promise-token header forwarding behavior added by #2480. They depend on that PR landing first:

• returns 400 for CS site when x-promise-token header is missing and authentication is required
• passes x-promise-token header to retrievePageAuthentication for CS site

These will go red on CI until #2480 merges into main and this branch is updated.

Conflict resolution (2026-05-29 merge from main)

LLMO-5190 landed on main and removed workspaceId UUID validation in src/index.js. Conflict resolved by keeping the new preflightId validation and dropping workspaceId. The three new site-scoped routes were also added to test/routes/index.test.js's dynamic-routes expectation list.

Test plan

• 66 unit tests in test/controllers/preflight.test.js (2 of which depend on feat: resolve promise token from the header required to create a preflight job #2480)
• Full coverage of happy path and error paths for all three new endpoints
• Access denied (403), site not found (404), preflight not found (404), entitlement not enabled (403), invalid URL (400), internal errors (500)
• ESLint clean on all changed files

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[GeezerPelletier]

Note on CI failures: The failing checks in this PR are pre-existing failures on main and are unrelated to these changes. Verified locally — our diff against main is exactly the 8 files in this PR (preflight controller, DTO, routes, index, tests, and the two ADR docs). The PlgOnboardingController failures came in via 88edf952 on main, and the original ~51 failures (organizations, configurations, AI visibility, etc.) pre-date this branch entirely. All preflight tests pass cleanly.

[clotton]

Strong overall — the ADR is excellent, cross-site isolation is done right (404 not 403 in getPreflightById), createdBy is derived from the authenticated profile rather than client input, and mode: 'suggest' is correctly pinned server-side. One shippable bug and a few smaller concerns below.

🚨 Critical — ?url= filter never works in production

src/controllers/preflight.js:532 reads urlFilter from context.params?.url. context.params is populated by matchPath from path segments — the route GET /sites/:siteId/preflights has no :url segment, so params.url is always undefined. Query strings live at context.invocation.event.rawQueryString and need to be parsed explicitly (see feature-flags.js:40-49 and brands.js:129 for the pattern).

The existing test at preflight.test.js:1418 doesn't catch this because it injects params: { url } directly, bypassing the routing layer.

Suggested fix: add a getQueryParams(context) helper, pull url from there, and add a test that constructs context.invocation.event.rawQueryString = 'url=...'.

Concerns worth addressing

1. Mysticat error messages leak to API clients (line 515). The Mysticat response body (caught at 361-364 in the unchanged helper) can contain stack traces / internal IDs. Log the full error; return a generic upstream error to clients.
2. Silent rollback failures at lines 493, 512, 514 — .catch(() => {}) hides DB inconsistencies from ops. Use .catch((e) => log.warn(...)).
3. No timeout on Mysticat fetch + env.MYSTIQUE_API_BASE_URL not validated (line 499 / call site). If Mysticat hangs, this request hangs until Lambda timeout. If the env var is unset, fetch URL becomes "undefined/v1/preflight/analyze". Add an AbortController (~10-15s) and validate required env vars at controller construction.
4. Status enum inconsistency — AsyncJob.Status.IN_PROGRESS (line 469) vs raw string 'IN_PROGRESS' (line 487).
5. Silent findByPreviewURL error swallow at line 405. Behavior is correct (falls through to !siteByUrl), but a log.debug would give ops visibility if this starts firing.
6. Synchronous HEAD to customer URL in checkEnableAuthentication (unchanged helper) — pre-existing pattern, also in legacy createPreflightJob. Not a regression, but worth a follow-up to bound it.

Strengths

• ADR-003 is genuinely well-written — clearly states why a dedicated Preflight entity beats extending AsyncJob, and was pre-aligned with @ekdogan. This is what an ADR should look like.
• Authorization ordering (!site → !hasAccess → URL validation) is correct — never disclose data before access verification.
• preflightId UUID validation centralized in the router (index.js:356) follows the existing jobId/siteId pattern.
• Required capabilities registered for all three new routes with the right read/write semantics.
• 61 tests with broad coverage of error paths (access denied, not found, internal errors, entitlement gate).

Happy to discuss any of these — the URL-filter fix is the only one I'd consider a hard block.

[clotton]

🚨 context.params?.url is always undefined for this route.

context.params is populated by matchPath from path segments — GET /sites/:siteId/preflights has no :url segment, so query strings never land here. The test at preflight.test.js:1418 doesn't catch this because it injects params: { url } directly.

Query strings need to be parsed from context.invocation.event.rawQueryString — see feature-flags.js:40 or brands.js:129 for the established pattern.

Suggested fix:

function getQueryParams(context) {
  const raw = context.invocation?.event?.rawQueryString;
  if (!raw) return {};
  const out = {};
  raw.split('&').forEach((p) => {
    const [k, v] = p.split('=');
    if (k) out[decodeURIComponent(k)] = v !== undefined ? decodeURIComponent(v) : '';
  });
  return out;
}

// in getAllPreflights:
const urlFilter = getQueryParams(context).url;

And add a test that constructs a real rawQueryString so this can't regress.

[GeezerPelletier]

Fixed - getAllPreflights now parses from context.invocation.event.rawQueryString following the pattern from feature-flags.js and brands.js. The test has also been updated to construct a real rawQueryString with URL-encoded values so this can't regress.

[clotton]

mysticatError.message may include the response body from Mysticat (see line 363 in the unchanged helper), which could contain stack traces, internal IDs, or other internal info we don't want to leak to API clients. Suggest logging the full error and returning a generic upstream message:

log.error(`Mysticat analyze failed: ${mysticatError.message}`);
return preflightError('PREFLIGHT_UPSTREAM_ERROR', 'Upstream analyze service failed', 502);

[GeezerPelletier]

Fixed - the full Mysticat error is logged but the client response now returns the generic 'Upstream analyze service failed' message so internal details aren't leaked.

[clotton]

.catch(() => {}) silently swallows the rollback failure. If asyncJob.remove() fails here, the AsyncJob is orphaned and ops has no visibility. The TTL will eventually clean it up, but a .catch((e) => log.warn(\Failed to roll back AsyncJob ${asyncJob.getId()}: ${e.message}`))` would surface drift.

[GeezerPelletier]

Fixed - rollback now uses .catch((re) => log.warn(Failed to roll back AsyncJob : `))` so orphaned jobs surface in logs.

[clotton]

Same silent-rollback concern at lines 512 and 514 — if either preflight.save() or asyncJob.save() fails while persisting the FAILED state, the DB ends up inconsistent and we never log it. A .catch((e) => log.warn(...)) on each would help operability.

[GeezerPelletier]

Fixed - both preflight.save() and asyncJob.save() now have .catch((e) => log.warn(...)) so inconsistent DB state is surfaced in logs rather than swallowed.

[clotton]

Two things on this call:

1. No timeout on the fetch inside callMysticatAnalyze. If Mysticat hangs, this request hangs until Lambda's invocation timeout. Suggest adding an AbortController with ~10-15s in callMysticatAnalyze so caller can surface upstream timeouts cleanly.

2. env.MYSTIQUE_API_BASE_URL not validated. If unset, the URL becomes "undefined/v1/preflight/analyze" and fetch fails with a confusing error. The constructor already validates env is a non-empty object — recommend adding a specific check for MYSTIQUE_API_BASE_URL (and any other required env keys) at controller construction so misconfiguration fails loud at startup, not silently mid-request.

[GeezerPelletier]

Fixed both points:

1. Added AbortController with a 15s timeout in callMysticatAnalyze, cleared in a finally block so it doesn't leak.
2. MYSTIQUE_API_BASE_URL validation moved into createPreflight at call time (returns 500) rather than the constructor, so getAllPreflights and getPreflightById can still be instantiated without it - the misconfiguration fails loud when it matters.

[clotton]

Status string mismatch with line 469 — there it's AsyncJob.Status.IN_PROGRESS (typed enum), here it's the raw string 'IN_PROGRESS'. If Preflight has its own Status enum, use it for consistency. If not, worth adding one to spacecat-shared-data-access so we don't end up with typos like 'IN_PROGESS' later.

[GeezerPelletier]

Fixed - Preflight.create now uses AsyncJob.Status.IN_PROGRESS consistently throughout, matching line 469.

[clotton]

Silent error swallow — if findByPreviewURL throws (vs. returns null), the subsequent !siteByUrl check handles it correctly, so the behavior is fine. But adding a log.debug(\findByPreviewURL failed for ${previewBaseURL}: ${e.message}`)` would help ops spot if this starts firing unexpectedly.

[GeezerPelletier]

Fixed - added log.debug(findByPreviewURL failed for : `)` in the catch block so unexpected failures surface in logs.

[johobot]

Nit: new URL(url) is parsed twice here. Parse once:

const { protocol, hostname } = new URL(url);
const previewBaseURL = `${protocol}//${hostname}`;

[GeezerPelletier]

Fixed - destructuring on the first parse: const { protocol, hostname } = new URL(url); const previewBaseURL = \\\//\\\\.

[johobot]

If findByPreviewURL throws (e.g. a DB error), the exception is swallowed at debug level and execution falls through to the if (!siteByUrl || ...) check below, which returns 400 PREFLIGHT_INVALID_REQUEST — "URL does not belong to this site" — when the real cause is an infrastructure fault.

Suggest treating the throw as a 500:

} catch (e) {
  log.error(`findByPreviewURL failed for ${previewBaseURL}: ${e.message}`);
  return preflightError('PREFLIGHT_INTERNAL_ERROR', 'Internal error', 500);
}

[GeezerPelletier]

Good catch - fixed. A throw from findByPreviewURL now returns PREFLIGHT_INTERNAL_ERROR 500 rather than falling through to the !siteByUrl check and returning a misleading 400. Added a test for the throw case as well.

[johobot]

startedAt is set here, before the callMysticatAnalyze call below. ADR-003 defines it as "When processing began", but actual processing begins when Mysticat accepts the POST. If Mysticat ever queues requests (or is slow to respond), the recorded startedAt will be earlier than real processing. Worth moving this to after callMysticatAnalyze succeeds, or clarifying in ADR-003 that it means "when the request was dispatched".

[GeezerPelletier]

Fair point on the semantics. startedAt is set at record creation because the Preflight row must exist before we call Mysticat (so we can persist FAILED state if it throws). Moving it to post-call would require a second save on the happy path. Clarified the field description in ADR-003 to read: ''When the request was dispatched to the analyze service (set at record creation, before the upstream call completes)''.

[johobot]

Hand-rolled query-string parsing can be replaced with the built-in URLSearchParams:

const urlFilter = new URLSearchParams(context.invocation?.event?.rawQueryString ?? '').get('url') ?? undefined;

The current split('=') approach would split on every = in the key-value pair (edge case: bare = in an un-encoded value), and Object.fromEntries silently discards duplicate keys.

[GeezerPelletier]

Good suggestion - replaced with new URLSearchParams(rawQueryString ?? '').get('url') ?? undefined. Handles the duplicate-key and bare-= edge cases correctly.

[johobot]

This 404 response uses notFound(...) which returns { message } only. Every other error path in the three new endpoints goes through preflightError(...) and returns { errorCode, message }. The inconsistency will make MFE error-handling harder — suggest:

return preflightError('PREFLIGHT_NOT_FOUND', `Preflight with ID ${preflightId} not found`, 404);

[GeezerPelletier]

Fixed - now uses preflightError('PREFLIGHT_NOT_FOUND', ..., 404) for consistency. Updated both the not-found and siteId-mismatch paths, and tightened the tests to assert errorCode rather than just status.

[johobot]

The old createBetaPreflightJob suite had 5 auth-path tests (CS+promiseToken, AMS, CS_CW IMS fallback, SP+Secrets Manager, ErrorWithStatusCode propagation). createPreflight runs the same resolvePromiseToken → retrievePageAuthentication → authorizationHeader path, but none of those scenarios are covered here — only the no-auth happy path is tested. A regression in the auth branch would go undetected.

[GeezerPelletier]

Valid gap - we've added two createPreflight tests for the x-promise-token path (missing header -> 400, valid header -> 202 with token forwarded to retrievePageAuthentication). These will activate fully once PR #2480 (which makes x-promise-token the required mechanism) is merged and we rebase. The remaining cookie/IMS scenarios from the old flow will be superseded by that change.

[johobot]

Comments are non-blocking suggestions — nothing here should hold up the merge.