feat(brand-claims): trigger prerequisite audits before an on-demand claims run (LLMO-7263) - #3208
feat(brand-claims): trigger prerequisite audits before an on-demand claims run (LLMO-7263)#3208akshaymagapu wants to merge 4 commits into
Conversation
…laims run (LLMO-7263) When a trial customer requests an on-demand Brand Claims run, fire the offsite-brand-presence and wikipedia-analysis audits before the brand-claims trigger so mystique has fresh inputs for claims extraction. All three enqueue to AUDIT_JOBS_QUEUE_URL; the audit-worker delays the on-demand claims ready-signal (850s) so the prerequisites land first. No deploy-order dependency: if this ships before the audit-worker change, claims simply runs without the delay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…uisite audits (LLMO-7263) The controller-level requestBrandClaims test asserted a single SQS send; the on-demand request now enqueues offsite-brand-presence + wikipedia-analysis before brand-claims. Assert three sends in order. (Missed locally — this file wasn't in the single-file run; caught by CI.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hey @akshaymagapu,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - one blocking finding on error-path observability for the new multi-send flow.
Complexity: MEDIUM - small diff; API surface signal.
Changes: Fires two prerequisite audit triggers (offsite-brand-presence, wikipedia-analysis) before the on-demand brand-claims trigger so mystique has fresh inputs (3 files).
Note: CI checks are currently failing (semantic-release, deploy-stage, branch-deploy) - resolve before merge.
Must fix before merge
- [Important] Partial enqueue failure logs a generic error without naming which messages already landed -
src/controllers/llmo/brand-claims.js:240(details inline)
Non-blocking (1): minor issues and suggestions
- suggestion: Add a test where
sqsSendresolves on the first call then rejects on the second, asserting 500 and verifying the partial-send path is covered -test/controllers/llmo/brand-claims.test.js
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 4m 28s | Cost: $3.56 | Commit: 70f261835e28185e5f9467618cd133a66564f7c1
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| // claims; the audit-worker delays the claims ready-signal (onDemand) to let these | ||
| // complete. All three land on the same audit queue. | ||
| for (const type of BRAND_CLAIMS_PREREQUISITE_AUDIT_TYPES) { | ||
| // eslint-disable-next-line no-await-in-loop |
There was a problem hiding this comment.
issue (blocking): The three sendMessage calls are sequential inside one try/catch. If the first prerequisite succeeds but a later send fails, the catch returns 500 - but the error log says "failed to enqueue audit" without naming which messages already landed on the queue. An operator investigating a partial failure has no signal about what was sent.
Track which messages were sent before the failure:
let enqueuedTypes = [];
try {
for (const type of BRAND_CLAIMS_PREREQUISITE_AUDIT_TYPES) {
await sqs.sendMessage(queueUrl, {
type, siteId: site.getId(),
auditContext: { trigger: 'on-demand-brand-claims' },
});
enqueuedTypes.push(type);
}
await sqs.sendMessage(queueUrl, { /* claims trigger */ });
enqueuedTypes.push('brand-claims');
} catch (sqsError) {
log.error(`Brand Claims on-demand: failed after enqueuing [${enqueuedTypes.join(', ')}] for site ${site.getId()}: ${sqsError.message}`);
return internalServerError('Brand Claims on-demand is temporarily unavailable');
}The orphaned prerequisite audit is harmless (idempotent, no onDemand flag), so the external contract does not need to change - only the error log needs to reflect what actually happened.
…alert (LLMO-7263) The alert now includes who triggered the run - "by <Name> (<email>)" - resolved from the caller's IMS profile (trial_email, then preferred_username; profile.email is an IMS GUID last resort), mirroring user-details.js. The lookup is fail-safe: it is wrapped in try/catch returning 'unknown', and runs only in the best-effort Slack block after the run is already enqueued, so it can never block the run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s Slack alert (LLMO-7263)" This reverts commit 790765e.
…alert (LLMO-7263) (#3215) ## What Name the person who triggered an on-demand Brand Claims run in the Slack alert (LLMO-7263). The alert now reads: `:rocket: On-demand Brand Claims requested for *<site>* (<id>) by <Name> (<email>).` The requester is resolved from the caller's IMS profile, mirroring `user-details.js`: - **email**: `trial_email` -> `preferred_username` -> `email` (the trial user's real address is `trial_email`; `profile.email` is an IMS user GUID, used only as a last resort). - **name**: `first_name`/`given_name` + `last_name`/`family_name` when present. - Falls back to just the email, or `unknown`. ## Fail-safe The requester lookup can never block the run: 1. The SQS trigger is enqueued **before** the Slack block, so the run is already in flight. 2. The Slack call is best-effort (`try/catch`, a failure only logs a warning). 3. `getRequesterLabel` itself is wrapped in `try/catch` returning `unknown`. ## Testing - `test/controllers/llmo/brand-claims.test.js` - asserts the alert names the requester (name+email), the `preferred_username` fallback, and `unknown` when no profile is present. Existing paths unchanged. - Tests passing, eslint clean. ## Notes Split out from #3208 (prerequisite audits) to keep that PR standalone; both are LLMO-7263. No dependency between them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
# [1.797.0](v1.796.1...v1.797.0) (2026-09-07) ### Bug Fixes * add diagnostic logging to edge-live-preview fetch (LLMO-7416) ([#3216](#3216)) ([4279a84](4279a84)) ### Features * **brand-claims:** name the requester in the on-demand claims Slack alert (LLMO-7263) ([#3215](#3215)) ([143d1c4](143d1c4)), closes [#3208](#3208) * **plg-onboard:** waitlist authenticated (login/SSO) sites during onboarding ([#3142](#3142)) ([be909a9](be909a9)), closes [Hi#level](https://github.com/Hi/issues/level) [hi#precision](https://github.com/hi/issues/precision) [hi#precision](https://github.com/hi/issues/precision)
What
When a trial customer requests an on-demand Brand Claims run (
POST /sites/:siteId/llmo/brand-claims/request, LLMO-7263), fire theoffsite-brand-presenceandwikipedia-analysisaudits before thebrand-claimstrigger, so mystique has fresh inputs when it runs claims extraction.All three enqueue to
AUDIT_JOBS_QUEUE_URL. The prerequisites are plain audit triggers ({ type, siteId, auditContext }); only the claims trigger carriesonDemand: true. The audit-worker delays the on-demand claims ready-signal (850s) so the prerequisites land first - see adobe/spacecat-audit-worker#2943.Coordination
No deploy-order dependency with the audit-worker change:
Neither order breaks.
Testing
test/controllers/llmo/brand-claims.test.jsandtest/controllers/llmo/llmo.test.js- updated for the 3 sends, assert order (offsite-brand-presence,wikipedia-analysis,brand-claims), and that only the claims trigger carriesonDemand. Existing cooldown/Slack paths unchanged.test/it/postgres/brand-claims-request.test.js) does not exercise the 202 happy path, so no IT change.🤖 Generated with Claude Code