fix(serenity): resolve languageCode by Semrush BCP-47 code, not English name by aliciadriani · Pull Request #3221 · adobe/spacecat-api-service

aliciadriani · 2026-09-07T14:53:32Z

1. Abstract

Resolves Brand Markets languageCode to a Semrush language id by exact BCP-47 code match instead of English-name matching, and surfaces code from the org-level language catalog endpoint.

2. Reasoning

resolveLanguageId converted a market's BCP-47 languageCode to an English display name via Intl.DisplayNames and matched it against Semrush's catalog name — brittle by construction: Semrush spells "Chinese Simplified" where Intl.DisplayNames says "Simplified Chinese" (word-order collision), aliases like Filipino/Tagalog exist, and any future catalog rename silently drops a language. This is the root cause behind LLMO-7309 (Chinese language selection broke, tracked in the still-open, not-yet-merged #3183). Semrush's language catalog now returns a stable BCP-47 code per entry (LLMO-7420, confirmed live 2026-09-07); this PR makes that code the resolution key end-to-end and deletes the English-name matching entirely.

3. High-level overview of the changes

resolveLanguageId (src/support/serenity/handlers/markets.js) no longer converts languageCode to an English name via Intl.DisplayNames and matches by name. It now caches the language catalog keyed by lowercased code and looks up the (already-normalized) languageCode directly. An unresolved code still 400s unknownLanguage — no fallback heuristics, so a new or renamed Semrush language needs no Adobe-side mapping change.
listLanguageCatalog now includes code in each returned item (previously only id/name) and drops rows missing either name or code.
SerenityLanguage OpenAPI schema (docs/openapi/schemas.yaml) gained a required code field; the GET /serenity/languages route description (docs/openapi/serenity-api.yaml) no longer describes name as "the resolution key" — code is.
No change to the languageCode input validation (normalizeLanguageCode) or to the slice-key shape.

4. Required information

Jira / issue: LLMO-7420
Other: companion PRs — adobe/spacecat-shared fix(deps): update adobe fixes #1919 (adds the code field this PR consumes), adobe/project-elmo-ui LLMO-7420-semrush-bcp47-language-source-of-truth (builds the language picker from code)

6. Affected / used mysticat-workspace projects

spacecat-shared: @adobe/spacecat-shared-project-engine-client's listLanguages() must return code per entry for this resolver to work — depends on PR fix(deps): update adobe fixes #1919 publishing before this can be deployed against live Semrush (verified locally via npm link to the unpublished branch; the dependency version in package.json is intentionally not bumped yet).
project-elmo-ui: consumes code from GET /v2/orgs/{orgId}/serenity/languages to build the Brand Markets language picker.

8. Test plan

(a) Ran the full unit suite locally, including the rewritten Chinese-script resolution cases (zh-Hans/zh-Hant/fil resolving by exact code, no name-matching fallback) and the listLanguageCatalog catalog-shape tests, plus npm run type-check and npm run docs:lint against the updated OpenAPI schema.
(b) Dev/stage: after bumping @adobe/spacecat-shared-project-engine-client to the published version carrying code, exercise POST /serenity/markets for a Hong Kong brand with languageCode: zh-Hans and confirm it resolves to the Chinese Simplified project (previously broken per LLMO-7309); confirm GET /v2/orgs/{orgId}/serenity/languages returns code per entry.

9. Deployment & merge order

Depends on: adobe/spacecat-shared fix(deps): update adobe fixes #1919 must merge and publish before this PR is deployed (it can merge before publish, but the live Semrush catalog resolution won't work in a deployed environment until the dependency is bumped to the published version in a follow-up commit).
Related: adobe/project-elmo-ui LLMO-7420-semrush-bcp47-language-source-of-truth (consumes this PR's code field from the languages endpoint).
Sequence: merge/publish spacecat-shared fix(deps): update adobe fixes #1919 → bump the dependency here and deploy → merge/deploy the project-elmo-ui companion PR.

🤖 Generated with review-kit

Change Management

cm-assessment: v1
changeType: standard
impact: unnoticeable
risk: minor
changeApprovedBy: ["Dominique Jäggi"]
rationale: "Internal resolution-key bug fix (matches Semrush catalog by BCP-47 code instead of brittle English-name matching); test-covered and reversible by redeploy/rollback, no auth or data-destructive surface."

…sh name resolveLanguageId matched a languageCode's Intl.DisplayNames English name against Semrush's catalog name, which silently broke on catalog renames (e.g. "Chinese Simplified" vs "Simplified Chinese") or aliases (Filipino/ Tagalog) — the root cause behind LLMO-7309. Semrush's language catalog now carries a stable BCP-47 code (LLMO-7420); resolve directly against it instead, and drop the English-name matching entirely. listLanguageCatalog and the SerenityLanguage OpenAPI schema now surface that code so clients can build language pickers from it. Supersedes #3183 without merging it. Depends on a future @adobe/spacecat-shared-project-engine-client release carrying the catalog's code field (see the spacecat-shared LLMO-7420 branch) — the dependency version is not bumped here since that package hasn't published yet.

github-actions · 2026-09-07T14:55:04Z

This PR will trigger a patch release when merged.

…code field listSerenityOrgLanguages' AJV contract test fixture returned { id, name } without code, failing schema validation now that SerenityLanguage requires it. Caught by re-running the full suite directly (npm test), not the piped/truncated run used earlier.

codecov · 2026-09-07T15:49:03Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

aliciadriani · 2026-09-07T16:40:09Z

    handlerName: 'listLanguageCatalog',
    handlerResult: {
-      items: [{ id: 'lang-en', name: 'English' }],
+      items: [{ id: 'lang-en', name: 'English', code: 'en' }],


Must Fix: This fixture's handlerResult returned { id, name } without code. Since SerenityLanguage now requires code, the AJV contract check for listSerenityOrgLanguages failed: must have required property 'code'. Only surfaced when npm test was run unpiped (an earlier local check piped through tail, which reports tail's exit code, not npm test's — masked the failure).

aliciadriani · 2026-09-07T16:40:17Z

    handlerName: 'listLanguageCatalog',
    handlerResult: {
-      items: [{ id: 'lang-en', name: 'English' }],
+      items: [{ id: 'lang-en', name: 'English', code: 'en' }],


Must Fix: This fixture's handlerResult returned { id, name } without code. Since SerenityLanguage now requires code, the AJV contract check for listSerenityOrgLanguages failed: must have required property code. Only surfaced when npm test was run unpiped (an earlier local check piped through tail, which reports tail exit code, not npm test's - masked the failure).

Fixed in 6f8c3b6 - fixture now returns code: 'en'. Verified: npx mocha --timeout 20000 test/openapi-contract/serenity-api.test.js -> 37 passing, 0 failing.

aliciadriani · 2026-09-07T16:40:34Z

+ * when Semrush adds or renames a language.
+ * @param {SerenityTransport} transport
+ */
+export async function resolveLanguageId(transport, languageCode, log) {


Should Fix (informational, not blocking): ci/it-postgres will fail on this PR until adobe/spacecat-shared#1919 merges and publishes and the @adobe/spacecat-shared-project-engine-client dependency here is bumped. The IT suite pulls a version-pinned, already-published GHCR mock image that predates the code field, so resolveLanguageId's catalog cache ends up empty against it and every market-creation IT test 400s unknownLanguage. Confirmed locally with Docker and reproduced on this PR's own CI run. This is the sequencing already called out in the PR description, not a defect in this diff - flagging inline so it is not missed as a real regression during review.

Acknowledged - not fixable within this PR; it resolves itself once adobe/spacecat-shared#1919 merges+publishes and the dependency here is bumped in a follow-up commit (see section 9, Deployment & merge order, in the PR description). Not addressed by a code change.

…U logic resolveLanguageId no longer converts languageCode to an English name via Intl.DisplayNames — the comment and test title still described that deleted code path. Update both to describe the actual behavior: an unresolved code simply isn't a key in the byCode map.

aliciadriani · 2026-09-07T16:44:41Z

-  // tags. The handler guards against that and returns null from
-  // isoToEnglishName, which surfaces as 400 unknownLanguage.
-  it('400s when the language tag is not a real language (ICU returns it unchanged)', async () => {
+  // Branch coverage: a syntactically valid but unresolvable code (not a key in


Nit: the comment above this test (and its title) still referenced isoToEnglishName/ICU-DisplayNames-echo behavior, which was deleted in this PR. The test itself still passed (input 'xx' just isn't a key in the new byCode map), but the comment would mislead the next reader into thinking ICU is still involved in this resolution path.

Fixed in 58fdef0 - comment and test title now describe the actual byCode-lookup behavior. Verified: npx mocha --timeout 20000 test/support/serenity/handlers/markets.test.js -> 105 passing, 0 failing.

MysticatBot

Mysticat could not complete a verified review of this pull request.

This is a service-side gap, not a finding about your code: no binding APPROVE or REQUEST_CHANGES review could be produced and verified for the current commit.

Current head: 58fdef0bcf8ad818766e0e27ade6ad5b7bf5a98a
Last reviewed head: 6f8c3b60e03988fd192dff458ea4b5fab13ae5bf
Failure category: head_drift
Request ID: b8afd7c0-aacf-11f1-810f-aac20616eec9
Cost: $9.26
Duration: 4m 35s

Please re-request a review (e.g. push a new commit or re-request from the configured reviewer) to try again.

…ruth

MysticatBot

Hey @aliciadriani,

Verdict: Request changes - two addressable gaps before this ships.
Complexity: MEDIUM - medium diff; API surface change.
Changes: Replaces brittle English-name-based Semrush language resolution with direct BCP-47 code matching, adds code to the SerenityLanguage schema and catalog endpoint (6 files).
Note: CI checks are currently pending - confirm they pass before merge.

Must fix before merge

[Important] No regression test for the root-cause scenario (zh-Hans/zh-Hant script-subtag disambiguation) - test/support/serenity/handlers/markets.test.js (details inline)
[Important] listLanguageCatalog silently drops catalog entries missing code with no observability - src/support/serenity/handlers/markets.js:~1035 (details inline)

Non-blocking (3): minor issues and suggestions

suggestion: Add a brief code comment at the byCode.set call explaining the .toLowerCase() normalization (BCP-47 tags are case-insensitive per RFC 5646) so a future reader does not "fix" it to preserve casing - src/support/serenity/handlers/markets.js:100
suggestion: Hoist the if (!hasText(languageCode)) return null guard above the cache-refresh block to avoid a wasted transport.listLanguages() call on null/empty input when the cache is cold - src/support/serenity/handlers/markets.js:114
suggestion: The OpenAPI code field description shows zh-Hans with original casing, but the resolution path lowercases; consider noting the case-insensitive matching so API consumers know they should store as-is and send as-is - docs/openapi/schemas.yaml:~12639

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 57s | Cost: $5.69 | Commit: 946d22df863a8427b12a1aea901d2436b06a3201
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

MysticatBot · 2026-09-08T08:36:27Z

@@ -338,7 +338,7 @@ describe('handlers/markets.js — handleCreateMarket', () => {
    dataAccess.BrandSemrushProject.create.resolves();


issue (blocking): No regression test for the root-cause scenario. Every listLanguages stub uses simple 2-letter codes (en, de). The motivating bug - Chinese zh-Hans/zh-Hant collapsing to the same resolution under the old English-name approach (LLMO-7309) - is never exercised.

The case-folding path (String(item.code).toLowerCase() in cache vs. the already-lowercased input from normalizeLanguageCode) is load-bearing for CJK languages and has no test coverage.

How to fix: Add a test where the transport returns at least two entries with different script subtags:

listLanguages: sinon.stub().resolves({ items: [ { id: 'lang-zhcn', name: 'Chinese Simplified', code: 'zh-Hans' }, { id: 'lang-zhtw', name: 'Chinese Traditional', code: 'zh-Hant' }, ] })

Then assert that languageCode: 'zh-hans' resolves to lang-zhcn and languageCode: 'zh-hant' resolves to lang-zhtw. This is the single most valuable test this PR can carry - it guards against re-regression of the exact bug it fixes.

Fixed in c1f5c6c - added a dedicated resolveLanguageId describe block with a test resolving zh-Hans and zh-Hant to distinct catalog ids (the exact LLMO-7309 root-cause scenario), plus a sibling test confirming an unknown code returns null with no fallback matching.

MysticatBot · 2026-09-08T08:36:27Z

-        languageCache.byTag.set(String(item.name).toLowerCase(), String(item.id));
+      if (hasText(item?.code) && hasText(item?.id)) {
+        languageCache.byCode.set(String(item.code).toLowerCase(), String(item.id));
      }


issue (blocking): listLanguageCatalog silently drops entries missing code with no log trail. The filter at ~line 1035 changed from hasText(l.name) to hasText(l.name) && hasText(l.code). If the upstream catalog returns entries with a name but no code (partial rollout, Semrush adding a new language before populating code, or companion PR not yet deployed), those entries vanish from the catalog response. The UI language picker would show fewer languages than Semrush supports, with no observability signal.

resolveLanguageId already has a log.warn for when the catalog returns no usable entries - listLanguageCatalog has no equivalent.

How to fix: Add a warn-level log when entries are dropped for missing code. This requires threading log into listLanguageCatalog, which is a minor signature change:

const dropped = rawItems.filter(l => l && typeof l === 'object' && hasText(l.name) && !hasText(l.code)); if (dropped.length > 0) { log?.warn?.('listLanguageCatalog: dropped entries missing code', { count: dropped.length }); }

Fixed in c1f5c6c - listLanguageCatalog now takes an optional log param and warns (with a dropped count) when entries are dropped for missing code, threaded through from the listOrgLanguages controller. Added two tests covering the warn and no-warn paths.

MysticatBot

Hey @aliciadriani,

⚠ 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 - two gaps to close before merge.
Complexity: HIGH - medium diff; OpenAPI schema is a risk surface.
Changes: Replaces English-name matching with exact BCP-47 code-based resolution for Semrush language IDs, adding a required code field to the SerenityLanguage schema and updating all test fixtures (6 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI schema). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are still pending - verify before merge.

Must fix before merge

[Important] Stale JSDoc on listLanguages() in src/support/serenity/rest-transport.js still describes the old resolution mechanism ("resolve the language_id UUID from an ISO 639-1 code"). The resolution key is now the BCP-47 code field from the catalog, not an ISO 639-1 to English-name lookup. Update the JSDoc to reflect the BCP-47 code-based resolution (LLMO-7420). (File not in diff; cannot anchor inline.)
[Important] No tests exercise the motivating regression scenarios (zh-Hans, zh-Hant, fil) - test/support/serenity/handlers/markets.test.js:614 (details inline)

Non-blocking (3): minor issues and suggestions

nit: resolveLanguageId JSDoc could clarify the function is intentionally case-insensitive regardless of caller normalization - src/support/serenity/handlers/markets.js:85
suggestion: listLanguageCatalog shows id-less languages that resolveLanguageId cannot resolve (pre-existing asymmetry); consider aligning filters or adding a comment documenting the intended gap - src/support/serenity/handlers/markets.js:1035
suggestion: || null at src/support/serenity/handlers/markets.js:118 could be ?? null to express intent more precisely (functionally equivalent given the hasText guard on cache population)

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 7m 35s | Cost: $5.86 | Commit: 946d22df863a8427b12a1aea901d2436b06a3201
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

MysticatBot · 2026-09-08T08:41:47Z

+  // Branch coverage: a syntactically valid but unresolvable code (not a key in
+  // the catalog's byCode map) surfaces as 400 unknownLanguage — no fallback
+  // heuristic catches it (LLMO-7420: exact code match only).
+  it('400s when the language code is not in the Semrush catalog', async () => {


issue (blocking): No tests exercise the motivating regression scenarios. Every test stub uses simple 2-letter codes (en, de, fr, xx), but the entire rationale for this PR (LLMO-7420) is that Chinese (zh-Hans, zh-Hant) and Filipino (fil) broke under the old English-name matching.

What's wrong: The fix this PR ships - exact BCP-47 code match with case-insensitive lowering - is not exercised by any runnable assertion with script subtags or 3-letter primary subtags. A regression (e.g., someone reintroducing .split('-')[0]) would pass the full test suite.

Why it matters: The lowercased round-trip (zh-Hans -> zh-hans in cache, matching zh-hans from normalizeLanguageCode) and the ability to distinguish zh-Hans from zh-Hant as separate IDs are the core invariants this PR establishes. Without tests, they are documented in JSDoc but not proven.

How to fix: Add a test that stubs the catalog with entries like [{ id: 'lang-zh-s', name: 'Chinese (Simplified)', code: 'zh-Hans' }, { id: 'lang-zh-t', name: 'Chinese (Traditional)', code: 'zh-Hant' }, { id: 'lang-fil', name: 'Filipino', code: 'fil' }] and asserts:

languageCode: 'zh-hans' resolves to lang-zh-s

languageCode: 'zh-hant' resolves to lang-zh-t

languageCode: 'fil' resolves to lang-fil

languageCode: 'zh' (bare primary subtag) does NOT resolve when only script-qualified entries exist

Strengthened in 97d9c84 - the regression test now covers all three motivating LLMO-7309 scenarios in one fixture (zh-Hans, zh-Hant, fil) plus a negative assertion that the bare primary subtag 'zh' does NOT resolve when only script-qualified entries exist. (Note: the core zh-Hans/zh-Hant distinction test already existed from an earlier round of this same review loop, in c1f5c6c - this commit adds fil and the negative case this specific comment asked for.)

…t + observability - Add a direct resolveLanguageId unit test resolving zh-Hans/zh-Hant to distinct catalog ids — the exact LLMO-7309 root-cause scenario (script subtags collapsing under English-name matching) had no regression coverage. - listLanguageCatalog now warns when catalog entries are dropped for missing code, threading log through from the controller — previously silent, with no observability signal if the catalog partially regresses. - Hoist resolveLanguageId's empty-languageCode guard above the cache refresh to avoid a wasted upstream call. - Document BCP-47 case-insensitive matching at the byCode.set call site and in the OpenAPI code field description.

aliciadriani · 2026-09-08T08:44:29Z

-      if (hasText(item?.name) && hasText(item?.id)) {
-        languageCache.byTag.set(String(item.name).toLowerCase(), String(item.id));
+      if (hasText(item?.code) && hasText(item?.id)) {
+        // BCP-47 tags are case-insensitive (RFC 5646) — lowercase both the catalog's `code`


MysticatBot suggestion: add a comment at byCode.set explaining the .toLowerCase() normalization so a future reader doesn't 'fix' it to preserve casing.

Fixed in c1f5c6c - added the RFC 5646 case-insensitivity comment at the byCode.set call site.

aliciadriani · 2026-09-08T08:44:47Z

+ * @param {SerenityTransport} transport
+ */
+export async function resolveLanguageId(transport, languageCode, log) {
+  if (!hasText(languageCode)) {


MysticatBot suggestion: hoist the empty/null languageCode guard above the cache-refresh block to avoid a wasted transport.listLanguages() call when the cache is cold and input is invalid.

Fixed in c1f5c6c - the guard now runs before the cache-refresh block.

aliciadriani · 2026-09-08T08:45:03Z

+      description: English language name as Semrush spells it. Metadata/fallback text only — not a resolution key (LLMO-7420).
+    code:
+      type: string
+      description: BCP-47 language code (e.g. zh-Hans, zh-Hant, fil) — the stable identifier a client persists and later sends back as languageCode (LLMO-7420). Matching is case-insensitive (RFC 5646); store and send it as returned here, exact casing is not significant.


MysticatBot suggestion: the code field description shows zh-Hans with original casing while resolution is case-insensitive - note that for API consumers.

Fixed in c1f5c6c - the description now notes matching is case-insensitive (RFC 5646) and consumers should store/send the code as returned.

…solver - Document that a null-id catalog entry is never resolvable, in both the OpenAPI schema description and (already present) the code comment. - Add a test pinning resolveLanguageId's early-return guard: a null/empty languageCode returns null without ever calling the transport. - Clarify in the JSDoc that the hasText guard is defense-in-depth, not redundant with caller-side normalization.

aliciadriani · 2026-09-08T10:00:10Z

    id:
      type: [string, 'null']
-      description: Semrush language catalog id (null when the catalog omits it).
+      description: Semrush language catalog id (null when the catalog omits it). A null id is never resolvable — createProject will 400 unknownLanguage for this entry's code regardless of what the catalog otherwise reports.


MysticatBot suggestion: listLanguageCatalog can return entries with code but id: null that can never resolve at create time - document this for API consumers.

Fixed in 4709942 - added a note to the id field description that a null id is never resolvable.

Update: strengthened beyond the doc-only fix in 4709942 - d16e1de now actually filters id-less entries out of the catalog response instead of just documenting the gap, per direct follow-up feedback on this finding.

aliciadriani · 2026-09-08T10:00:27Z

+    expect(await resolveLanguageId(transport, 'xx')).to.equal(null);
+  });
+
+  it('returns null for a null/empty languageCode without ever calling the transport (early-return guard)', async () => {


MysticatBot suggestion: add a one-line test pinning the early-return hasText guard, confirming the transport is never called for null/empty languageCode.

Fixed in 4709942 - added a test asserting resolveLanguageId(transport, null) and (transport, '') both return null and transport.listLanguages is never called.

aliciadriani · 2026-09-08T10:00:54Z

+ * is intentionally case-insensitive regardless of caller normalization
+ * (BCP-47 tags are case-insensitive per RFC 5646) — this function does not
+ * rely on the caller having lowercased its input. An unresolved code returns
+ * `null` (400 `unknownLanguage` at the call sites) —


MysticatBot nit: clarify the hasText guard is defense-in-depth, not redundant with caller-side normalization, so a future reader doesn't remove it.

Fixed in 4709942 - JSDoc now explicitly says the guard is defense-in-depth, called directly in tests without assuming prior normalization.

aliciadriani · 2026-09-08T10:01:11Z

        { receivedKeys: Object.keys(items[0] || {}) },
      );
      /* c8 ignore stop */
    }


MysticatBot nit: ?? null vs || null is cosmetically correct but doesn't alter behavior here, since Map.get never returns 0/'' for a missing key.

Acknowledged - agreed this is cosmetic-only, no behavior change intended or introduced. No further action needed.

MysticatBot

Hey @aliciadriani,

⚠ 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: Approve - prior findings addressed, no new blocking issues.
Complexity: HIGH - medium diff; API surface change (OpenAPI schema).
Changes: Adds JSDoc for the hasText defense-in-depth guard, expands the OpenAPI id-field description to document the null-id-is-never-resolvable behavior, and pins the null/empty languageCode early-return with a direct unit test (3 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI schema). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are still pending - verify before merge.

Non-blocking (1): minor issues and suggestions

suggestion: listLanguageCatalog can return entries with code but id: null that will always 400 at create time (the byCode cache only admits entries where both are present). The comment at the filter documents this asymmetry. Consider filtering out id: null entries from the catalog response in a follow-up so the UI picker never offers unresolvable options, or adding a resolvable flag - src/support/serenity/handlers/markets.js:1048

Previously flagged, now resolved

Direct unit test for null/empty languageCode early-return guard now pins behavior and confirms transport is never called
resolveLanguageId JSDoc expanded to explain the hasText guard is defense-in-depth, not redundant with caller normalization
OpenAPI id field description now documents that null id is never resolvable (400 unknownLanguage)

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 26s | Cost: $4.08 | Commit: 4709942435e3d7d9ece86bcdb5494daf1df4957f
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

…talog listLanguageCatalog kept entries with code but no usable id (id: null), which resolveLanguageId's byCode map can never admit — the UI picker could offer a language that then 400s unknownLanguage at create time. Filter those out at the source instead of just documenting the gap, so listLanguageCatalog's notion of "resolvable" matches resolveLanguageId's. id is now always a real string on every returned entry; the OpenAPI schema reflects that.

aliciadriani · 2026-09-08T11:48:56Z

-    .filter((l) => l && typeof l === 'object' && hasText(l.name))
-    .map((l) => ({ id: hasText(l.id) ? String(l.id) : null, name: String(l.name) }))
+  const usable = rawItems.filter((l) => l && typeof l === 'object' && hasText(l.name));
+  const resolvable = usable.filter((l) => hasText(l.code) && hasText(l.id));


MysticatBot suggestion: listLanguageCatalog can return entries with code but id: null that will always 400 at create time (the byCode cache only admits entries where both are present). The comment at the filter documents this asymmetry. Consider filtering out id: null entries from the catalog response in a follow-up so the UI picker never offers unresolvable options, or adding a resolvable flag.

Fixed in d16e1de - went with the stronger option (filtering, not just documenting). listLanguageCatalog now drops any entry missing a usable id, not just entries missing code, so every returned item is guaranteed resolvable via resolveLanguageId's byCode map. id is now a required, non-nullable string in the OpenAPI schema and the handler's output type. Also updated the earlier 'document only' fix (comment 3956660401) - superseded by this stronger fix.

MysticatBot

Hey @aliciadriani,

Verdict: Approve - prior suggestion addressed, no blocking issues.
Complexity: HIGH - medium diff; API surface change (OpenAPI schema).
Changes: Tightens listLanguageCatalog to drop entries missing a usable id (not just code), making SerenityLanguage.id required and non-nullable so every catalog entry is guaranteed resolvable (8 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI schema). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are still pending - verify before merge.

Non-blocking (2): minor issues and suggestions

suggestion: The endpoint description in docs/openapi/serenity-api.yaml (line ~1171) still says id (nullable), but the schema now declares id as required and non-nullable. Drop the parenthetical or change to match - docs/openapi/serenity-api.yaml:1171
nit: Stale comment above the listLanguageCatalog drops entries with a missing or blank id test still references the old ternary (id: hasText(l.id)?String(l.id):null) and old line number. The test title was updated but the comment block was not - test/support/serenity/handlers/markets.test.js:2336

Previously flagged, now resolved

listLanguageCatalog now drops entries missing a usable id (not just code), closing the picker-to-resolver asymmetry flagged in the prior review
OpenAPI id field updated from nullable to required, matching the runtime invariant

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 15s | Cost: $4.23 | Commit: d16e1dece095aebedcbe04c4389039c818c77551
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

…ruth

- serenity-api.yaml: drop the stale "(nullable)" on the languages endpoint description now that SerenityLanguage.id is required. - markets.test.js: replace the stale comment referencing the old id:null ternary and line number with one matching the current drop-on-missing-id filter behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

aliciadriani · 2026-09-09T08:29:23Z

-      and the English `name` used as the resolution key. Tolerates a 404/405
-      from the upstream by returning an empty list. Org access is enforced; IMS
-      authentication is required.
+      rejected at create time). Each item carries the catalog `id`,


Fixed in 00cb4b1: dropped the stale (nullable) parenthetical from the endpoint description — SerenityLanguage.id is required and non-nullable in the schema, so the description no longer contradicts it.

aliciadriani · 2026-09-09T08:29:24Z

-  // when a language item has a blank or missing `id` field. listLanguageCatalog
-  // keeps such rows (they have a valid name) and maps id to null.
-  it('listLanguageCatalog maps a language item with missing id to null', async () => {
+  // listLanguageCatalog drops any entry with a blank or missing `id` entirely —


Fixed in 00cb4b1: replaced the stale comment that referenced the old id: hasText(l.id)?String(l.id):null ternary and its old line number with one describing the current behavior — listLanguageCatalog drops (not nulls) any entry with a blank or missing id.

…ruth

adobe/spacecat-shared#1919 (which adds the BCP-47 code field this PR consumes) merged and published as 1.23.0. The it-postgres suite was failing because the pinned 1.22.0 mock image predates that field, so listLanguageCatalog's resolvable-entry filter dropped every catalog row (GET /serenity/languages returned an empty list) and every downstream /serenity/markets and /serenity/tags IT case 400/404'd. Bumping the dependency also bumps the lockstep mock image tag (SERENITY_PE_MOCK_TAG) to 1.23.0, which carries the code field. Verified locally: npm test (full suite), npx mocha against test/it/postgres/serenity.test.js (81 passing), npm run type-check, npm run docs:lint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ruth

## [1.799.3](v1.799.2...v1.799.3) (2026-09-09) ### Bug Fixes * **serenity:** resolve languageCode by Semrush BCP-47 code, not English name ([#3221](#3221)) ([23aef8c](23aef8c)), closes [#3183](#3183) [Hi#level](https://github.com/Hi/issues/level) [#1919](#1919) [#1919](#1919) [#1919](#1919) [#1919](#1919)

solaris007 · 2026-09-09T10:16:55Z

🎉 This PR is included in version 1.799.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

aliciadriani requested a review from MysticatBot September 7, 2026 15:20

aliciadriani self-assigned this Sep 7, 2026

aliciadriani commented Sep 7, 2026

View reviewed changes

MysticatBot requested changes Sep 7, 2026

View reviewed changes

aliciadriani added 2 commits September 7, 2026 19:22

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

483c5c3

…ruth

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

2f1cdfb

…ruth

aliciadriani requested a review from MysticatBot September 8, 2026 08:05

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

946d22d

…ruth

aliciadriani requested review from MysticatBot and removed request for MysticatBot September 8, 2026 08:12

MysticatBot requested changes Sep 8, 2026

View reviewed changes

MysticatBot added ai-reviewed Reviewed by AI complexity:medium AI-assessed PR complexity: MEDIUM labels Sep 8, 2026

MysticatBot requested changes Sep 8, 2026

View reviewed changes

MysticatBot added needs-human-review AI reviewer recommends a human read before merge complexity:high AI-assessed PR complexity: HIGH and removed complexity:medium AI-assessed PR complexity: MEDIUM labels Sep 8, 2026

aliciadriani commented Sep 8, 2026

View reviewed changes

aliciadriani requested a review from MysticatBot September 8, 2026 08:45

MysticatBot added the complexity:high AI-assessed PR complexity: HIGH label Sep 8, 2026

aliciadriani commented Sep 8, 2026

View reviewed changes

aliciadriani requested a review from MysticatBot September 8, 2026 10:01

MysticatBot approved these changes Sep 8, 2026

View reviewed changes

aliciadriani requested a review from MysticatBot September 8, 2026 11:43

aliciadriani commented Sep 8, 2026

View reviewed changes

aliciadriani requested review from MysticatBot and removed request for MysticatBot September 8, 2026 11:49

MysticatBot approved these changes Sep 8, 2026

View reviewed changes

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

dc59b46

…ruth

aliciadriani requested a review from MysticatBot September 9, 2026 06:52

aliciadriani and others added 2 commits September 9, 2026 09:04

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

e10b489

…ruth

aliciadriani commented Sep 9, 2026

View reviewed changes

aliciadriani and others added 4 commits September 9, 2026 10:33

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

150d5ee

…ruth

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

dfa2825

…ruth

Merge branch 'main' into LLMO-7420-semrush-bcp47-language-source-of-t…

716017a

…ruth

aliciadriani temporarily deployed to dev-branches September 9, 2026 09:15 — with GitHub Actions Inactive

aliciadriani merged commit 23aef8c into main Sep 9, 2026
22 checks passed

aliciadriani deleted the LLMO-7420-semrush-bcp47-language-source-of-truth branch September 9, 2026 10:03

solaris007 added the released label Sep 9, 2026

		@@ -338,7 +338,7 @@ describe('handlers/markets.js — handleCreateMarket', () => {
		dataAccess.BrandSemrushProject.create.resolves();

Uh oh!

Conversation

aliciadriani commented Sep 7, 2026 • edited by vkublytskyi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Abstract

2. Reasoning

3. High-level overview of the changes

4. Required information

6. Affected / used mysticat-workspace projects

8. Test plan

9. Deployment & merge order

Change Management

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

codecov Bot commented Sep 7, 2026

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MysticatBot left a comment

Choose a reason for hiding this comment

Uh oh!

MysticatBot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Must fix before merge

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MysticatBot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Must fix before merge

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aliciadriani commented Sep 7, 2026 •

edited by vkublytskyi

Loading

MysticatBot left a comment •

edited

Loading

MysticatBot left a comment •

edited

Loading

MysticatBot left a comment •

edited

Loading

MysticatBot left a comment •

edited

Loading