fix(llmo): read brand region from V2 brands table instead of hardcoding US (LLMO-4258)

[dzehnder]

Summary

• LLMO-4258: Removes premature prompt generation from onboarding — deferred to DRS post-Brandalf completion (option b)
• LLMO-4294: Fixes type: 'url' → type: 'base' constraint violation blocking all V2 brand creation

Problem

LLMO-4258: Wrong language prompts

During onboarding, prompt generation fired immediately with hardcoded region: 'US' — before Brandalf had a chance to populate the brand's actual region. Even with a region lookup, the timing was wrong (reviewer feedback: Brandalf is async and hasn't completed yet).

Fix: Remove prompt generation from the onboarding flow entirely. DRS now triggers prompt_generation_base_url automatically after Brandalf completes and syncs brands (with correct regions) to SpaceCat. See companion PR.

LLMO-4294: Brand creation constraint violation

The upsertBrand call passed type: 'url' in the brand's URLs array, but the brand_sites table only allows 'base' or 'localized'. This silently failed, meaning NO brand entity was created for ANY V2 onboarding.

Fix: Change type: 'url' → type: 'base' in 3 places (lines 218, 278, 1366).

Changes

• src/controllers/llmo/llmo-onboarding.js:
  • Removed prompt gen submission block (~40 lines) + region lookup + listBrands import
  • Added comment explaining new deferred flow
  • Fixed type: 'url' → type: 'base' in 3 places
• test/controllers/llmo/llmo-onboarding.test.js:
  • Removed 3 prompt-gen-related tests (no longer applicable)
  • Updated happy path test to assert prompt gen is NOT submitted
  • Updated type assertions from 'url' to 'base'

Companion PR

• llmo-data-retrieval-service#1308 — expands REGION_TO_LANGUAGE + triggers prompt gen after Brandalf brand sync

Test plan

• 7930 tests passing, 100% coverage
• Prompt gen is no longer called during onboarding
• type: 'base' used for brand URLs
• E2E: onboard a test site, verify Brandalf → prompt gen chain produces correct-language prompts

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a patch release when merged.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[solaris007]

Hey @dzehnder,

Strengths

• Correct architectural direction - reading region from the V2 normalized brands table (the system of record, populated by Brandalf) instead of hardcoding is the right move. (llmo-onboarding.js:1418-1435)
• Solid defensive structure - the region lookup is isolated in its own try/catch with fallback to 'US', so a PostgREST failure or missing data can never break onboarding. (llmo-onboarding.js:1420-1434)
• Null-safe access with context.dataAccess?.services?.postgrestClient and the if (postgrest) guard prevents NPEs when PostgREST is unavailable. (llmo-onboarding.js:1421-1422)
• The default stub listBrands: sinon.stub().resolves([]) in the test helper ensures all existing tests pass without changes. (llmo-onboarding.test.js:260)
• Good coverage of both the happy path (region from brands table) and error fallback (PostgREST failure).

Issues

Important (Should Fix)

1. Timing: the fix is ineffective during initial onboarding
llmo-onboarding.js:1418-1435

Trace the execution order inside performLlmoOnboarding:

• Step A (~line 1361): upsertBrand creates/updates the brand with no region field, resulting in regions: [] being written to the DB
• Step B (~line 1383): triggerBrandalfOnboardingJob submits an async DRS job - Brandalf will eventually populate the brand's regions, but that takes minutes
• Step C (this new code, line 1423): listBrands reads the brand back - finds the brand from Step A with region: [], so matchingBrand?.region?.length > 0 is false - falls back to 'US'
• Step D: submitOnboardingPromptGenerationJob fires with region: 'US' - same as before the fix

During initial onboarding, Brandalf has not completed by the time prompt generation is submitted. The region will always be empty. The fallback will always trigger. The stated problem (Dutch customers getting English prompts) is not fixed for the initial onboarding path.

On re-onboarding, upsertBrand at Step A also writes regions: [], overwriting whatever Brandalf previously populated - so the fix is also ineffective there.

Options to address:

• (a) Derive region from an already-available signal earlier in the flow (site TLD, org metadata, site locale config) and pass it into the initial upsertBrand call
• (b) Defer prompt generation until Brandalf completes (event-driven via SNS/callback rather than submitting both jobs sequentially) - cleanest architecturally
• (c) Have DRS resolve region at job processing time from the brands table rather than baking it into job parameters at submission time
• (d) If this fix is intentionally scoped to a narrow re-trigger scenario only, document that in a comment and update the PR description

2. Brand matching priority - baseSiteId should take precedence over name
llmo-onboarding.js:1424-1426

brands.find((b) => b.name === brandName.trim() || b.baseSiteId === site.getId()) uses OR, so a name collision with a different brand in the same org could return that brand's region. The baseSiteId match is a direct link to the site being onboarded and is the more precise signal. Prefer it:

const matchingBrand = brands.find((b) => b.baseSiteId === site.getId())
  || brands.find((b) => b.name === brandName.trim());

3. Over-fetching: listBrands joins 5 child tables for a single-field lookup
llmo-onboarding.js:1423

listBrands uses BRAND_SELECT which joins brand_aliases, brand_social_accounts, brand_earned_sources, competitors, and brand_sites. All that data is fetched and mapped through mapDbBrandToV2, then discarded - only region is used. A targeted PostgREST query would be far lighter:

const { data } = await postgrest
  .from('brands')
  .select('regions')
  .eq('organization_id', orgId)
  .eq('site_id', siteId)
  .eq('status', 'active')
  .maybeSingle();

4. No validation of region value against known region codes
llmo-onboarding.js:1428

brandRegion is read from the database and passed to DRS without verifying it is a valid region code. The regions reference table already exists (listRegions() in brands-storage.js), but the onboarding code does not consult it. An unexpected value (freeform string, the 'gl' global marker, or a very long value) would be forwarded to DRS and could produce wrong-language prompts. Validate against the reference table or a static allowlist of supported regions, falling back to 'US' with a warning if invalid.

5. Missing test coverage for realistic edge cases
test/controllers/llmo/llmo-onboarding.test.js

Three production-realistic scenarios lack tests:

• Brand matched by name (not baseSiteId) - the current test only exercises the baseSiteId path
• Brand found but region is empty (region: []) - the most common state during initial onboarding
• postgrestClient is undefined - environments where PostgREST is not yet configured

These are quick to add given the existing test scaffolding.

Minor (Nice to Have)

6. Log message lacks stack trace
llmo-onboarding.js:1433 - log.warn only logs regionError.message. If the catch fires due to an unexpected TypeError, the stack trace would be essential. Consider log.warn('Failed to read brand region, defaulting to US', regionError).

7. Multi-region ambiguity undocumented
llmo-onboarding.js:1428 - [brandRegion] = matchingBrand.region picks the first element. For a brand with ['NL', 'BE'], this silently picks 'NL'. A comment explaining the heuristic (e.g., "first region is the primary market") would help future readers.

8. Inconsistent variable naming
llmo-onboarding.js:1421 - The new code uses postgrest while every other reference in this file uses postgrestClient.

Recommendations

• Consider whether upsertBrand at line 1360 should preserve existing regions rather than implicitly resetting them. That would make the region lookup work correctly on re-onboarding.
• Long-term, if region is only reliably available after Brandalf completes, consider an event-driven design where Brandalf's completion triggers prompt generation - this removes the temporal coupling entirely.
• If the over-fetching is not addressed in this PR, consider a follow-up to add a targeted getBrandRegion helper rather than repurposing listBrands.

Assessment

Ready to merge? No - needs discussion on scope

The code is safe and will not break anything - the fallback behavior is solid. However, the timing interaction with upsertBrand and the async Brandalf job means the fix does not achieve its stated goal during initial onboarding or re-onboarding, which is the scenario described in the ticket. Before merging, the team should either address the timing gap, or reframe the PR scope to document which scenario this actually fixes. The matching priority, over-fetching, and missing test coverage are secondary but worth addressing.

[alinarublea]

Security & Architecture Review — PR #2198

Reviewer perspective: Security specialist / Distinguished engineer
Verdict: Approve

---

Executive Summary

This PR removes ~75 lines of premature prompt generation logic from the LLMO onboarding flow, deferring it to DRS post-Brandalf completion. It directly addresses the timing bug identified in the previous review (option b). The type: 'url' → type: 'base' fix mentioned in the PR description was already merged to main separately and is no longer in this diff — the PR description is stale on that point.

---

Security Assessment: No Issues

1. Attack surface reduction — Removing submitOnboardingPromptGenerationJob and buildPromptGenerationMetadata eliminates a direct code path from SpaceCat to DRS prompt generation. One fewer outbound RPC from the onboarding hot path means one fewer place where parameter injection, SSRF via baseUrl, or model override attacks (model: 'gpt-5-nano') could be exploited. Net positive.

2. No new trust boundaries introduced — The DRS companion PR (adobe-rnd/llmo-data-retrieval-service#1308) takes over prompt generation, but SpaceCat already trusts DRS for Brandalf jobs. No new cross-service trust relationship is created.

3. No secrets, credentials, or auth changes — Clean removal of business logic only.

4. No input validation regressions — The removed code only consumed internal state (siteConfig, brandName, baseURL), so removing it doesn't weaken any validation path.

---

Architecture Assessment

What's good:

• Correct resolution of the timing bug. The previous review identified that upsertBrand writes regions: [], then Brandalf is async, so the prompt gen job always fell back to 'US'. Deferring to post-Brandalf is the cleanest fix — it removes temporal coupling rather than papering over it.
• Clean deletion. Both the function definitions and their sole call site are removed. No dead imports left behind. buildOnboardingMetadata is still used by the Brandalf metadata builder, so it's correctly retained.
• Tests assert the negative. expect(...secondCall).to.be.null and expect(...submitJob).to.not.have.been.called correctly verify the behavior change rather than just deleting assertions.

Observations (non-blocking):

1. Tombstone comment at line 137 — // submitOnboardingPromptGenerationJob removed — prompt generation is now triggered by DRS... Comments describing what used to be here become noise within a few months. The function is gone; git blame/git log is the right tool for archaeology. Consider replacing with nothing, or at most a one-liner about the current contract.

2. Observability gap — The removed code had explicit error handling with Slack notification (:warning: Failed to start DRS prompt generation (will need manual trigger)). With prompt gen now deferred to DRS, if DRS fails to trigger it after Brandalf completes, there is no visibility from SpaceCat's side. Consider:

   • Does DRS have equivalent alerting for failed prompt generation?
   • Is there a reconciliation mechanism to detect sites that completed onboarding but never got prompts generated?
   • Not a blocker for this PR, but worth a follow-up ticket.

3. PR description drift — The description prominently features the LLMO-4294 type: 'url' → type: 'base' fix, but that change is not in this diff (it already landed on main via another PR). Consider updating the PR title/description to reflect only what's actually in scope.

---

Test Coverage Assessment

• Removed 3 prompt-gen-related tests: appropriate since the code path no longer exists
• Updated 2 assertions to verify prompt gen is not called: correct negative testing
• Removed the "DRS prompt generation failure" test (~94 lines): appropriate since the error path no longer exists
• Remaining Brandalf-related error tests are retained and still exercise relevant code paths

No gaps identified.

---

Summary

Category	Rating
Security	No issues
Correctness	Sound — directly addresses the root cause
Test coverage	Adequate
Code hygiene	Good (minor tombstone comment)
Operational risk	Low — follow up on DRS-side observability

🤖 Generated with Claude Code

implemented requested changes. Alina re-reviewed in your absence

[solaris007]

🎉 This PR is included in version 1.445.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀