Skip to content

feat(serenity): async Semrush-market AI-prompt provisioning (#3194) - #3252

Open
dzehnder wants to merge 13 commits into
mainfrom
feat/LLMO-semrush-market-async-producer
Open

feat(serenity): async Semrush-market AI-prompt provisioning (#3194)#3252
dzehnder wants to merge 13 commits into
mainfrom
feat/LLMO-semrush-market-async-producer

Conversation

@dzehnder

@dzehnder dzehnder commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

1. Abstract

The spacecat-api-service half of async Semrush-market AI-prompt provisioning: onboarding a site/market with "include Semrush prompts" now generates real monitoring prompts from the Semrush topic catalogue (as seeds, via a stateless synchronous DRS call) and writes the validated prompts to the market, instead of tracking the raw catalogue strings verbatim.

2. Reasoning

The prior path (generateAndAttachPrompts) copied the Semrush catalogue's example strings verbatim into the market and ran synchronously inside the request, sharing one ~12s write budget across every market in a batch — which cannot fit an LLM generation + regeneration loop, and yields low-quality, wrong-language, or empty prompt sets. Generation must move to an async worker and be produced by DRS (which owns the LLM, the language gate, and the quality/guardrail filtering). This is the api-service half of the cross-repo feature specified in serenity-docs#443. It also builds directly on the merged SEC-5 IDOR fix (#3222) so any new AsyncJob-returning endpoint stays scoped to the caller.

3. High-level overview of the changes

Token-bearing write executor, hardened first (it gates any token-bearing write job type):

  • Fail-closed promise-pair binding — a Semrush-write job's enqueue is rejected before a token is minted if the Semrush pair can't resolve, rather than silently defaulting to the IMS pair.
  • One typed promise-token contract — the bare-string header shape is rejected at both enqueue and the worker exchange (it previously stranded jobs in the exchange → DLQ).
  • An atomic per-job lease as a security control against promise-token replay + double-write to Semrush under SQS at-least-once delivery. The AsyncJob Active-Record save() has no conditional write, so the lease is a conditional PostgREST compare-and-set on async_jobs; exactly one concurrent delivery wins, a lost claim drops the duplicate, a claim-query error fails closed.
  • The worker now dispatches on the record's stored jobType (not the SQS message's type) and drops a message whose type contradicts it.
  • The new job type exchanges its write-scoped Semrush token inside the handler, after DRS returns — so generation quality is not raced against a 5-minute token, and no token crosses the DRS boundary.

Behaviour delta at the three onboarding entry points (new brand/site, new market, activate), gated by the default-off SERENITY_ASYNC_PROMPT_GEN:

  • Flag off (today): unchanged — synchronous verbatim-catalogue behaviour. Merge is a no-op in production.
  • Flag on: the market is created synchronously without generation, a single shared producer enqueues one generation job (deterministic (brand, geoTargetId, language) idempotency key — a live job is reused, a stale one recycled), and the create/activate response gains an additive promptGeneration: { jobId, status: "provisioning" } handle. The worker synchronously invokes DRS, persists the returned batch before the write phase (and resumes from it on retry — DRS is never re-invoked), writes the prompts tagged source=semrush + origin=ai, publishes, and records the outcome. A held / gate_error / zero-prompt outcome fails terminally and never publishes a market; the parent brand/site stays valid and recoverable.

New endpoints (both brand-scoped, under /serenity/markets/generation/jobs/{jobId}):

  • GET .../jobs/{jobId} — poll job status/result through the shared loadJobScopedToCaller primitive; returns a strict token-safe DTO (never a promise token). Status is IN_PROGRESS|COMPLETED|FAILED; the UI's five states map via error.code (NEEDS_REAUTH→authorization-required, PROMPT_GENERATION_HELD→held, other→failed) and error.needsReauth, with COMPLETED→live (result.verdict==="ship"). Held/gate_error/zero-prompt are FAILED with a distinct code, never a COMPLETED+held.
  • POST .../jobs/{jobId}/reauth — recover a job whose Semrush token IMS rejected. Strict, fail-closed identity: the caller must be the original enqueuer by one stable user_id claim; a replacement token is accepted only on the explicit Semrush pair; the record is updated atomically and the same job id re-enqueued.

4. Required information

5. Affected / used mysticat-workspace projects

6. Additional information outside the code

  • DRS contract locked to the shared fixture. test/fixtures/semrush_market_generation_contract.json is the canonical cross-repo fixture mirrored from DRS [Serenity][Prompt Strategy] Async Semrush-market AI prompt provisioning — AsyncJob producer, token lifecycle, Semrush writes, polling & reauth #3194. toDrsRequestPayload maps the worker's semantic request onto the exact snake_case wire payload (site_id, brand, brand_aliases, base_url, market_country, language_code, audience, num_prompts, model, catalogue_seeds[{topic,volume,example_prompts}], catalogue_status, metadata.imsOrgId); a contract test asserts the mapping reproduces the fixture request exactly and parses the fixture response (v1 empty category tolerated). Open reconciliation items flagged to drs-package: whether market_country wants the geo code (what the producer supplies) or the display name (the fixture shows "France"); whether audience is required (the producer does not populate it today); subpath is not in the contract and is not sent; model defaults to gpt-5-nano; catalogue_status defaults to populated.
  • The atomic lease is a conditional PostgREST update against async_jobs (AsyncJob exposes no conditional save() at the model layer). It is unit-covered via a stubbed PostgREST client (win / lose / fail-closed branches); DB-level (postgres IT) validation of the compare-and-set is a deferred follow-up.
  • Verified against the merge-base that the prereq defects still hold as behaviours (e.g. resolvePromisePair returns undefined on an absent x-promise-audience header) and the fixes are anchored on tests, not line numbers.
  • spacecat-infrastructure#780 contract alignment (reconciled with infra PR fix: org creation in onboard command #786):
    • Enqueues onto a DEDICATED queue SERENITY_MARKET_JOBS_QUEUE_URL (not the shared classify/bulk-tags runner queue); the same serenity-job-runner Lambda consumes both via two ESMs. The market DLQ is deliberately not auto-redriven — recovery is the lease-aware runbook.
    • A SECOND fail-closed gate beyond the feature flag: the producer reads the infra SSM param /spacecat/serenity-market-worker/consumer-ready and enqueues only when it is exactly "true" (cached 60s). SERENITY_ASYNC_PROMPT_GEN = "feature wired on"; the SSM flag = "a live consumer + DRS Lambda exist in this env". Both must hold.
    • The worker emits DRSInvokeDurationMs (Milliseconds) and DRSInvokeFailure (Count) in namespace SpacecatSerenityMarketWorker, both {Environment}-only (infra's alarms match dimensions={Environment=<env>}). DRSInvokeFailure is the PAGING metric and fires ONLY on a genuine failure (transport/invoke error or terminal gate_error) — never on held (a fail-open outcome) or ship, so a normal held market never pages on-call. The failure reason lives in the log.
    • Timeout nesting matches infra's assertion visibility 960s > lease 930s > worker 900s > (DRS 300s + writes 120s): the lease TTL is 930s and the DRS invoke carries a 300s client-side abort (a timeout surfaces as a retryable job error).

7. Test plan

  • No live end-to-end run was performed this session — the async path depends on infra#780 (queue + cross-account DRS invoke) and a deployed DRS generation Lambda, neither available here; the DRS boundary is exercised through an injected-invoker seam.
  • eph/dev (once infra#780 + DRS fix(akamai): recover fetcher key on reconciled deploy status #3193/[Serenity][Prompt Strategy] Async Semrush-market AI prompt provisioning — AsyncJob producer, token lifecycle, Semrush writes, polling & reauth #3194 deploy): set SERENITY_ASYNC_PROMPT_GEN=true and DRS_PROMPT_GENERATION_FUNCTION for the api-service worker; onboard a market with generatePrompts:true; confirm the create response carries promptGeneration.jobId, poll GET .../serenity/markets/generation/jobs/{jobId} to COMPLETED, and confirm the market publishes generated (not verbatim) prompts tagged source=semrush/origin=ai.
  • Reauth: force a NEEDS_REAUTH (expired promise token), confirm the job status surfaces error.needsReauth, then POST .../reauth with x-promise-audience: semrush and confirm the same job id re-runs to completion; confirm a different IMS user is rejected 403.
  • Rollback check: with the flag unset, confirm the three entry points behave exactly as before (synchronous, no promptGeneration field).

8. Deployment & merge order

Ordered sequence: this PR can merge independently (flag default-off, merge is a production no-op). Enable SERENITY_ASYNC_PROMPT_GEN only after DRS (#3193/#3194) and infra (#780) are deployed to the target environment; roll the flag dev → stage → prod; elmo#3071 ships alongside for the UI.

Closes #3194

🤖 Generated with Claude Code

Change Management

cm-assessment: v1
changeType: standard
impact: unnoticeable
risk: minor
scope: multi-repo
relatedPRs: ["adobe-rnd/llmo-data-retrieval-service#3193", "adobe-rnd/llmo-data-retrieval-service#3194", "adobe/spacecat-infrastructure#780", "adobe/project-elmo-ui#3071"]
rationale: "Adds async Semrush-market prompt generation (shared producer, token/lease hardening, synchronous DRS invoke, polling + reauth) behind a default-off flag SERENITY_ASYNC_PROMPT_GEN; with the flag off the three onboarding entry points keep their exact synchronous behavior, so merge is a no-op in production. The token/lease work is defensive (fail-closed Semrush-pair binding, typed-token contract, atomic anti-replay lease, dispatch-on-stored-jobType); the SEC-5 IDOR fix is the already-merged #3222. A held/failed generation never publishes a market and is retryable/reauthable — recoverable, not destructive."
recommendations: "Enable the flag dev->stage->prod only after infra#780 (queue/DLQ + cross-account DRS invoke IAM) and elmo#3071 (202/polling UI) deploy; pin a shared DRS request/response fixture with DRS #3193/#3194 before enabling; add IT (postgres) coverage for the lease compare-and-set."
backout: "Set SERENITY_ASYNC_PROMPT_GEN unset/false to fully disable with no code change, or redeploy the previous release."

dzehnder and others added 6 commits September 9, 2026 17:10
Generate real monitoring prompts from Semrush topic seeds via a stateless
synchronous DRS invoke, instead of tracking the raw catalogue strings, and
write the validated prompts to the Semrush market. api-service owns the
request contract, AsyncJob + promise-token lifecycle, Semrush writes,
publication, polling and reauth.

Harden the token-bearing write executor first (gates any token-bearing write
job): fail-closed Semrush-pair binding (Gap 1), one typed token contract
(Gap 2), an atomic per-job lease as an anti-replay/double-write security
control (Gap 4), dispatch on the stored jobType with mismatch rejection, and
deferred token exchange (exchange the write token after DRS returns). The
existing serenity-classify-prompts consumer runs on the new contract with
regression coverage. Built on the merged SEC-5 IDOR fix (#3222): the new
polling endpoint reuses loadJobScopedToCaller.

One shared producer for all three entry points (new brand/site, new market,
activate) with a deterministic (brand, geoTargetId, language) idempotency key.
The worker persists the DRS batch before writes and resumes from it — never
re-invoking DRS on retry — writes source=semrush + origin=ai tags (category
deferred to serenity-docs#44), publishes, and records the outcome. A held or
failed generation never publishes a market.

Entry-point wiring is flag-gated (SERENITY_ASYNC_PROMPT_GEN, default off) so
merge is a no-op until infra#780 (queue/DLQ, cross-account invoke) and
elmo#3071 (polling UI) are ready; when on, the response carries an additive
promptGeneration handle to poll.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fold the spacecat-infrastructure#780 contract into the same feature:

- Dedicated market-jobs queue: the producer + reauth enqueue to
  SERENITY_MARKET_JOBS_QUEUE_URL (a new queueUrl option on createAndEnqueueJob),
  separate from the shared classify/bulk-tags runner queue; its DLQ is not
  auto-redriven (lease-aware runbook recovery).
- Fail-closed consumer-readiness gate: the producer reads the infra SSM param
  /spacecat/serenity-market-worker/consumer-ready and refuses to enqueue unless
  it is exactly "true" (cached 60s). This is separate from SERENITY_ASYNC_PROMPT_GEN
  ("feature wired on") — both must hold to enqueue. Adds @aws-sdk/client-ssm.
- Worker metrics the infra alarms depend on: DRSInvokeFailure (Count) and
  DRSInvokeDurationMs (Milliseconds) in namespace SpacecatSerenityMarketWorker,
  both carrying exactly the EMF Environment dimension (the failure reason is
  logged, not dimensioned, so the two metrics share one dimension set).
- Timeout nesting: lease TTL raised to 930s (between the 900s worker timeout and
  the 960s SQS visibility timeout) and a 300s client-side abort on the DRS invoke
  so it can't consume the whole worker budget.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restore the Reason dimension (invoke | verdict:<v>) on DRSInvokeFailure so the
infra#780 alarm's SUM(SEARCH(...)) across Reason values works and ops keeps the
per-reason failure breakdown. Both metrics still carry the EMF Environment
dimension: DRSInvokeDurationMs is {Environment}, DRSInvokeFailure is
{Environment, Reason} — neither is dimensionless, so the alarms match on
Environment=<env>. Pin both dimension sets in the unit test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Align with the infra#780 deployed convention (PR #786): all four custom-metric
alarms match dimensions={Environment=<env>}, so DRSInvokeFailure must carry
exactly {Environment} — a {Environment,Reason} metric would not be read by a
plain-Environment alarm. The failure reason stays in the log, not a dimension.
Both DRSInvoke* metrics now share the Environment-only dimension set.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ic dim

Two contract-locking changes:
- Restore the Reason dimension on DRSInvokeFailure ({Environment, Reason}) to
  match infra#780's deployed SUM(SEARCH({...,Environment,Reason})) failure alarm,
  which sums across Reason values (per-reason breakdown preserved). Locking here.
- Give terminal generation outcomes distinct public error codes so the polling
  DTO's error.code lets the UI (elmo#3071) render all five states:
  PROMPT_GENERATION_HELD (held → UI "held"), PROMPT_GENERATION_GATE_ERROR and
  PROMPT_GENERATION_EMPTY and DRS_GENERATION_TERMINAL (→ UI "failed"); NEEDS_REAUTH
  stays flagged via error.needsReauth. COMPLETED always means verdict ship (UI "live").

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Team-lead arbitration (final, frozen): revert DRSInvokeFailure to the
{Environment}-only dimension set matching infra#780's plain-metric alarms, and —
the correctness fix — increment the PAGING failure metric ONLY on a genuine
failure (a transport/invoke error, or a terminal gate_error), never on `held`
or `ship`. `held` is fail-open (a successful invoke whose substance layer
legitimately held prompts); counting it would page on-call for every normal
held market. The per-failure reason stays in the structured log. Terminal error
codes (HELD/GATE_ERROR/EMPTY/TERMINAL) for the UI are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

This PR will trigger a minor release when merged.

@dzehnder
dzehnder requested a review from MysticatBot September 9, 2026 16:03
…fixture

Mirror the pinned cross-repo contract fixture (DRS #3194,
test/fixtures/semrush_market_generation_contract.json) and map the worker's
semantic request onto the EXACT snake_case wire payload DRS consumes via a new
toDrsRequestPayload: site_id, brand, brand_aliases, base_url, market_country,
language_code, audience, num_prompts, model, catalogue_seeds[{topic,volume,
example_prompts}], catalogue_status, metadata.imsOrgId (imsOrgId stays camelCase
inside metadata). model defaults to gpt-5-nano (env DRS_PROMPT_GENERATION_MODEL);
catalogue_status defaults to "populated"; subpath is not part of the contract and
is not sent. Response parsing already reads ship_summary.verdict/error_category
and tolerates the v1 empty `category`. A contract test asserts the mapping
reproduces the fixture request exactly and parses the fixture response.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review — PR #3252

Verdict: REQUEST_CHANGES | Complexity: HIGH | Spec: serenity-docs#443

Adds async Semrush-market AI-prompt provisioning behind a default-off SERENITY_ASYNC_PROMPT_GEN flag. Introduces a three-phase checkpoint worker handler (generate via DRS Lambda, write to Semrush, publish), deferred token exchange, deterministic job reservation via SHA-256 UUID, SSM-based readiness gate, and atomic per-job lease via PostgREST CAS.

The security posture and worker-level code quality are strong. The deferred-exchange pattern, fail-closed pair binding, typed token assertion, atomic lease, dispatch-on-stored-jobType integrity check, and token-safe DTO projection are well-designed. Eight items need attention before merge.


Must-Fix Index

# Finding Location
1 createMarket async enqueue not fault-isolated src/controllers/serenity.js
2 Reauth TOCTOU: concurrent requests can leak promise tokens src/controllers/serenity.js
3 siteId: undefined silently bypasses SEC-5 site-level ownership src/controllers/brands.js, serenity.js
4 clearJobLease retryable-error path silently swallows save failure src/serenity-prompt-classification/index.js
5 brands.js async generation path has zero test coverage src/controllers/brands.js
6 activate/createMarket controller-level async paths untested src/controllers/serenity.js
7 Deferred-exchange NeedsReauthError propagation untested src/serenity-prompt-classification/index.js
8 :jobId param not classified in FACS_NON_RESOURCE_PARAMS src/routes/facs-capabilities.js

1. createMarket async enqueue not fault-isolated

Severity: Important
File: src/controllers/serenity.js (createMarket)

createBrandForOrg and activate wrap maybeEnqueueMarketGeneration in try/catch so an enqueue failure is non-fatal (the market/brand still creates successfully). createMarket does not. An SQS send failure or transient PostgREST error during enqueue returns 500 to the caller even though the market was successfully created.

Fix: Wrap the maybeEnqueueMarketGeneration call in createMarket in the same try/catch + log.warn pattern used by the other two entry points.


2. Reauth TOCTOU: concurrent requests can leak promise tokens

Severity: Important
File: src/controllers/serenity.js (reauthSemrushMarketGenerationJob)

The reauth handler performs a non-atomic sequence: (1) load the job, (2) check status === FAILED && error.code === NEEDS_REAUTH, (3) mint a new promise token, (4) set status to IN_PROGRESS, (5) job.save(). Between steps 2 and 5, a concurrent reauth request for the same job can pass the same status check and perform a second token mint + save. The second request wins (last-writer-wins) and the first minted token is leaked (never exchanged, never invalidated).

The blast radius is narrow (same user, same job, two tightly-timed requests), but this breaks the "every minted token is tracked and invalidated" invariant the rest of the PR enforces.

Fix: Apply the same PostgREST CAS pattern used by claimJobLease (conditional UPDATE on status=FAILED AND error->>'code'='NEEDS_REAUTH') so only one concurrent reauth wins the row.


3. siteId: undefined silently bypasses SEC-5 site-level ownership

Severity: Important
Files: src/controllers/brands.js, src/controllers/serenity.js

When a brand is created without a linked site, or when createMarket is invoked without a siteId, the producer stores siteId: undefined in job metadata. In the polling endpoint's loadJobScopedToCaller, when resolveOwnerSiteId yields undefined, the hasText(siteId) check fails and the entire site-level ownership check is skipped. The job is then authorized solely by the jobType allowlist and the brand-scoping check.

The brand-scoping check provides a secondary barrier (this is not an open IDOR), but the SEC-5 site-level ownership primitive -- documented as the primary scoping mechanism -- is silently bypassed for all siteless jobs.

Fix: Either (a) require that siteId is always populated on generation jobs (resolve from the brand's linked site if not supplied), or (b) add an explicit comment + test acknowledging that brand-scoping is the sole ownership control for siteless jobs.


4. clearJobLease retryable-error path silently swallows save failure

Severity: Important
File: src/serenity-prompt-classification/index.js

On the retryable-error path, clearJobLease(job) is followed by job.save().catch(() => {}). If the save fails, the lease stays on the record (cleared only in memory), there is no log line for operator visibility, and the job is stuck until the lease TTL (~930s) expires. The rethrow triggers SQS redelivery, but the new delivery cannot claim the lease until TTL expiry.

Fix: Add a log.warn inside the .catch() so operators have visibility into lease-clear failures.


5. brands.js async generation path has zero test coverage

Severity: Important
File: src/controllers/brands.js

createBrandForOrg gains ~35 lines of new production logic: an asyncGenBrand flag, suppression of the synchronous generateTopics path, a maybeEnqueueMarketGeneration call, a try/catch swallow, and a promptGeneration annotation on the 201 response body. No test in the diff exercises this path.

This is a production entry point that mints a promise token, creates an async job, mutates the API response body, and suppresses the existing synchronous flow. Any regression here silently leaves new brands without generated prompts.

Fix: Add tests covering: (a) async path enqueues and annotates the 201 body, (b) failure is non-fatal (201 still returns), (c) flag-off preserves synchronous behavior.


6. activate/createMarket controller-level async paths untested

Severity: Important
File: src/controllers/serenity.js

Both activate and createMarket gain async generation branches that construct producerParams, call maybeEnqueueMarketGeneration, and annotate the response body. The new test file serenity-market-generation.test.js covers only the polling and reauth endpoints. The underlying maybeEnqueueMarketGeneration is unit-tested, but the controller-level wiring (param construction, 201-vs-409 filtering in the activate loop, response body mutation) is not.

Fix: Add controller-level tests for the async paths in createMarket and activate.


7. Deferred-exchange NeedsReauthError propagation untested

Severity: Important
File: src/serenity-prompt-classification/index.js

The deferred-exchange path skips the runner's exchange block. The handler calls exchangeAndPersistPromiseToken internally, which can throw NeedsReauthError. This error falls into the worker's generic catch block, which must set the correct NEEDS_REAUTH error code on the job record for the reauth UI to work. No test verifies this propagation path.

If the generic catch does not preserve the NEEDS_REAUTH code, the polling DTO shows needsReauth: false and the reauth UI flow is broken.

Fix: Add a test where the handler rejects with NeedsReauthError and assert the job record gets error.code === 'NEEDS_REAUTH'.


8. :jobId param not classified in FACS_NON_RESOURCE_PARAMS

Severity: Important
File: src/routes/facs-capabilities.js

The PR adds two routes with a :jobId parameter. Per CLAUDE.md: "Every dynamic :param in src/routes/index.js must be classified in src/routes/facs-capabilities.js [...] The routeFacsCapabilities test suite fails the build if a param is unclassified." The diff shows routes added to the capability map but no modification to the param classification arrays.

Fix: Add 'jobId' to the FACS_NON_RESOURCE_PARAMS array.


Non-Blocking

9. [Minor] toGenerationJobDto returns createdAt/updatedAt fields not declared in the OpenAPI schema (docs/openapi/schemas.yaml). Consider adding them to the GenerationJob schema definition.

10. [Minor] resolveBrandName falls back to an empty string when the brand name cannot be resolved. This sends an empty brandName to DRS. Consider whether DRS handles this gracefully or whether a validation check is warranted.

11. [Minor] isMarketConsumerReady uses strict === 'true' comparison on the SSM parameter value, while isAsyncPromptGenEnabled uses case-insensitive comparison on the env var. The asymmetry is intentional (SSM is operator-controlled) but undocumented.

12. [Minor] The lease token (randomUUID()) is persisted in the metadata JSONB column alongside the promise token. toGenerationJobDto strips it, but a future endpoint returning raw metadata would expose both. Consider clearing the lease token on job completion.


Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 7m 28s | Cost: $16.94 | Commit: 8d28b6b8c7cb9c966284d74c65a32e5d377898e1
If this code review was useful, please react with 👍. Otherwise, react with 👎.

@MysticatBot MysticatBot added ai-reviewed Reviewed by AI complexity:high AI-assessed PR complexity: HIGH needs-human-review AI reviewer recommends a human read before merge labels Sep 9, 2026
dzehnder and others added 2 commits September 9, 2026 18:26
drs-package closed the contract (c21ef36a5): market_country is the ISO code "FR"
(DRS resolves the display name internally — the producer already sends the code,
no mapping change); audience is optional and omitted at onboarding. Re-mirror the
fixture verbatim and:
- omit `audience` from the wire payload when the caller has none (matches the
  fixture, which no longer carries the key);
- validate `model` against the DRS priced set {gpt-5.4, gpt-5.4-mini,
  gpt-5.4-nano, gpt-5-nano} at the seam (resolveDrsModel) and fall back to
  gpt-5-nano for an unpriced request/env value, with a warn — so a config typo
  can't surface as a terminal DRS gate_error (invalid_model). catalogue_status
  stays "populated" (allowed set is populated|empty; a capped catalogue is still
  populated). Contract test updated to the FR/no-audience fixture; model-guard
  tests added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Security:
- Reauth TOCTOU: atomic PostgREST compare-and-set (claimJobForReauth) flips
  FAILED+NEEDS_REAUTH -> IN_PROGRESS; only the winner mints, the loser 409s, so
  two racing reauths can't both bank an un-invalidated promise token.
- Populate metadata.siteId from the brand's linked base site so the SEC-5
  site-level ownership check is exercised; document brand-scoping as the sole
  control for genuinely siteless jobs.

Correctness:
- Fault-isolate createMarket's async enqueue (try/catch + warn) so an enqueue
  failure never fails a created+published market (parity with activate/brands).
- clearJobLease release on the retryable paths now logs a failed save (a stale
  lease blocks redelivery until its ~930s TTL — on-call must see it).
- Scrub the lease unconditionally on every terminal path (defense-in-depth).
- Skip generation for an empty/whitespace brand name (ungrounded prompts).

Minors: OpenAPI createdAt/updatedAt on the job-status schema; comment on the
strict SSM 'true' vs case-insensitive env-flag asymmetry.

Tests: reauth CAS win/lose/409 + claimJobForReauth unit; siteId resolution +
siteless brand-scoping; createMarket/activate/createBrandForOrg async paths
(annotate 201/200, enqueue-failure non-fatal, flag-off preserves sync, activate
201-vs-409 filtering); deferred-exchange NeedsReauthError -> error.code; empty-brand
guard. FACS :jobId classification verified already-green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dzehnder

dzehnder commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@MysticatBot review — incorporated the review (8 Important + 4 minor) in 06ad91c: reauth TOCTOU CAS, siteId population + siteless brand-scoping, createMarket enqueue fault-isolation, lease-release warn + unconditional scrub, empty-brand guard, OpenAPI createdAt/updatedAt, SSM-vs-env comment, and the missing async-path tests (createMarket/activate/brands, deferred NeedsReauthError). Full suite green.

@dzehnder
dzehnder requested a review from MysticatBot September 9, 2026 16:51

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dzehnder,

Verdict: Approve - the two fix commits address all prior findings; no new blocking issues.
Complexity: HIGH - large diff (4716 lines, 28 files); API surface + dependency signal.
Changes: Adds async Semrush-market AI-prompt provisioning behind a default-off feature flag (SERENITY_ASYNC_PROMPT_GEN), introducing a three-phase checkpointed worker (generate via DRS Lambda, exchange write token, publish), with CAS-based job leasing, reauth TOCTOU fix, SSM readiness gate, and a dedicated SQS queue (28 files).

Previously flagged, now resolved

  • Brands controller async path now tested (flag ON happy/failure, flag OFF preservation)
  • Controller-level async tests added for createMarket and activate entry points
  • Deferred NeedsReauthError propagation now tested at the runner level (error.code = NEEDS_REAUTH verified)
  • All 8 prior Important findings addressed in the two new commits
Non-blocking (6): minor issues and suggestions
  • suggestion: The SSM readiness cache has a 60s staleness window per warm Lambda container - document in the runbook that an emergency halt has a 60s propagation tail - src/support/serenity/market-worker-readiness.js
  • suggestion: The lease TTL (930s), worker timeout (900s), and SQS visibility timeout (960s) form a three-value invariant spanning two repos - consider a cross-repo contract test or CI assertion in spacecat-infrastructure to enforce the ordering - src/support/serenity/job-lease.js:38
  • suggestion: Seeds and generatedBatch stored in the JSONB metadata column could reach tens of KB per job at the current 50-topic cap - document the cap as a hard constraint or consider S3 references if the cap is raised - src/support/serenity/handlers/semrush-market-generation-job.js
  • suggestion: Add a handler-level test that stubs exchangeAndPersistPromiseToken to throw NeedsReauthError and verifies the handler does not catch it (pins the deferred-exchange contract at the handler boundary) - test/support/serenity/handlers/semrush-market-generation-job.test.js
  • suggestion: Add a fault-isolation test for the activate async path where maybeEnqueueMarketGeneration rejects on one market and verify the response is still 200 - test/controllers/serenity.test.js
  • nit: The DRS model allowlist is hardcoded as a frozen array; an env-based override would be more operationally flexible when the next model ships - src/support/serenity/drs-generation-client.js

Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 22s | Cost: $15.59 | Commit: 06ad91cc2aabff69019b379b45bd183fa1b581b2
If this code review was useful, please react with 👍. Otherwise, react with 👎.

The two new market-generation endpoints (poll GET + reauth POST) were
unclassified in the route-capability map and had no OpenAPI-contract fixtures,
failing CI's global `route coverage` and `every operationId has a fixture`
guards (the targeted controller suites did not exercise them).

- Add both routes to routeRequiredCapabilities: poll -> organization:read,
  reauth -> organization:write, mirroring the adjacent serenity market routes.
- Add contract fixtures for getSerenityMarketGenerationJobStatus (200, ship
  verdict, siteless brand-owned job) and reauthSerenityMarketGenerationJob (202
  happy path). resolvePromisePair/getIMSPromiseToken/claimJobForReauth are
  stubbed in the shared esmock block (inert for every other fixture).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

The worker invokes DRS with a region-only LambdaClient (no cross-account creds —
cross-account invoke works via the IAM grant + DRS resource policy), so
DRS_PROMPT_GENERATION_FUNCTION MUST be the DRS Lambda's full ARN. A bare name
resolves in the caller's own account (spacecat) and fails ResourceNotFound
mid-invoke — a silent misroute an operator could thread in at enablement.

- Fail-fast guard at the invoke seam: if the target is set but is not a full
  Lambda ARN, fail the job terminally with code DRS_INVALID_TARGET rather than
  surfacing a confusing AWS ResourceNotFound. Logs the value's SHAPE for ops
  (never the full value, which carries an account id). Added isFullLambdaArn.
- .env.example: replace the bare-name placeholder with a full cross-account ARN
  example + a comment stating it must be the DRS Lambda's full ARN and must equal
  infra#780's drs_generation_lambda_arn.
- Docstrings on DRS_GENERATION_TARGET_ENV and the invoker now say "full
  cross-account ARN". Guard tests: ARN passes; bare name / malformed ARN -> clear
  terminal error, never invoked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dzehnder and others added 2 commits September 10, 2026 17:14
…ket-async-producer

# Conflicts:
#	src/controllers/serenity.js
Merge origin/main brought in #3232 (SEC-5 hardening), which made
loadJobScopedToCaller fail-closed: a resolver-supplied job with no siteId is now
denied (404) instead of falling through. The market-generation poll/reauth
endpoints supply resolveOwnerSiteId, so their jobs must carry metadata.siteId
(the producer already stamps it from the brand base site).

- Contract fixtures: the two market-generation job fixtures now carry
  metadata.siteId and the fakeContext resolves Site.findById + an admin scope so
  the (unmocked, transitive) AccessControlUtil admits the legitimate owner.
- Corrected the two controller comments that claimed a siteless job falls back to
  the brand-match as the sole ownership control — post-#3232 that path is denied
  fail-closed at the primitive; the brand-match is now defense-in-depth only.
- Also resolves the import-block + esmock duplicate-key merge conflicts from main's
  #3214 (getRawPromiseToken/getSemrushPair/exchangePromiseTokenResponse) coexisting
  with this branch's reauth (getIMSPromiseToken).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI complexity:high AI-assessed PR complexity: HIGH needs-human-review AI reviewer recommends a human read before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Serenity][Prompt Strategy] Async Semrush-market AI prompt provisioning — AsyncJob producer, token lifecycle, Semrush writes, polling & reauth

2 participants