feat(project-engine-client): add BCP-47 code to the language catalog - #1919
feat(project-engine-client): add BCP-47 code to the language catalog#1919aliciadriani wants to merge 6 commits into
Conversation
GET /v1/languages returns a BCP-47 code per catalog entry (verified 2026-09-07); add it via a spec overlay CR so LLMO-7420's Brand Markets rework can resolve languageCode by code instead of English-name matching. Aligns the mock catalog/factory to the corrected contract.
|
This PR will trigger a minor release when merged. |
…ed code field
Two e2e assertions asserted the read-view language shape as { id, name },
missing the now-required code field, so they'd fail once code became a
real property on every LanguageResponse. Only caught by running the e2e
suite directly (MOCK_E2E=1) — not part of the default npm test run.
| // The created read-view carries the live shapes the consumer's langOf/geoOf reconstruct a | ||
| // market from: language.name is the ISO code (NOT the English display name), location echoed. | ||
| expect(created.settings.ai.language).to.deep.equal({ id: ENGLISH_ID, name: 'en' }); | ||
| expect(created.settings.ai.language).to.deep.equal({ id: ENGLISH_ID, name: 'en', code: 'en' }); |
There was a problem hiding this comment.
Must Fix: Two e2e assertions (listLanguages returns the full live language taxonomy at what is now line ~2094, and this market round-trip test) asserted the read-view language shape as { id, name } without code. Since model.LanguageResponse now requires code, both failed when run directly (MOCK_E2E=1 npx mocha test/e2e/project-engine-mock.e2e.js) — this suite isn't part of the default npm test run, so it wasn't caught before push.
There was a problem hiding this comment.
Fixed in 0646e9d — both assertions now include code: 'en'. Verified: MOCK_E2E=1 npx mocha --no-package --reporter dot --timeout 60000 test/e2e/project-engine-mock.e2e.js → 117 passing, 0 failing.
| * - `code` — the BCP-47 language code `GET /v1/languages` returns per entry (e.g. `zh-Hans`, | ||
| * `zh-Hant`, `fil`) — the stable identifier `languageCode` is resolved against | ||
| * (LLMO-7420, replacing the old English-name-matching approach). Live-verified for | ||
| * `zh-Hans`/`zh-Hant`/`fil` (rainer-friederich, 2026-09-07); the rest are best-effort |
There was a problem hiding this comment.
Should Fix (non-blocking): 35 of the 38 code values are best-effort guesses (same value as the existing iso column), live-verified only for zh-Hans/zh-Hant/fil. If Semrush's real BCP-47 code for any of the other 35 languages differs from the guess (plausible for macrolanguages/region variants, e.g. Norwegian could be nb not no), local dev and CI against this mock will diverge silently from production - a resolver bug for that specific language could pass every local/CI check and only surface live. Worth a full catalog re-capture against the live GET /v1/languages at some point; not blocking this PR since the doc comments are honest about the caveat and the 3 languages the ticket is scoped around are verified.
There was a problem hiding this comment.
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 test coverage gaps on the new resolution-key contract.
Complexity: MEDIUM - medium diff, single package.
Changes: Adds BCP-47 code field to the project-engine-client language catalog contract via spec overlay, generated types, mock data, and factory updates (9 files).
Must fix before merge
- [Important] Missing
codeuniqueness test in catalog -test/mock/language-catalog.test.js(details inline) - [Important] No test covers the
buildAiSettingscode/iso divergence for Chinese -mock/factories.js:227(details inline)
Non-blocking (3): minor issues and suggestions
- nit:
namepromoted from optional to required alongsidecode(in the CR5 overlay update) without explicit callout in the PR description - worth noting for downstream TypeScript consumers even though the tightening is safe in practice -spec/overlays/corrections.yaml:317 - suggestion: Loop assertions in the catalog structural test lack per-entry failure context - adding a message parameter (e.g.,
expect(entry.code, entry.name + ' code').to.be...) would make failures diagnostic -test/mock/language-catalog.test.js - nit: The CR26 top comment references "CR5 marks it required" but does not note this PR expanded CR5's required list from
[id]to[id, name, code]-spec/overlays/corrections.yaml:160
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 7m 41s | Cost: $5.31 | Commit: 0646e9df76618cee0732546629765ca33f893c61
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| for (const entry of LANGUAGE_CATALOG) { | ||
| expect(entry.id).to.match(/^[0-9a-f-]{36}$/); | ||
| expect(entry.name).to.be.a('string').with.length.greaterThan(0); | ||
| expect(entry.code).to.be.a('string').with.length.greaterThan(0); |
There was a problem hiding this comment.
issue (blocking): Missing code uniqueness test. The existing test suite has an 'ids are unique' test for id, which guards against reverse-index collisions. The new code field is the resolution key for LLMO-7420 - resolveLanguageId will resolve a BCP-47 code to a UUID. If two catalog entries share a code, resolution is ambiguous. There is no uniqueness assertion for code. This matters especially because BCP-47 codes could plausibly collide if the catalog grows (e.g., regional variants like pt-BR vs pt).
Add a sibling uniqueness test, same pattern as the existing id check:
it('codes are unique (BCP-47 resolution requires a 1:1 code->id mapping)', () => {
const codes = LANGUAGE_CATALOG.map((l) => l.code);
expect(new Set(codes).size).to.equal(codes.length);
});There was a problem hiding this comment.
Fixed in b1fc901 — added a codes-are-unique test mirroring the existing ids-are-unique check. Verified: npm test → 377 passing, 0 failing (up from 375).
| language: createLanguageMock({ | ||
| id: request.language_id ?? '', | ||
| name: isoForLanguageId(request.language_id), | ||
| code: isoForLanguageId(request.language_id), |
There was a problem hiding this comment.
issue (blocking): No test covers the buildAiSettings code/iso divergence for Chinese. buildAiSettings sets code: isoForLanguageId(request.language_id), reusing the ISO resolver. For Chinese Simplified (UUID 728bef4c-...), isoForLanguageId returns 'zh', but the catalog BCP-47 code is 'zh-Hans'. The comment in the diff calls this "best-effort" and "unverified for code" - but no test exercises createProjectResponseFromRequest with a Chinese language_id to document this known divergence.
If a downstream consumer of the mock starts relying on language.code from the read-view (which LLMO-7420 is about), their tests would pass against the mock but fail against production for Chinese.
Add a test that pins the known limitation:
it('read-view language.code is best-effort (isoForLanguageId), NOT the catalog BCP-47 code', () => {
const p = createProjectResponseFromRequest({
language_id: '728bef4c-94cf-4e14-bc06-56534751c71a',
country_code: 'cn',
});
expect(p.settings.ai.language.code).to.equal('zh');
});Alternatively, add a proper codeForLanguageId resolver (parallel to isoForLanguageId) so the embedded read-view carries the correct BCP-47 code.
There was a problem hiding this comment.
Fixed in b1fc901 — added a test pinning the known divergence: settings.ai.language.code for Chinese Simplified reuses isoForLanguageId's 'zh', not the catalog's 'zh-Hans'. Left the underlying best-effort behavior as-is (per the doc comment, this embedded read-view field is genuinely unverified for code, unlike the GET /v1/languages catalog list) rather than adding a codeForLanguageId resolver, since fabricating a value with no live confirmation would misrepresent the mock's faithfulness bar more than the current honest divergence does.
MysticatBot
added
ai-reviewed
…ness + Chinese divergence pin - Add a codes-are-unique test mirroring the existing ids-are-unique check (BCP-47 resolution needs a 1:1 code -> id mapping). - Add a test pinning the known buildAiSettings Chinese code/iso divergence (settings.ai.language.code reuses isoForLanguageId's 'zh', not the catalog's 'zh-Hans') so a future change to that behavior is deliberate. - Add per-entry failure messages to the catalog structural test's loop assertions. - Clarify the CR26/CR5 overlay comment: this PR is what first gives model.LanguageResponse a CR5 required-fields entry at all.
…hub.com/adobe/spacecat-shared into LLMO-7420-language-code-source-of-truth
| # a best-effort value there too (same treatment `language.name` already gets in that context — | ||
| # see factories.js `buildAiSettings`), so this required bar holds everywhere the schema serves. | ||
| update: | ||
| required: [id, name, code] |
There was a problem hiding this comment.
MysticatBot nit (from review summary): name was promoted from optional to required alongside code in this CR5 entry, without an explicit callout in the PR description for downstream TypeScript consumers - even though the tightening is safe in practice (the mock's create handlers already always populate name).
There was a problem hiding this comment.
Addressed by updating the PR description (section 3) to explicitly call out that this CR5 entry also promotes name from optional to required, not just adding code.
| # serenity-docs `category-delete-implementation-plan.md` WP0 gate 2). Mark it optional so a | ||
| # caller can omit it rather than sending a meaningless placeholder value. Remove if Semrush | ||
| # documents (or starts enforcing) real per-prompt scoping. | ||
| # CR26 model.LanguageResponse carries a BCP-47 `code`. GET /v1/languages returns a `code` field |
There was a problem hiding this comment.
MysticatBot nit (from review summary): this CR26 top comment referenced 'CR5 marks it required' without noting that this PR is what first gives model.LanguageResponse a CR5 required-fields entry at all (there was no prior entry for this schema).
There was a problem hiding this comment.
Fixed in b1fc901 — the comment now explicitly states this schema had no CR5 entry before this PR added one.
| expect(entry.iso).to.be.a('string').with.length.greaterThan(0); | ||
| expect(entry.id, `${entry.name} id`).to.match(/^[0-9a-f-]{36}$/); | ||
| expect(entry.name, `${entry.id} name`).to.be.a('string').with.length.greaterThan(0); | ||
| expect(entry.code, `${entry.name} code`).to.be.a('string').with.length.greaterThan(0); |
There was a problem hiding this comment.
MysticatBot suggestion (from review summary): loop assertions in this structural test lacked per-entry failure context, making a failure hard to diagnose (which of the 38 entries failed, and on which field).
There was a problem hiding this comment.
Fixed in b1fc901 — each assertion now carries a message identifying the entry (by name or id) and the failing field, e.g. ${entry.name} code.
MysticatBot
left a comment
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - clean, well-scoped contract addition with thorough documentation and all prior findings addressed.
Complexity: MEDIUM - medium diff, single package.
Changes: Adds BCP-47 code field to the project-engine-client language catalog contract via spec overlay CR26, regenerated types, mock data, factory updates, and comprehensive tests (9 files).
Non-blocking (4): minor issues and suggestions
- suggestion: Add
createLanguageMocktotest/types/factories.type-test.tsfor direct compile-time shape enforcement. The factory is transitively covered viacreateProjectResponseFromRequest(which the type-test already checks), but every other entity factory has a direct assignment line in that file. Four lines close the gap -packages/spacecat-shared-project-engine-client/test/types/factories.type-test.ts - suggestion: The e2e
listLanguagestest verifies the English entry but not any of the three live-verified Chinese/Filipino BCP-47 codes (zh-Hans,zh-Hant,fil). A spot-check of one Chinese entry would strengthen the end-to-end contract for the entries wherecodediffers fromiso-packages/spacecat-shared-project-engine-client/test/e2e/project-engine-mock.e2e.js:2093 - nit:
CATALOG_CAPTUREDis still2026-06-25but thecodecolumn was added from a 2026-09-07 live probe. Consider a comment or companion constant noting thecodecolumn was populated separately -packages/spacecat-shared-project-engine-client/mock/language-catalog.js:54 - suggestion: CR26 action-site comment block (lines 347-356 in the overlay) largely duplicates the CR26 index entry above (lines 157-163). The other CRs keep their action-site comment to one line referencing the index -
packages/spacecat-shared-project-engine-client/spec/overlays/corrections.yaml:347
Previously flagged, now resolved
- Code uniqueness test added (codes-are-unique, mirroring ids-are-unique)
- Chinese divergence test pinning the known
zhvszh-Hansdifference inbuildAiSettings - PR description updated to call out
namepromotion from optional to required - Loop assertions now carry per-entry identifying messages
- CR26 comment clarified that the CR5 entry is new to this PR
- Add createLanguageMock to the compile-time factories.type-test.ts shape-enforcement list, matching every other entity factory. - e2e listLanguages now spot-checks a live-verified code/iso divergence entry (Chinese Simplified), not just English. - Document that CATALOG_CAPTURED intentionally stays pinned to the id/name capture date and does not cover the later, partial code addition. - Trim CR26's action-site comment, which duplicated the CR26 index entry above almost verbatim.
| const brandUrl: BrandUrl = createBrandUrlMock(); | ||
| // 1d-lang. the language catalog factory (the GET /v1/languages list item, LLMO-7420 — code is | ||
| // required alongside id/name on model.LanguageResponse). | ||
| const language: Language = createLanguageMock({ code: 'de', name: 'German' }); |
There was a problem hiding this comment.
MysticatBot suggestion (round 2): createLanguageMock had no direct compile-time shape-enforcement line in this file, unlike every other entity factory.
There was a problem hiding this comment.
Fixed in 6a88752 — added a direct createLanguageMock -> Language assignment line, matching every other entity factory in this file.
| }); | ||
| // Spot-check a live-verified entry where code diverges from the mock-only iso column | ||
| // (rainer-friederich, 2026-09-07) — the whole point of LLMO-7420 is resolving by this code. | ||
| expect(data.items).to.deep.include({ |
There was a problem hiding this comment.
MysticatBot suggestion (round 2): this e2e test only spot-checked the English entry, not any of the three live-verified entries where code diverges from iso (zh-Hans/zh-Hant/fil) - the exact case LLMO-7420 exists for.
There was a problem hiding this comment.
Fixed in 6a88752 - added a deep.include assertion for the Chinese Simplified entry (id 728bef4c..., code zh-Hans) alongside the existing English check.
| * from the live taxonomy is discoverable/greppable rather than buried in prose. Bump it when the | ||
| * catalog is re-captured. | ||
| * | ||
| * `code` was NOT captured on this date — it was added later (2026-09-07, LLMO-7420), live-verified |
There was a problem hiding this comment.
MysticatBot nit (round 2): CATALOG_CAPTURED is still 2026-06-25 but the code column was added from a separate, later (2026-09-07) live probe - worth a comment noting the two dates cover different columns.
There was a problem hiding this comment.
Fixed in 6a88752 - added a doc-comment paragraph explaining CATALOG_CAPTURED intentionally stays pinned to the id/name capture date and does not cover the later, partial code addition.
| # ── CR26: model.LanguageResponse carries a BCP-47 `code` — see the CR26 note above for the | ||
| # live-verification detail; not repeated here. | ||
| - target: "$.components.schemas['model.LanguageResponse'].properties" | ||
| description: "CR26: add the code (BCP-47) field to the language catalog entry" |
There was a problem hiding this comment.
MysticatBot suggestion (round 2): the CR26 action-site comment block duplicated the CR26 index entry above almost verbatim.
There was a problem hiding this comment.
Fixed in 6a88752 - trimmed the action-site comment to a single line pointing back to the CR26 index entry instead of restating the live-verification detail.
There was a problem hiding this comment.
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 - all four prior suggestions addressed cleanly, no new issues.
Complexity: MEDIUM - medium diff, single package.
Changes: Addresses the four non-blocking suggestions from the prior review - adds createLanguageMock type-test, Chinese Simplified e2e spot-check, CATALOG_CAPTURED date scope comment, and trimmed CR26 action-site comment (4 files).
Non-blocking (0): no issues or suggestions
No findings on this incremental commit.
Previously flagged, now resolved
- createLanguageMock now has a direct compile-time type-test in factories.type-test.ts
- e2e listLanguages test now spot-checks Chinese Simplified (zh-Hans) alongside English
- CATALOG_CAPTURED doc comment explains the code column was added later and stays pinned to the id/name capture date
- CR26 action-site comment trimmed to a single back-reference line
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 43s | Cost: $4.90 | Commit: 6a887525d96db25dd78e9fcdc269f1c619461401
If this code review was useful, please react with 👍. Otherwise, react with 👎.
2 participants
1. Abstract
Adds the BCP-47
codefield Semrush'sGET /v1/languagesnow returns to the project-engine-client's language catalog contract.2. Reasoning
Brand Markets language resolution (spacecat-api-service, project-elmo-ui) currently matches Semrush's English catalog
nameagainst an English-name derived from the storedlanguageCode, which silently breaks on catalog renames (e.g. "Chinese Simplified" vs "Simplified Chinese") or aliases (Filipino/Tagalog) — the root cause behind LLMO-7309's three-commit workaround PR. A live probe against Semrush Project Engine (2026-09-07) confirmedGET /v1/languagesnow returns a stable BCP-47codeper catalog entry. This PR is the first of three companion changes (LLMO-7420) making thatcodethe resolution key end-to-end; it lands the corrected contract in this client so the consuming repos can build on it.3. High-level overview of the changes
code: stringonmodel.LanguageResponse— the vendored swagger only documentsid/name, so the overlay mechanism (this package's standard way to model a live-but-undocumented field) captures the new field and marks itrequiredfor the catalog-list context.src/generated/types.tssoLanguageResponsenow typesid,name, andcode(previouslyid/nameoptional;codeis now required alongside them).mock/language-catalog.js) with acodecolumn carrying each entry's BCP-47 code — live-verified forzh-Hans/zh-Hant/fil, best-effort (same value as the existing mock-onlyisocolumn) for the rest.codeandisoare documented as answering different questions and are NOT interchangeable:isois the mock's stand-in for the separate, unrelated project-read-view field.GET /v1/languagesroute handler and thecreateLanguageMockfactory to carrycodethrough.model.LanguageResponsealso promotesnamefrom optional to required (previously this schema had no CR5 entry at all). This is safe in practice — the mock's create handlers already always populatename— but it's a type-level tightening beyond just addingcode, called out here per MysticatBot review feedback.4. Required information
LLMO-7420-semrush-bcp47-language-source-of-truth, adobe/project-elmo-uiLLMO-7420-semrush-bcp47-language-source-of-truth6. Affected / used mysticat-workspace projects
listLanguages()contract; its LLMO-7420 companion PR resolveslanguageCodeby the newcodefield once this publishes.codefield indirectly via spacecat-api-service's/serenity/languagespassthrough; its LLMO-7420 companion PR builds the Brand Markets language picker from it.8. Test plan
(a) Ran this package's full unit suite, the type-check (
npm run test:types), and the spec-overlay freshness gate (test/overlay.test.js) locally — all exercise the newcodefield end-to-end through the mock's Counterfact handler and the create-response's embeddedsettings.ai.languageshape.(b) No environment-specific verification applies — this package ships nothing runtime-deployable on its own; it takes effect once published and consumed by the companion PRs.
9. Deployment & merge order
LLMO-7420-semrush-bcp47-language-source-of-truth(needs this package's publishedcodefield to bump its dependency) and adobe/project-elmo-uiLLMO-7420-semrush-bcp47-language-source-of-truth(consumes the field transitively).🤖 Generated with review-kit