fix(project-engine-client): enforce https:// on the brand_urls mock#1778
Conversation
) The vendored Project Engine mock accepted any brand-URL value on POST .../brand_urls, but the live Semrush gateway REQUIRES a literal https:// (write-probed 2026-07-06): a scheme-less value 400s on the go-validator `url` tag, a valid non-https URL on the `startswith=https://` tag. That fidelity gap let the url/resolve mis-design (serenity-docs#25) ship — an IT went green while the same write 400s in prod. Add a pure, unit-tested `brandUrlHttpsTag` helper (mock/brand-url-validation.js, exposed via $.context like resolveUrl/tagId) and enforce it in the POST route, returning the live 400 BasicResponse. An e2e case pins the 400 over the real route for both the scheme-less and http:// forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This PR will trigger a patch release when merged. |
There was a problem hiding this comment.
Hey @rainer-friederich,
Verdict: Approve - clean, well-tested patch that closes a real mock fidelity gap.
Complexity: LOW - small single-service change, test-only mock code.
Changes: Adds https:// URL validation to the brand_urls mock POST handler to match the live Semrush Project Engine API's go-validator behavior (5 files).
Non-blocking (2): minor issues and suggestions
- nit: Unit test covers
undefinedbut notnullas input tobrandUrlHttpsTag- addingexpect(brandUrlHttpsTag(null)).to.equal('url')documents the coercion contract for readers -packages/spacecat-shared-project-engine-client/test/mock/brand-url-validation.test.js - suggestion: Confirm existing e2e brand_urls creation tests (above line 1834) send
https://URLs, which would implicitly validate the happy path through the new validation gate -packages/spacecat-shared-project-engine-client/test/e2e/project-engine-mock.e2e.js
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 15s | Cost: $3.41 | Commit: b8bd6e7be585233e2a36231002d77399399c2864
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…actly Re-probed the live `create_brand_urls` contract (2026-07-13, prod adobe-hackathon.semrush.com, throwaway benchmarks in the LLMO-Dev-2 dev sub-workspace, all cleaned up) across ~40 edge cases. The probe overturned three assumptions the first cut had made from JS `URL` semantics: - An upper/mixed-case scheme (`HTTPS://X`) was ACCEPTED but live 400s it on `startswith` — go's HasPrefix compares the raw bytes, case-sensitively. - Opaque, hostless values (`mailto:`, `tel:`, `https:x.com`) were reported on the `url` tag; live clears `url` on them and fails `startswith`. - A trailing-space value (`https://x.com `) was ACCEPTED because JS `URL` trims; live rejects it on `url`. Leading space and inner whitespace likewise. Also captured, and now modelled: an empty/null/missing url reports `required` (not `url`); a non-string url 400s as an unmarshal error with no tag; every `file:` form clears `url` and dies on `startswith` (the deployed validator exempts the scheme, which go-validator's current source would not); a non-empty fragment substitutes for a missing host (`https://#frag` is accepted, `https://#` is not); and the batch is atomic — one bad entry creates nothing. The validator is rewritten to reproduce go's parse rather than JS's, and the unit test now records the probed matrix case by case. The route drops its unreachable branches: the spec marks `url` required + typed, so request validation answers a missing/non-string url before the handler. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Re-probed the live MysticatBot (Approve, 2 non-blocking):
Write-probe (2026-07-13, prod
Also newly captured and now modelled:
The validator now reproduces go's parse rather than JS's, and the unit test records the probed matrix case by case. The route drops two branches that turned out to be unreachable: the spec marks Verification: lint, |
There was a problem hiding this comment.
Hey @rainer-friederich,
Verdict: Approve - clean, focused mock fidelity fix with thorough test coverage; no blocking issues found on re-review.
Complexity: HIGH - medium diff; API surface risk flag (mock route handler).
Changes: Adds https:// URL validation to the brand_urls mock POST handler, reproducing the live Semrush go-validator behavior so integration tests cannot pass over writes the live gateway would reject (5 files).
Note: CI "Test" check is still pending - confirm it passes before merge.
Non-blocking (2): minor issues and suggestions
- nit: No unit test exercises a non-whitespace control character (e.g., U+0001 or U+007F) against
isGoParseableUrl- the regex handles the U+0000-U+001F and U+007F ranges explicitly, but coverage is only indirect via whitespace overlap. Adding a test case likeexpect(brandUrlHttpsTag('https://x' + String.fromCharCode(1) + '.com')).to.equal('url')would pin the control-byte path directly. -packages/spacecat-shared-project-engine-client/test/mock/brand-url-validation.test.js - suggestion: The batch-atomicity e2e test asserts
response.status === 400and verifies the good entry was not created, but does not assert the error message content. Aexpect(error.message).to.match(/failed on the 'url' tag/)would confirm the rejection came from the validation gate rather than from some other 400 source. -packages/spacecat-shared-project-engine-client/test/e2e/project-engine-mock.e2e.js:257
Previously flagged, now resolved
- Unit test for
nullinput tobrandUrlHttpsTagwas already present at line 377 (prior nit was based on an incorrect reading of the test file).
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 15s | Cost: $4.02 | Commit: 673ae62f5f2b433f93a79d253d2a020683838fdb
If this code review was useful, please react with 👍. Otherwise, react with 👎.
… assertions - Unit: assert NUL, U+0001 and U+007F reject on the `url` tag, wherever they sit in the value — probed live rather than inferred, so the control-byte path in the parser is covered directly instead of only via its whitespace overlap. - E2E: assert the atomic batch rejection carries the `url` tag message, proving the 400 comes from the validation gate rather than any other source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed both non-blocking items from the re-review in
The Verification on this head: lint, |
…primary domain (#2799) <!-- mysticat-pr-skill --> ## 1. Abstract Brand URLs are written to Semrush verbatim with their `https://` scheme, and Project Engine's `url/resolve` is dropped from the write path entirely. The brand's own primary domain is skipped when building the brand-URL set, so it is not written as a redundant `website` entry alongside the benchmark that already carries it. ## 2. Reasoning Semrush's brand-URLs API requires a literal `https://` scheme and stores the submitted value verbatim. It rejects a scheme-less value with `400 … Field validation for 'URL' failed on the 'url' tag`, and an `http://` value with `400 … failed on the 'startswith' tag`. The write path on `main` resolves website brand URLs through `GET /v1/url/resolve` to Project Engine's canonical form — which is **scheme-less** — and writes that. Semrush therefore rejects every brand-URL write we make. In production, for serenity-active orgs: - **Brand edit** (`PATCH /brands` → `syncBrandUrlsAcrossMarkets`) hard-fails *after* the brand row has already been committed: the caller gets a 500 and the database drifts out of sync with Semrush. - **Market create** (`handleCreateMarketSubworkspace`) swallows the failure by design and the market goes live with **no brand URLs propagated at all**, emitting `SERENITY_MARKET_URL_ATTACH_DIVERGENCE`. This reached production in v1.624.0 (2026-07-06) and is still live. The cause is a merge accident rather than a design decision: the pull request that introduced the `url/resolve` approach was squash-merged while only its first three commits existed. The two commits that reverted the approach — after it was discovered that a scheme-less value cannot be a `brand_urls` value at all — were authored after that merge landed and remained on the feature branch, so they never reached `main`. This change re-applies that correction to current `main`. The duplicate the resolve was originally meant to prevent is real, but `url/resolve` was the wrong instrument for it: the brand's primary domain appears both as the benchmark's own scheme-less `primary_url` and as an explicit `website` brand-URL row. Since a `brand_urls` value must carry `https://` and a benchmark's domain never does, the two can never be made equal by normalization — the duplicate has to be avoided by not writing the row in the first place. ## 3. High-level overview of the changes **Before:** every `website`-type brand URL was sent through `transport.resolveUrl` and the returned scheme-less canonical value was written to Semrush, which rejected it. The brand's primary domain was written as a `website` row, duplicating the benchmark's own `primary_url`. **After:** - Brand URLs are written **verbatim**, keeping the `https://` scheme the API demands. `social` and `earned` entries — which are identity handles such as `https://instagram.com/brand`, not brand domains — were never a sensible target for apex-normalization and are likewise written unchanged. - `resolveWebsiteEntries` and the `resolveUrl` transport method are removed. `url/resolve` is no longer called from any write path; it is not a valid sink for `brand_urls`. - `collectBrandUrlEntries` takes the project's own primary domain and skips a `website` entry whose host matches it, compared host-wise so that both the apex and the `www.` form are recognised. Market create passes the domain from the create payload; the edit re-sync passes the project's own domain. The benchmark continues to carry the primary domain; socials and secondary sites are unaffected. Operationally: brand edits that touch URLs stop returning 500 and stop leaving the database diverged from Semrush, and newly created markets get their brand URLs attached instead of silently going live without them. ## 4. Required information - Other: adobe/serenity-docs issue 25 — Brand URLs: skip the brand's own primary domain (drop url/resolve — invalid sink) adobe/serenity-docs#25 ## 5. Affected / used mysticat-workspace projects - **spacecat-shared** (contract) — the vendored `spacecat-shared-project-engine-client` supplies the typed Project Engine client and its mock. Its `brand_urls` mock currently accepts any URL, which is why the scheme-less write path passed CI while failing against the live API. adobe/spacecat-shared#1778 makes the mock enforce the live `https://` 400 and closes that fidelity gap. - **mysticat-data-service** (contract, no change needed) — the Serenity migration CLI writes brand URLs to the same Semrush endpoint and already coerces to `https://` and skips the primary domain, so it stays consistent with this change. - **project-elmo-ui** (consumed, no change needed) — already normalizes user-entered brand URLs to `https://` before calling this service, so the values arriving here carry a scheme. ## 6. Additional information outside the code The live contract was re-probed against the production Semrush tenant on 2026-07-13, using a throwaway benchmark in an internal test sub-workspace. The benchmark and every row written during the probe were deleted afterwards; the workspace was left as found. Against `POST /v2/workspaces/{ws}/projects/{pid}/aio/benchmarks/{bid}/brand_urls`: | submitted | result | |---|---| | `example.net`, `www.example.org`, `example.net/products` | 400 — `Field validation for 'URL' failed on the 'url' tag` | | `http://example.com` | 400 — `… failed on the 'startswith' tag` | | `https://example.org` | 200 | | `https://www.example.net` and `https://example.net` | 200 — persisted as two distinct rows, verbatim, with no `www`/apex de-duplication | Two further observations that shaped this change: - Semrush announced a deploy "unifying the format for brand URLs" on 2026-07-13. That unification is **display-only**: live rows are still *stored* verbatim with their scheme, while the Semrush UI renders them scheme-less. The write contract is unchanged, so writing scheme-less values remains incorrect. - A `website` row whose host equals the benchmark's own primary domain is still accepted and still persists through a publish. The duplicate therefore does not resolve itself upstream, and the skip is genuinely required rather than merely defensive. ## 7. Test plan Verified against the live production Semrush tenant as described in section 6: the scheme-less values this service currently writes are rejected, and the `https://` values it writes after this change are accepted and stored as submitted. **dev:** with a serenity-active org, `PATCH /brands` on a brand whose `urls` are touched should return 2xx rather than 500, and the brand's `website`/`social`/`earned` URLs should appear on the market's own-brand benchmark with their `https://` scheme intact. The brand's own primary domain should be absent from the brand-URL list — it is carried by the benchmark's `primary_url` instead. **prod:** after release, confirm that `SERENITY_MARKET_URL_ATTACH_DIVERGENCE` stops appearing in the api-service logs on Serenity market creates, and that a brand edit touching URLs no longer produces the post-commit re-sync failure. ## 8. Deployment & merge order - adobe/spacecat-shared#1778 — related. Makes the vendored project-engine-client mock enforce the live `https://` 400, so a regression of this class fails CI instead of shipping. Independent of this fix at runtime: it changes only the mock, so it neither blocks nor is blocked by this PR. This PR is safe to merge and deploy on its own, and should not wait on the shared PR. Merging the shared PR first (or after) has no effect on the correctness of this change; landing it at some point is what prevents the regression from recurring. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @rainer-friederich,
⚠ 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 - both prior nits addressed; no new issues on the incremental delta.
Complexity: HIGH - medium diff; API surface risk flag (mock route handler).
Changes: Adds https:// URL validation to the brand_urls mock POST handler to match the live Semrush go-validator behavior (5 files).
Note: CI "Test" and "E2E" checks are still pending - confirm they pass before merge.
Previously flagged, now resolved
- Control-byte unit test now pins U+0001, U+007F, and U+0000 directly against
brandUrlHttpsTag. - Batch-atomicity e2e test now asserts the error message content (
failed on the 'url' tag), confirming the 400 came from the validation gate.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 8m 20s | Cost: $1.75 | Commit: 1bf8bc9e59158a2b2c5b711b914d25449a83eb87
If this code review was useful, please react with 👍. Otherwise, react with 👎.
## [@adobe/spacecat-shared-project-engine-client-v1.10.1](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-project-engine-client-v1.10.0...@adobe/spacecat-shared-project-engine-client-v1.10.1) (2026-07-13) ### Bug Fixes * **project-engine-client:** enforce https:// on the brand_urls mock ([#1778](#1778)) ([46c4860](46c4860)), closes [serenity-docs#25](https://github.com/adobe/serenity-docs/issues/25) [serenity-docs#25](https://github.com/adobe/serenity-docs/issues/25) [#2748](https://github.com/adobe/spacecat-shared/issues/2748) [#737](#737) [#2748](https://github.com/adobe/spacecat-shared/issues/2748) [#737](#737) [#2748](https://github.com/adobe/spacecat-shared/issues/2748)
|
🎉 This PR is included in version @adobe/spacecat-shared-project-engine-client-v1.10.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1. Abstract
The vendored Project Engine mock now enforces the live Semrush validation on
POST .../brand_urls: a brand URL must be a literal, lower-casehttps://URL, and the mock answers a non-conforming entry with the same 400 (and the same go-validator tag) the live gateway returns, instead of silently accepting it.2. Reasoning
While reworking the serenity-docs#25 brand-URL feature, a prod write-probe showed
create_brand_urlsREQUIRES a literalhttps://: a scheme-less value 400s on the go-validatorurltag, and a valid non-https URL (http://,ftp://) 400s on thestartswith=https://tag. The mock accepted all of these, so an integration test could go green over a write the live gateway rejects — exactly the fidelity gap that let the earlier "resolve brand URLs to their scheme-less form before writing" mis-design ship (the scheme-less resolve value cannot be written as a brand URL at all).Because the point of this gate is fidelity, the contract was probed rather than inferred: ~40 edge cases were POSTed to the live API and the mock reproduces the observed matrix, including the places where go's semantics differ from JavaScript's and a
URL-based check silently gets the wrong answer.3. High-level overview of the changes
Before: the mock's
POST .../brand_urlshandler created a row for any{ url, type }entry, regardless of scheme, and always returned 200.After: each entry's
urlis validated against the live contract before any row is created. A non-conforming entry short-circuits the whole batch with the gateway's 400BasicResponse, naming the tag live names.BrandURLRequest.URLcarriesrequired,url,startswith=https://, evaluated in that order:required— an empty (or null/missing) value; reported beforeurlis ever evaluated.url— the value must parse, under go's rules: a non-empty scheme plus a host, a fragment, or opaque content. A scheme-less value (lovesac.com,www.x.com,//x.com), a scheme with only a rooted path (https:/x.com), a barehttps://, and any value containing whitespace fail here.startswith=https://— a literal, case-SENSITIVE prefix check against the raw value, reached by anything that parsed.http://,ftp://, an upper- or mixed-case scheme (HTTPS://X,Https://x), everyfile:form, and the hostless-but-opaque forms (mailto:,tel:,https:x.com) fail here — not onurl.A conforming value is accepted and stored verbatim (no scheme or
www.normalization), unchanged from today; an upper-case host, an IDN host, an IP, and a port/path/query are all accepted, as ishttps://#frag(a non-empty fragment satisfiesurleven with no host). The batch is atomic: one bad entry creates nothing.The check lives in a pure
brandUrlHttpsTaghelper exposed on the request context (same convention as the existingresolveUrl/tagIdlib helpers), so it is unit-tested independently of the coverage-excluded route handler. It reproduces go's parse by hand rather than delegating to JavaScript'sURL, whose WHATWG parsing disagrees with go exactly where it matters: it trims surrounding whitespace, and it reports no host for the opaque forms go accepts.4. Required information
create_brand_urlscontract (2026-07-13, throwaway benchmarks in a dev workspace, cleaned up in the same run)5. Affected / used mysticat-workspace projects
https://brand URLs after the skip-primary-domain redesign; this mock keeps its integration tests honest to the live 400.https://brand URLs; same fidelity guarantee.6. Additional information outside the code
The contract was captured by a live write-probe against prod
adobe-hackathon.semrush.comon 2026-07-13: throwaway competitor benchmarks were created in the LLMO-Dev-2 dev sub-workspace, ~40 candidate values were POSTed one per request, the accept/reject verdict and go-validator tag were recorded for each, and every benchmark and row created was deleted in the same run. No customer data was touched.Two results are worth calling out because they contradict what the code would otherwise assume:
startswithisstrings.HasPrefixagainst the raw value, so an upper-case scheme (HTTPS://…) is rejected, while an upper-case host (https://EXAMPLE.NET) is fine. Only the scheme must be literal.file:form clears theurltag and fails onstartswith— includingfile://andfile:///, which go-validator's current source would reject onurl. The deployed validator predates that path check, so the probed behaviour is what the mock models; the code says so at the point it matters.7. Test plan
file:, degenerate and whitespace forms,required, and the non-string unmarshal failure. The package's 100% branch-coverage gate is met.test:e2e, which boots the real mock server): the route answers the live 400 and tag over real HTTP for the scheme-less,http://, upper-case-scheme,mailto:and empty-url forms, a missing url is rejected by request validation, the batch is proven atomic (a good entry alongside a bad one is not created), and the existing create/list/delete e2e (which uses anhttps://URL) still passes through the new gate.spacecat-shared-project-engine-clientrelease and is picked up by consumers when they bump.8. Deployment & merge order
🤖 Generated with Claude Code