feat(serenity): re-enable changing a Semrush-attached site's URL - #3098
feat(serenity): re-enable changing a Semrush-attached site's URL#3098byteclimber wants to merge 3 commits into
Conversation
Replaces the unconditional 403 on PATCH /sites/:siteId baseURL changes with an accept-and-propagate flow, per serenity-docs#349. A brand's own primary site can now have its URL changed to another URL on the same registrable domain (subdomain/subpath) -- the change is propagated to Semrush (project primary_url, own-brand benchmark domain, republish) before the SpaceCat-side URL is persisted, so a Semrush failure never leaves the two out of sync. Market-mirror sites stay 403'd and a registrable-domain change is refused (422) -- both are explicitly out of scope for this pass (serenity-docs#349 workstreams 3-4). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
|
Mysticat review failed: Claude CLI crashed (exit 1): stderr= stdout={"type":"result","subtype":"error_max_budget_usd","duration_ms":3,"duration_api_ms":1503306,"is_error":true,"num_turns":1,"stop_reason":null,"session_id":"2f148ebc-363d-46ca-8a13-a759d817e4be","total_cost_usd":10.07505215,"usage":{"input_tokens":0,"cache_creation_input_tokens":0,"cache_read_input_to |
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Request changes - three material issues around edge-case URL handling and partial-failure observability.
Complexity: HIGH - large diff; API surface change.
Changes: Replaces the unconditional 403 on site URL edits for Semrush-managed brands with a validate-propagate-persist flow scoped to same-registrable-domain changes (9 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 currently pending - verify before merge.
Must fix before merge
- [Important] getDomain returns null for IP/localhost/private-TLD URLs, causing null===null to pass the domain gate -
src/controllers/sites.js:~1258(details inline) - [Important] Missing URL normalization before collision check, inconsistent with createSite which uses composeBaseURL -
src/controllers/sites.js:~1214(details inline) - [Important] Partial propagation loop lacks observability of which projects were already updated before failure -
src/support/serenity/site-url-propagation.js:~80(details inline)
Non-blocking (5): minor issues and suggestions
- nit:
new URL(site.getBaseURL())can throw on malformed stored URLs (data corruption), producing an opaque 500 -src/controllers/sites.js:~1258 - suggestion: Add 409 (siteUrlTaken) and 422 (crossDomainNotSupported) response definitions to the PATCH /sites/{siteId} endpoint in the OpenAPI spec -
docs/openapi/schemas.yaml - nit: The non-transport error catch path passes
err.messageto the client for non-500 statuses - currently safe (only quotaExceeded hits it) but fragile if new throw sites are added -src/controllers/sites.js:~1275 - nit:
if (benchmarkId)is falsy for a "0" string ID; preferbenchmarkId != null-src/support/serenity/site-url-propagation.js:93 - suggestion: Add a controller-level test for the SerenityTransportError -> 502 path to verify the upstream message scrubbing -
test/controllers/sites.test.js
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 4m 54s | Cost: $9.86 | Commit: 9ae77794067dd8a3a12b7b3d125a82948f72184c
If this code review was useful, please react with 👍. Otherwise, react with 👎.
The IT test 'returns 403 when changing the baseURL of a site attached to a Semrush-managed brand' asserted the OLD unconditional-403 behavior replaced by the previous commit. Its target URL (site1.example.com -> semrush-managed-rename.example.com) shares a registrable domain, so under the new code this is a same-domain edit that now succeeds and propagates instead of 403ing. Splits it into three IT tests matching the new contract: same-domain change succeeds (200, persisted -- BRAND_1 has no brand_to_semrush_projects mapping row in this seed, so propagation resolves to zero projects and no-ops, no live Semrush mock call needed), a cross-registrable-domain change is refused (422, crossDomainNotSupported), and a market-mirror site (MARKET_SITE_1, linked via brand_sites) stays 403'd. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Live-verified against adobe-hackathon.semrush.com (2026-08-18, LLMO-Dev-2 child workspace, deleted after): a project's PATCH accepts and persists a changed `domain` (not just `primary_url`), and a subsequent publish settles cleanly with no project recreation. The "would this lose history?" concern that motivated refusing cross-domain edits (422 crossDomainNotSupported) does not hold, so this removes that refusal -- propagateSiteUrlToSemrush now also PATCHes the project's own `domain` alongside `primary_url`, keeping Semrush's registrable-domain grouping key in step with the new URL. Market-mirror sites remain out of scope (serenity-docs#349 workstream 4, still an open decision). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Mysticat review failed: Claude CLI crashed (exit 1): stderr= stdout={"type":"result","subtype":"error_max_budget_usd","duration_ms":3,"duration_api_ms":1823377,"is_error":true,"num_turns":1,"stop_reason":null,"session_id":"5a22e3f7-2283-4f5c-be1c-071bb45c23d8","total_cost_usd":11.46132545,"usage":{"input_tokens":0,"cache_creation_input_tokens":0,"cache_read_input_to |
Summary
Closes the backend half of adobe/serenity-docs#349 (issue #348 having already landed the prerequisite:
primary_urlwritten at Semrush provisioning time, PR #3073).PATCH /sites/:siteIdbaseURLchanges for a Semrush-managed brand's own primary site with an accept-and-propagate flow.adobe-hackathon.semrush.com(2026-08-18, throwaway workspace, deleted after) that a project'sdomainPATCH is accepted, persists, and a subsequent publish settles cleanly with no project recreation -- so the refusal was removed andpropagateSiteUrlToSemrushnow PATCHes the project'sdomainalongsideprimary_url.domain+primary_url, own-brand benchmarkdomainvia a full-body PUT, republish) runs before the SpaceCat-side URL is persisted, so a Semrush failure leaves the site untouched rather than desyncing it from what Semrush actually tracks.409 siteUrlTaken(collision with another site),409 quotaExceeded(existing convention, now reachable from this path too).Changes
docs/openapi/schemas.yaml--baseURLadded toSiteUpdate.src/controllers/sites.js-- guard replaced with validation + collision check (409) + Semrush propagation (no domain-change restriction).src/support/serenity/site-url-propagation.js(new) -- re-points every live Semrush project mapped to the edited site (primary_url+domain).src/support/serenity/mapping-rows.js-- newprojectsForSite()helper (a brand's markets can share or differ in Site, so this scopes propagation correctly).test/it/shared/tests/sites.js-- IT coverage for same-domain, cross-domain, and market-mirror cases.Test plan
npm test-- 528+ relevant tests passing, no failuresnpm run type-check-- cleannpm run lint(scoped to changed files) -- cleannpm run docs:lint/npm run docs:build-- valid, docs rebuiltadobe-hackathon.semrush.comsandbox workspace (create -> PATCH domain -> publish -> delete, throwaway workspace cleaned up)test/it/postgres/sites.test.jscould not be run in this environment (Docker/ECR access unavailable) -- please run locally before merge:npx mocha --require test/it/postgres/harness.js --timeout 60000 test/it/postgres/sites.test.js🤖 Generated with Claude Code