feat: bind Semrush markets to SpaceCat Site entities (LLMO-6405)

[igor-grubic]

What

Phase 2 of the brand/market split (LLMO-6405): bind Semrush markets to SpaceCat Site entities. Builds on #2878 (merged).

All changes are additive and backward-compatible — the siteId-absent path is byte-for-byte unchanged, so existing callers (and the current UI) keep working.

Changes

• Markets DTO exposes siteId (nullable), both modes. Flat rows read it off the mapping row; sub-workspace slices are enriched from the brand's mapping rows via a best-effort index (buildMarketSiteIdIndex) — a failed/absent read just leaves siteId: null, never throws.
• POST /serenity/markets accepts siteId OR brandDomain. When only siteId is supplied, the server derives the domain (resolveSiteDomain) and links that Site directly (ensureMarketSite siteId fast-path, same-org guarded — skips the domain→Site find-or-create). A supplied-but-unresolvable siteId is a hard 400.
• Market DELETE cleans up an orphaned link (R12). Reference-counts the brand's live mapping rows and removes the brand_sites type='serenity' link only when the deleted market was the last one on that site and it is not the brand's primary site. The Site entity is never deleted; the primary site is never unlinked. Best-effort — never fails the 204; a failed primary-site lookup fail-safes to skip.
• tombstoneMappingRow returns the deleted row's siteId (captured pre-tombstone, one read); new lightweight getBrandBaseSiteId reads the brand's primary site for the guard.
• OpenAPI: SerenityMarket.siteId + SerenityCreateMarketRequest.siteId (one-of with brandDomain); docs/serenity.md updated.

Composes with the UI

Wire contract matches project-elmo-ui PR #2493: SerenityMarket.siteId?: string | null and the create input's optional siteId (sent alongside brandDomain).

Testing

• npm run type-check, npm run lint, npm test (unit + coverage gate), npm run docs:lint — all green locally. Changed-file coverage: site-linkage.js 100%/98.8%, markets-subworkspace.js 100%/96.7%, markets.js 99.7%/96.7%.
• Integration tests added to test/it/shared/tests/serenity.js (create-via-siteId, delete cleanup, siteId DTO round-trip). The Postgres IT suite needs Docker/ECR and runs on CI, not locally.

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[MysticatBot]

Mysticat review failed: Claude CLI crashed (exit 1): stderr= stdout={"type":"result","subtype":"error_max_budget_usd","duration_ms":3,"duration_api_ms":1197487,"is_error":true,"num_turns":1,"stop_reason":null,"session_id":"88943dcb-fd2b-4dc1-b85a-db6238e100b1","total_cost_usd":10.14522645,"usage":{"input_tokens":0,"cache_creation_input_tokens":0,"cache_read_input_to

[MysticatBot]

Hey @igor-grubic,

Verdict: Request changes - two issues with the fail-safe semantics and a performance concern on the single-market GET path.
Complexity: HIGH - large diff; API surface changes + database write cleanup logic.
Changes: Binds Semrush markets to SpaceCat Site entities via a new siteId parameter on the create endpoint, surfaces siteId on the market DTO, and cleans up orphaned brand_sites links on delete (16 files).

Must fix before merge

1. [Important] Fail-safe inversion in delete cleanup - src/support/brands-storage.js:309

   getBrandBaseSiteId guards !postgrestClient?.from || !hasText(brandId) but does NOT check organizationId. If organizationId is null/undefined (e.g. ctx?.params?.spaceCatId is absent), the query executes .eq("organization_id", null) which matches zero rows, returns null, and primaryResolved stays true in the controller. The orphan-unlink then proceeds with primarySiteId: null, effectively disabling the primary-site guard in unlinkMarketSiteIfOrphaned. This is the opposite of the stated fail-safe intent ("never risk removing the primary link on a transient read error").

   Fix: Either guard organizationId with the same !hasText(organizationId) early-return (return null or throw), or make the controller treat a null result from getBrandBaseSiteId when organizationId was falsy as an unresolved state (set primaryResolved = false).

2. [Important] handleGetMarketSubworkspace fetches ALL brand mapping rows to enrich a single slice - src/support/serenity/handlers/markets-subworkspace.js:180

   buildMarketSiteIdIndex calls BrandSemrushProject.allByBrandId(brandId) which loads every mapping row for the brand into memory, builds a full Map, and then does a single .get() lookup. The list endpoint amortizes this cost (N markets enriched per 1 read), but the GET-single-market endpoint pays the full read to find one value.

   For a brand with 50-100 markets (global brands with regional permutations, foreseeable at the 3-year horizon), every detail request returns 50-100 rows for one lookup. Consider a targeted single-row read: reuse the existing findBySlice(brandId, geoTargetId, languageCode) method the flat handler uses, or add a point-read for the sub-workspace path.

Non-blocking (4): minor issues and suggestions

• nit: Redundant !derivedDomain || !hasText(derivedDomain) double-check; either condition alone suffices - src/controllers/serenity.js:695
• suggestion: Add isValidUUID validation on siteId in the controller for defense-in-depth (PostgREST parameterizes, so no injection risk, but early rejection is cleaner) - src/controllers/serenity.js:689
• suggestion: OpenAPI schema does not express the "at least one of brandDomain or siteId" constraint formally; the prose description covers it, but code-gen consumers won't see the rule - docs/openapi/schemas.yaml
• suggestion: handleListMarketsSubworkspace signature grew to 5 positional args; a trailing options object ({ dataAccess, log }) would be easier to extend - src/support/serenity/handlers/markets-subworkspace.js

---

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

[igor-grubic]

Thanks for the review! Addressed both must-fix items in 8c3e7c0c0:

1. Fail-safe inversion in delete cleanup (getBrandBaseSiteId) — fixed. It no longer returns null when it can't scope the query; it now throws when organizationId / brandId / the postgrest client is missing. Returning null was ambiguous with a successfully-resolved "brand has no primary site" and let the orphan-unlink proceed with primarySiteId: null, disabling the primary-site guard. The controller's existing try/catch now treats an unresolved primary as "skip the unlink" — matching the stated intent. Added unit coverage for the three throw cases.

2. handleGetMarketSubworkspace loading all mapping rows — fixed. The single-market GET now point-reads just that slice's row via BrandSemrushProject.findBySlice(brandId, geoTargetId, languageCode) (the same finder the flat/tags/prompts handlers use) instead of buildMarketSiteIdIndex (which loads every brand mapping row). The list endpoint keeps the amortized index. Added point-read unit cases (found / tombstoned / read-fails).

On the non-blocking items:

• Redundant !derivedDomain || !hasText(derivedDomain) — kept intentionally. This directory type-checks under a blocking tsc gate, and hasText is typed (str: string) => boolean (not a type guard), so the !derivedDomain || is what narrows string | null → string for hasText. Removing it fails npm run type-check (documented in src/support/serenity/CLAUDE.md).
• isValidUUID on siteId — reasonable; a malformed siteId is already handled gracefully today (unresolvable → 400 on create, best-effort skip on the mirror), so deferring as polish.
• OpenAPI oneOf for brandDomain/siteId & options-object for the 5-arg signature — agreed, deferred as non-blocking polish.

[MysticatBot]

Hey @igor-grubic,

Verdict: Approve - both prior blocking findings are addressed correctly; no new issues introduced.
Complexity: HIGH - large diff; API surface changes + database write cleanup logic.
Changes: Binds Semrush markets to SpaceCat Site entities via a new siteId parameter on the create endpoint, surfaces siteId on the market DTO, and cleans up orphaned brand_sites links on delete (16 files).
Note: Recommend a human read before merge - this change modifies a shared contract (docs/openapi/schemas.yaml). The bot review is a complement to, not a replacement for, a human read here.

Non-blocking (1): minor issues and suggestions

• suggestion: Add a test for findBySlice resolving to null (no mapping row exists yet) in handleGetMarketSubworkspace - the tombstoned/throws/no-dataAccess branches are covered but the simple null-row path is not explicitly exercised - test/support/serenity/handlers/markets-subworkspace.test.js

Previously flagged, now resolved

• getBrandBaseSiteId fail-safe inversion fixed: now throws on missing organizationId, controller catch correctly sets primaryResolved = false and skips the unlink.
• handleGetMarketSubworkspace performance fixed: single-market GET now does a point-read via findBySlice instead of loading all brand mapping rows.

---

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

[solaris007]

🎉 This PR is included in version 1.680.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀