feat(serenity): audience-driven IMS promise pair for Semrush paths - #3033
feat(serenity): audience-driven IMS promise pair for Semrush paths#3033calvarezg wants to merge 5 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Let a request select the dedicated Semrush IMS promise-token pair via an optional x-promise-audience: semrush header, for both the synchronous serenity/elements/brands surfaces and the async Path-B classify-prompts job. Absent the header, everything uses the default pair (no behavior change); an unknown audience value returns 400 (fail closed). - utils.js: resolvePromisePair(context) maps the header to a pair selector or throws 400. getIMSPromiseToken/exchangePromiseToken take an optional pair, forwarded to ImsPromiseClient.createFrom. resolveSemrushImsToken derives the pair from the header, so all ~25 sync Semrush call sites are unchanged. - async-job-runner.js: persist the pair on job metadata at enqueue; exchange and invalidate read it back. Absent metadata -> default pair, so pre-change queued jobs keep working. Worker self-requeue forwards the job's pair. - Bump @adobe/spacecat-shared-ims-client 1.14.0 -> 1.16.0 (provides the pair selector); fix the stale 1.12.7 pin comment. The five stay-behind getIMSPromiseToken callers (edge-routing-auth, fixes, page-relationships, scrapeJob, suggestions) are untouched. Part of LLMO-6623. Inert until the UI sends the header; does not flip SEMRUSH_PROJECTS_BASE_URL. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @calvarezg,
Verdict: Approve - clean, well-scoped feature with correct token-flow threading and good test coverage.
Complexity: HIGH - medium diff; auth/IMS signal + dependency bump.
Changes: Adds audience-driven IMS promise-pair selection via an optional x-promise-audience header, threading the pair through sync and async (Path B) token flows (9 files).
Note: CI checks are currently pending - confirm they pass before merge.
Non-blocking (3): minor issues and suggestions
- suggestion: Consider truncating reflected input in the error message (
Unknown promise audience: ${audience}) to bound log/response size from long headers -src/support/utils.js:845 - nit: JSDoc on
getIMSPromiseTokencould clarify whatpairmeans for readers unfamiliar with IMS promise pairs (e.g. "determines which client-id/scope pair is used when minting") -src/support/utils.js:817 - nit:
{ pair: undefined }is passed tocreateFromwhen no audience is selected, which is semantically different from omittingoptsentirely - worth confirming the shared-ims-client 1.16.0 treats both identically -src/support/serenity/async-job-runner.js:167
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 8m 47s | Cost: $8.89 | Commit: 6b7eff7e0a8c743120a1e713fd61fdc918cbaaaa
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Address non-blocking MysticatBot review notes on #3033: truncate the reflected x-promise-audience value in the 'Unknown promise audience' 400 so a long or hostile header cannot bloat the response body or logs, and expand the getIMSPromiseToken pair JSDoc. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hey @calvarezg,
Verdict: Approve - clean, well-scoped feature with correct token-flow threading; new commit addresses prior suggestion.
Complexity: HIGH - medium diff; auth/IMS signal + dependency bump + API surface.
Changes: Adds audience-driven IMS promise-pair selection via an optional x-promise-audience header, threading the pair through sync and async (Path B) token flows (9 files).
Note: CI checks are currently pending - confirm they pass before merge.
Note: Recommend a human read before merge - this change introduces a new spec document (docs/specs/), codifying an architectural decision on IMS promise-pair routing. The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (2): minor issues and suggestions
- nit: No test exercises the 40-char truncation introduced in the latest commit. A string >40 chars would confirm the reflected value is bounded (e.g. pass an 80-char audience, assert error message does not contain the full string) -
test/support/utils.test.js - suggestion: The classify-prompts-job requeue test asserts
promiseTokenforwarding but notpromisePairforwarding. A one-lineexpect(enqueueArgs.promisePair).to.equal(expectedPair)closes the coverage gap at that integration seam -test/support/serenity/handlers/classify-prompts-job.test.js
Previously flagged, now resolved
- Truncate reflected input in error message (
src/support/utils.js) - addressed with.slice(0, 40)and explanatory comment.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 32s | Cost: $5.00 | Commit: 256e1b38a6b4d148a766e20dbaad55c282ee5c70
If this code review was useful, please react with 👍. Otherwise, react with 👎.
- CORS: add x-promise-audience to both allow-headers lists (index.js enableCors response + OPTIONS preflight). Without it a browser sending the header cross-origin fails preflight and the feature silently breaks from the UI. - utils: sanitize the reflected x-promise-audience value with cleanupHeaderValue (repo convention) and trim it before matching, so CR/LF can't inject into the 400 body/logs and surrounding whitespace doesn't fail closed. - utils: reorder so resolvePromisePair and getIMSPromiseToken each sit under their own JSDoc (the inserted helper had stranded getIMSPromiseToken's docs). - async-job-runner: when a pre-minted token is supplied, do not re-derive the pair from the request header — take the explicit promisePair only, so a supplied token and the persisted pair cannot diverge. Deferred (noted on LLMO-6930): a clearer error when the SEMRUSH env vars are unprovisioned — the clean fix would couple api-service to the Semrush env-var names, and the misconfigured window cannot occur under the rollout order (Vault before UI). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@igrubic thanks for the review. it came in over slack, so capturing it here on the PR for traceability. six findings, five addressed in c47a658, one deferred. rationale below, in your numbering. 1. CORS allowlist missing 2. stranded JSDoc (src/support/utils.js) — addressed. 3. pair re-derived from header even with a pre-minted token (async-job-runner.js) — addressed. 4. unsanitized header in the error message (src/support/utils.js) — addressed. 5. opaque 500 when the SEMRUSH env vars are unprovisioned (src/support/utils.js) — deferred. 6. exact-match without trim (src/support/utils.js) — addressed. tests, coverage, and type-check are green on the final commit. the feature stays dormant in prod until the UI sends the header, and this PR does not flip |
1. Abstract
Lets a request select the dedicated Semrush IMS promise-token pair via an optional
x-promise-audience: semrushheader, for both the synchronous serenity/elements/brands surfaces and the asynchronous Path-B classify-prompts job.2. Reasoning
LLMO-6623 gives the Semrush-facing paths a dedicated IMS promise pair (scoped for Semrush UDH) with limited blast radius, instead of the shared pair that serves every promise flow.
spacecat-shared-ims-client@1.16.0(LLMO-6928) added thecreateFrom(..., { pair })selector; this change is the api-service consumer of that selector. It is the exchange/mint side of the header the UI will send and auth-service will mint from.3. High-level overview of the changes
x-promise-audience. Absent → today's default pair (no behavior change).semrush→ the dedicated Semrush pair. Any other value → 400 (fail closed).resolvePromisePairreads the header;resolveSemrushImsTokenderives the pair and forwards it to the token exchange. All ~25 serenity/elements/brands call sites are unchanged — they inherit the behavior through that shared helper.@adobe/spacecat-shared-ims-client1.14.0 → 1.16.0.SEMRUSH_PROJECTS_BASE_URL— the proxy→native host flip is a separate Vault step after the end-to-end proofs.4. Required information
docs/specs/2026-08-12-llmo-6930-api-service-promise-audience.md(in this PR)5. Spec deviations
6. Affected / used mysticat-workspace projects
ImsPromiseClient.createFrom(..., { pair })andPROMISE_PAIRfrom@adobe/spacecat-shared-ims-client@1.16.0(LLMO-6928 / feat(ims-client): optional pair selector on ImsPromiseClient.createFrom spacecat-shared#1872, merged + published).x-promise-audienceto both. Mint and exchange must use the same pair.x-promise-audienceon the mint and on data calls.8. Test plan
resolvePromisePair(absent /semrush/ case-insensitive / unknown→400), pair pass-through onresolveSemrushImsToken, and Path-B persist/read-back (including the absent→default fallback). The Lambda bundle was built locally and the healthchecklambda()returns 200 (SITES-45260 gate), since this touches the bundle's dependency set.x-promise-audience. End-to-end proof of the Semrush pair (mint → exchange →semrushscope on the token) is owned by LLMO-6623's Path A/B runs, already validated at the IMS level for stage and prod. After Vault carries the Semrush pair, exercise one serenity route on dev with the header and confirm the exchange uses the Semrush consumer.9. Deployment & merge order
🤖 Generated with Claude Code