feat(serenity): add market detail endpoint exposing semrushProjectId (LLMO-5190 follow-up)

[rainer-friederich]

Related Issues

Follow-up to LLMO-5190 (PR 2506, "hide Semrush as an implementation detail"). Epic LLMO-5005. No new ticket filed yet.

Problem

LLMO-5190 dropped semrushProjectId from the public /serenity/* DTOs and now resolves the upstream project server-side from the (brandId, geoTargetId, languageCode) slice. That removed the value a UI consumer used to read off serenityProjectsApi.projects[].semrushProjectId.

The embedded Semrush AIO renderer MFE needs the resolved upstream project id to mount the dashboard for the selected market — a use case the abstraction did not anticipate (it assumed the Prompts Management page only needs list/CRUD, not a Semrush-rendered dashboard). With the new surface there is no way for the client to obtain the id.

Approach — Option B (dedicated detail endpoint), not a field on the list

GET /v2/orgs/:spaceCatId/brands/:brandId/serenity/markets/:geoTargetId/:languageCode
  -> 200 SerenityMarketDetail  (SerenityMarket + semrushProjectId)
  -> 404 marketNotFound        (slice has no row — addressing a specific missing resource is not an empty success, unlike the list)
  -> 400                       (malformed slice; same strict /^\d+$/ geo guard as deleteMarket)
  IMS-only, organization:read

Two options were weighed:

• A — re-add semrushProjectId to the /serenity/markets list item. Minimal, no extra round trip, but reverses LLMO-5190 on the most-used surface and invites every consumer to couple to the id.
• B — a single, explicit detail endpoint. Keeps the list provider-free, contains the one unavoidable Semrush leak (embedding the provider's own widget genuinely needs the provider's id) to one named, deprecable route, and follows the standard list=summary / get-one=full-representation split. Costs one keyed DB lookup per market selection (negligible).

B wins long-run: it preserves the abstraction the team just built and isolates the leak. Reuses the existing v2-serenity-market-by-slice path object (already had the :geoTargetId/:languageCode params and DELETE). The field keeps the honest semrushProjectId name — the value genuinely is Semrush's project UUID (same rule LLMO-5190 applied to semrushPromptId).

Deviation from the abstraction spec — documented

LLMO-5190 states semrushProjectId is a routing detail to drop from public DTOs. Re-exposing it is a deliberate, scoped deviation, recorded with rationale in:

• docs/specs/2026-05-28-prompts-api-abstraction.md section 11
• docs/specs/2026-05-29-serenity-market-detail-endpoint.md (full analysis, contract, validation gates)

Checklist

• Problem described above; linked to LLMO-5190 / LLMO-5005.
• API spec updated: get: operation (operationId: getSerenityMarket) on v2-serenity-market-by-slice; new SerenityMarketDetail schema (allOf of SerenityMarket + semrushProjectId).
• Endpoint is implemented (not a stub) — no "Not implemented yet" / 501 needed.
• Note on examples: the entire /serenity/* surface is schema-only (no examples.yaml entries); this endpoint follows that established convention. The OpenAPI contract test AJV-validates the 200 body against SerenityMarketDetail instead.

Tests & validation (all green locally)

• test/support/serenity/handlers/markets.test.js — handleGetMarket (400 invalid geo, 400 malformed lang, 404 marketNotFound, happy-path detail).
• test/controllers/serenity.test.js — getMarket (forwarding + ok(), strict-digit null-route, 404 token mapping, IMS-only 401).
• test/openapi-contract/serenity-api.test.js — operationId↔fixture parity gate + getSerenityMarket body conforms to SerenityMarketDetail.
• test/it/shared/tests/serenity.js — GET market-detail IMS-only auth gate (mirrors the DELETE gate).
• Gates run: lint, docs:lint, docs:build, handler/controller/contract mocha suites. The build (Lambda bundle) gate runs in CI — locally it stops at deploy-parameter resolution (${env.VPC_SUBNET_1}), an env gap, not a code issue; this change touches no bundle-layer concerns (no new assets, FS-at-boot reads, hlx.static, or top-level side-effects).

Follow-up (separate PR, separate repo)

project-elmo-ui: add a getSerenityMarket(orgId, brandId, geoTargetId, languageCode) client + hook and feed semrushProjectId to the renderer MFE. Lives on the elmo serenity feature branch (PR 1815), not on main.

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[MysticatBot-Dev]

Hey @rainer-friederich,

Strengths

• Deliberate abstraction containment - the decision to expose the provider id at a single, named detail endpoint (Option B) rather than re-polluting the list is architecturally sound. It preserves the LLMO-5190 investment, contains the leak to one deprecable route, and documents the deviation with rationale. This is how you handle an unavoidable abstraction breach.
• Reuse of existing path object (v2-serenity-market-by-slice) - adding get: alongside the existing delete: avoids path proliferation and keeps the URL space resource-oriented.
• Pure DB read with no transport dependency (src/support/serenity/handlers/markets.js:157-209) - the handler takes only dataAccess and the slice coordinates. No upstream failure modes, no rate limiting concerns, near-zero operational cost.
• Brand-scoped authorization prevents IDOR (src/controllers/serenity.js:339) - brandId comes from auth.brandUuid (derived from the validated IMS token), not from a user-controlled path parameter. Callers cannot enumerate another org's market slices.
• Strict input validation with safe regex (src/controllers/serenity.js:341) - /^\d+$/ for geoTargetId and the BCP-47 pattern for languageCode are both anchored, bounded, and free of catastrophic backtracking.
• Spec-first with documented deviation - recording the deviation in the parent spec (section 11) with a pointer to the full analysis doc is exactly the right pattern for future discoverability.
• Solid test coverage across four layers - handler unit tests (validation + happy path), controller tests (forwarding + auth gate), OpenAPI contract test (schema conformance), and IT auth-gate test provide defense in depth.

Issues

Important (Should Fix)

OpenAPI spec declares 502 but the handler cannot produce one

docs/openapi/serenity-api.yaml (the get: operation on v2-serenity-market-by-slice): The response table includes a 502 "Upstream returned a non-2xx response." However, handleGetMarket makes no upstream HTTP call - it is a pure DB read via findBySlice. The only error paths are 400 (validation) and 404 (no row).

A declared 502 that can never fire misleads consumers into writing retry/fallback logic for a scenario that cannot occur. It also signals "this endpoint talks to an upstream" which is architecturally false and could confuse future maintainers about the endpoint's dependencies.

Fix: Remove the '502' response entry from the get: operation. The delete: operation on the same path legitimately has a 502 (it calls upstream Semrush), so the discrepancy would actually be clarifying.

Minor (Nice to Have)

Handler validation tests don't assert specific status codes

test/support/serenity/handlers/markets.test.js - the 400-path tests use .to.be.rejectedWith(ErrorWithStatusCode) but do not assert .status === 400. If normalizeGeoTargetId were ever to throw a different status, these tests would still pass. The 404 test already follows the stronger pattern (catching and asserting .status and .code).

Fix: Add .and.eventually.have.property('status', 400) or catch-and-assert like the 404 test does.

Recommendations

• Consider cacheability signals. This endpoint returns stable data (the Semrush project id for a market slice rarely changes). The MFE calls it every time a user selects a market. Adding a Cache-Control response header or documenting expected caching behavior would reduce per-selection round trips and make Option B's extra request effectively free for repeat visits.

• DTO extraction if the response grows. The handler returns a plain object literal rather than going through a DTO class. This is consistent with the existing serenity domain pattern (no src/dto/serenity*.js exists), and the spirit of the DTO rule is satisfied (fields are explicitly picked via getters, not raw model exposure). If the detail response gains more fields over time, extracting a formal DTO would keep the serialization contract testable in isolation.

Assessment

Ready to merge? With fixes
Reasoning: Well-structured, follows existing patterns closely, solid test coverage, and the security posture is clean. The one Important finding (phantom 502 in the OpenAPI spec) is a contract mismatch that misleads consumers but does not affect runtime behavior. A one-line removal resolves it.

Next Steps

1. Remove the 502 response from the get: operation in docs/openapi/serenity-api.yaml.
2. Optionally tighten the handler-level 400 test assertions.

---

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

[rainer-friederich]

Thanks @MysticatBot-Dev — addressed in a022f4b9.

Important — phantom 502 in getSerenityMarket: Agreed, that was a copy-over from the sibling delete:. handleGetMarket is a pure DB read (findBySlice) with no upstream call, so 502 can never fire. Removed it; only 400/404/500 remain, and I left an inline comment in the spec noting the delete: legitimately keeps its 502.

Minor — 400-path test assertions: Both handleGetMarket 400 tests now catch-and-assert status === 400, matching the stronger 404 test pattern.

On the two recommendations:

• Cache-Control: Deliberately not adding it here. The rest of the serenity GET surface (listMarkets, tags, models) sets no cache headers, and a caching policy is better applied surface-wide than bolted onto this one route. The MFE already memoizes per (geoTargetId, languageCode) slice client-side, so the repeat-selection cost is effectively nil. Happy to file a follow-up if we want explicit cache semantics across /serenity/*.
• DTO extraction: Agreed it's not warranted yet — fields are explicitly picked via getters (the spirit of the DTO rule), and no src/dto/serenity*.js exists. If the detail response grows beyond the slice + project id, I'll extract a formal DTO then.

Re-requesting review.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[MysticatBot-Dev]

Hey @rainer-friederich,

Re-review after commit a022f4b - both prior findings are addressed. Clean approval.

Strengths

• Previously flagged issues resolved cleanly. The phantom 502 is removed from the OpenAPI spec with a clarifying inline comment explaining why the sibling delete: keeps its 502 (docs/openapi/serenity-api.yaml:358-360). The handler-level 400 tests now use catch-and-assert with explicit expect(err.status).to.equal(400) (test/support/serenity/handlers/markets.test.js:694-700), matching the stronger 404 test pattern.
• Deliberate abstraction containment - the decision to expose the provider id at exactly one named detail endpoint (Option B) rather than re-polluting the list preserves the LLMO-5190 investment, contains the leak to one deprecable route, and documents the deviation with rationale in section 11 of the parent spec.
• Pure DB read with no transport dependency (src/support/serenity/handlers/markets.js:157-209) - the handler takes only dataAccess and the slice coordinates. No upstream failure modes, no rate limiting concerns, near-zero operational cost.
• IDOR prevention via auth-derived brandId (src/controllers/serenity.js:339) - brandId comes from auth.brandUuid (derived from the validated IMS token), not from a user-controlled path parameter. Callers cannot enumerate another org's market slices.
• Strict input validation with safe regex (src/controllers/serenity.js:341) - /^\d+$/ for geoTargetId and the BCP-47 pattern for languageCode are both anchored, bounded, and free of catastrophic backtracking.
• Consistent controller shape - getMarket follows the exact same structure as the adjacent deleteMarket: IMS gate, authorize, strict-digit geo guard, delegate to handler, wrap in ok(), catch with mapError. A new team member scanning this file will immediately understand the pattern.
• Schema composition via allOf (docs/openapi/schemas.yaml) - SerenityMarketDetail extends SerenityMarket rather than duplicating fields. When SerenityMarket grows a field, the detail response picks it up automatically.
• Four-layer test coverage - handler unit (validation + happy path + 404), controller (forwarding + auth gate + error mapping), OpenAPI contract (schema conformance), and IT (auth-gate parity with DELETE). Each layer tests a distinct concern.
• Conventions fully followed - OpenAPI-first, schemas in schemas.yaml, route in src/routes/index.js, capability in required-capabilities.js, existing patterns reused. All documented CLAUDE.md rules for adding a new endpoint are satisfied.

Issues

Minor (Nice to Have)

createdAt/updatedAt in response - verify schema coverage - src/support/serenity/handlers/markets.js:196-197 returns createdAt and updatedAt, and the OpenAPI contract test validates the response against SerenityMarketDetail. These fields presumably come through the allOf reference to SerenityMarket. If SerenityMarket already declares them, no action needed. If it does not, the contract test might pass only because AJV allows additional properties by default, and the schema would not document what the endpoint actually returns.

Fix: Verify that SerenityMarket declares createdAt and updatedAt. If not, add them to the SerenityMarketDetail schema alongside semrushProjectId.

Assessment

Ready to merge? Yes
Reasoning: Well-structured, follows existing patterns closely, solid test coverage across four layers, security posture is clean, prior findings addressed, and all project conventions satisfied. The one Minor item is a schema-completeness check that is likely already satisfied by SerenityMarket's definition.

---

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

[rainer-friederich]

Thanks for the re-review and approval @MysticatBot-Dev.

On the Minor verify item (createdAt/updatedAt schema coverage): confirmed — SerenityMarket explicitly declares both as oneOf: [{ $ref: '#/DateTime' }, { type: 'null' }] (docs/openapi/schemas.yaml), and SerenityMarketDetail inherits them through the allOf reference. So the contract test passes because the fields are genuinely documented, not because of AJV's additional-properties leniency. No schema change needed.

Also addressed the codecov/patch gap in a follow-up commit: getMarket's pLang ? …toLowerCase() : null guard now has its : null side covered (empty-languageCode controller test), matching the deleteMarket coverage.