docs(adr): accept ADR-0001 — shared Semrush client ownership boundary - #1781
Conversation
Records the accepted boundary for LLMO-5975: shared client owns transport (auth, retry/backoff, timeouts, typed errors); consumers own error->HTTP translation, caching, redaction. Records LLMO-5980 (api-service migration) as deliberately deferred, not auto-funded by the confirmed transport-drift trigger -- the live resilience gap is closed directly in api-service instead (tracked separately).
Full revision, not a patch: adds Scope + Non-goals sections, rewrites Context to reflect that api-service already consumes the shared Project Engine/User Manager clients (retry was disabled via maxRetries:0, restored by #2766), and replaces the LLMO-5980 duplication framing with the confirmed four-path coverage map (Project Engine + User Manager migrated; Elements and AI Visibility are not). Boundary section unchanged.
|
This PR will trigger no release when merged. |
There was a problem hiding this comment.
Review(ADR-0001: shared Semrush client ownership)
Summary. Clear, well-scoped decision record: shared client owns transport (auth / retry / timeouts / typed errors); consumers own error→HTTP translation, caching, redaction — with an explicit boundary test and honest non-goals. Verified against code: api-service already consumes the shared clients; elements-transport.js is a hand-rolled POST on SEMRUSH_PROJECTS_BASE_URL with a 504 timeout and no retry (matches the Elements claim); LLMO-5976 shipped in #1777. One factual staleness to fix.
Must Fix
None.
Should Fix
- #2766 is still OPEN (verified today) — the ADR presents retry as already restored/closed. Lines 17 / 40 / 41 / 60 read as done. Until #2766 merges, the migrated surfaces still run one-shot in prod. Applying a minimal pending-merge correction — factual only; the boundary decision is unchanged.
Nits
- Follow-on drift: line 74 says LLMO-5978 + 5979 land "together as one hardening PR", but 5979 shipped standalone in #1819. Left as a comment for you to reframe — it's your plan narrative, not a fact I'll rewrite.
What's Good
- The "is it true for every consumer, or specific to one?" test is a crisp, reusable boundary heuristic.
- Redefines LLMO-5980 accurately to the two real remaining surfaces — matches the code (
elements-transport.js,grpc-transport.js). - Non-goals explicitly park AI Visibility (gRPC/OAuth2) rather than over-reaching.
|
|
||
| The Semrush Project Engine and User Manager clients exist as typed wrappers in `spacecat-shared` (`@adobe/spacecat-shared-project-engine-client`, `@adobe/spacecat-shared-user-manager-client`; PR #1661 lineage: IMS Bearer auth, retry/backoff). | ||
|
|
||
| `spacecat-api-service` **already consumes these shared clients** for its Project Engine and User Manager call paths (`controllers/serenity.js`, `controllers/brands.js`, `support/serenity/**`) — a migration that landed in prior shipped work. That migration pinned `maxRetries: 0` to preserve the previous one-shot behaviour, which silently disabled the shared clients' retry in production. PR #2766 removes that pin, restoring retry/backoff on those paths. |
There was a problem hiding this comment.
Verified today: PR #2766 is still OPEN (not merged). So "removes that pin, restoring retry/backoff" isn't true yet — the migrated surfaces still run one-shot in prod until it merges. Softening to a pending-merge framing (factual correction; the boundary decision is untouched).
|
|
||
| **LLMO-5980 is redefined as: close the two remaining surfaces, on their own merits.** | ||
|
|
||
| - **Elements** — same paradigm as Project Engine (REST, IMS Bearer, *same gateway* `SEMRUSH_PROJECTS_BASE_URL`), one endpoint (`fetchElement`, POST), hand-rolled, zero retry. This is the same resilience gap #2766 just closed, on the same gateway. It should be closed as a small, scoped follow-up — either by consuming the shared client if the endpoint fits it, or by mirroring the retry behaviour in place. Tracked separately; not bundled with the boundary decision. |
There was a problem hiding this comment.
Verified against spacecat-api-service/src/support/elements/elements-transport.js: POST, reads SEMRUSH_PROJECTS_BASE_URL, 504-on-timeout, no retry loop — so "hand-rolled, zero retry, same gateway" is accurate. (The path prefix is /enterprise/pages/api/v3, not projects/api, but the gateway/env is the same as stated.)
|
|
||
| - **AI Visibility transport (gRPC/HTTP2 + OAuth2).** Different protocol, different auth model; it does not drop onto the REST/IMS-Bearer shared client. Whether the shared-client boundary should ever stretch to cover gRPC/OAuth2 — or whether that surface gets its own client — is a separate decision, to be made when someone actually works that surface, not pre-emptively here. | ||
|
|
||
| ## Consequences |
There was a problem hiding this comment.
Factual: this says the gap "is closed (#2766)", but #2766 is still open. Changing to "will be closed once #2766 merges".
| - Consumers each implement error→HTTP translation. Mitigated by the typed error taxonomy being clear enough that translation is mechanical. | ||
|
|
||
| ## Follow-on work | ||
|
|
There was a problem hiding this comment.
Nit / plan drift: 5979 actually shipped on its own in #1819 (per your direction), not bundled with 5978. Flagging rather than rewriting — your call how to reframe the follow-on plan.
|
|
||
| - **LLMO-5977** — client facade in `spacecat-shared` (makes the boundary real in code). Gated on this ADR. | ||
| - **LLMO-5978 + LLMO-5979** — typed errors + request timeouts, landing together as one hardening PR. | ||
| - **LLMO-5980** — close Elements (small, same-shape); AI Visibility deferred as a non-goal. |
There was a problem hiding this comment.
Verified: LLMO-5976 spec-verify gate is Closed and shipped in PR #1777. Accurate.
ADR-0001 stated the api-service retry gap was already restored/closed by #2766, but #2766 is still open. Correct to pending-merge framing across the Context, the consumption table, and the Consequences. Factual only — the ownership boundary decision is unchanged. LLMO-5975 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - clean boundary ADR; minor factual drift in the follow-on section, non-blocking.
Complexity: LOW - small docs-only change, single file.
Changes: Establishes ADR-0001 defining the shared Semrush client ownership boundary - transport in shared client, context in consumers (1 file).
Note: Recommend a human read before merge - this change creates an architectural decision record (docs/adr/). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (3): minor issues and suggestions
- nit: Follow-on line says LLMO-5978 + 5979 land "together as one hardening PR" but they are tracked separately (5979 is PR spacecat-shared#1819, currently open) -
docs/adr/0001-shared-semrush-client-ownership.md:74 - suggestion: PR 2766 is in spacecat-api-service, not this repo; disambiguate with a repo qualifier (e.g.
spacecat-api-service#2766) or full URL to avoid reader confusion -docs/adr/0001-shared-semrush-client-ownership.md:7 - suggestion: Add a one-line rationale for why caching stays consumer-side (e.g. "staleness tolerance varies per consumer") to make the boundary more defensible against future re-litigation -
docs/adr/0001-shared-semrush-client-ownership.md:34
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 7m 44s | Cost: $2.50 | Commit: 5c2e242ad94ab295e5427f72e80ed9d857a266ee
If this code review was useful, please react with 👍. Otherwise, react with 👎.
MysticatBot
added
ai-reviewed
…tionale, follow-on drift - Qualify cross-repo references to spacecat-api-service#2766 (it's not in this repo). - Add a one-line rationale for why caching stays consumer-side (scope/keying/ staleness vary per consumer) so the boundary is defensible against re-litigation. - Correct the follow-on plan: 5979 shipped separately in #1819, not bundled with 5978 as "one hardening PR". LLMO-5975 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Addressed MysticatBot review (commit 54da758)
- ✅ suggestion — qualify cross-repo
#2766. Nowspacecat-api-service#2766everywhere it appears (Relates-to, Context, table, Consequences) — it's not in this repo. - ✅ suggestion — caching rationale. Added why caching stays consumer-side (scope / keying / staleness tolerance vary per consumer, e.g. api-service's workspace-layer cache) so the boundary resists re-litigation.
- ✅ nit — follow-on drift. Corrected: 5979 shipped on its own in #1819, not bundled with 5978 as "one hardening PR". (This matches the same item in my own review.)
Also noting MysticatBot's meta-point: this is an ADR, so a human read before merge is warranted — the bot + this automated review complement, not replace, that.
| ## Follow-on work | ||
|
|
||
| - **LLMO-5977** — client facade in `spacecat-shared` (makes the boundary real in code). Gated on this ADR. | ||
| - **LLMO-5979** — request timeouts. Shipped on its own in #1819 (open), not bundled with 5978. |
There was a problem hiding this comment.
Applied — the follow-on now reflects reality: 5979 shipped separately in #1819; 5978 is the remaining hardening item, gated on the facade (5977).
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - all prior nits addressed; one minor factual inconsistency remains, non-blocking.
Complexity: LOW - small docs-only change, single file.
Changes: Adds ADR-0001 defining the shared Semrush client ownership boundary (transport in shared client, context in consumers) with addressed follow-on nits (1 file).
Note: Recommend a human read before merge - this change creates an architectural decision record (docs/adr/). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (1): minor issues and suggestions
Previously flagged, now resolved
- Cross-repo PR ref now qualified as
spacecat-api-service#2766 - Caching rationale added (scope/keying/staleness vary per consumer)
- Follow-on items (LLMO-5978, 5979) now described with correct independent tracking
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 9m 10s | Cost: $2.24 | Commit: 54da758bc959a72252da167c5823681ac3e20d73
If this code review was useful, please react with 👍. Otherwise, react with 👎.
MysticatBot nit: an open PR hasn't shipped. Reword to "In progress in #1819 (open)". LLMO-5975 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Addressed MysticatBot re-review (commit incoming)
- ✅ nit — "Shipped … (open)" contradiction. Reworded to "In progress in #1819 (open), on its own PR rather than bundled with 5978." An open PR hasn't shipped — good catch.
All prior items resolved; no other findings.
| ## Follow-on work | ||
|
|
||
| - **LLMO-5977** — client facade in `spacecat-shared` (makes the boundary real in code). Gated on this ADR. | ||
| - **LLMO-5979** — request timeouts. Shipped on its own in #1819 (open), not bundled with 5978. |
There was a problem hiding this comment.
Fixed the contradiction — "In progress in #1819 (open)" instead of "Shipped … (open)".
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - all prior nits addressed; no new issues.
Complexity: LOW - small docs-only change, single file.
Changes: Establishes ADR-0001 defining the shared Semrush client ownership boundary - transport in shared client, context in consumers (1 file).
Note: Recommend a human read before merge - this change creates an architectural decision record (docs/adr/). The bot review is a complement to, not a replacement for, a human read here.
Previously flagged, now resolved
- "Shipped on its own in #1819 (open)" contradiction corrected to "In progress in #1819 (open)" - consistent with the PR's open status
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 7m 50s | Cost: $2.27 | Commit: 34a6ed979ee5d63d6e17e93f5c170678328c08e0
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…-client-ownership
Two follow-on lines drifted since the ADR's 2026-07-06 date: - LLMO-5976 (spec-verify gate) shipped via #1811, not #1777 (#1777 is still open). - LLMO-5979 (request timeouts) has since shipped in #1819 (was "in progress / open"). Docs-only; no decision change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - all prior nits addressed; latest commit corrects stale follow-on PR refs, no new issues.
Complexity: LOW - small docs-only change, single file.
Changes: Corrects stale follow-on PR references in ADR-0001 to reflect current tracking state (1 file).
Note: Recommend a human read before merge - this change creates an architectural decision record (docs/adr/). The bot review is a complement to, not a replacement for, a human read here.
Previously flagged, now resolved
- Follow-on PR references now accurately reflect current tracking (LLMO-5979 in progress in its own PR, LLMO-5976 shipped in PR 1811)
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 2s | Cost: $2.46 | Commit: 597a47c0b48b164992749f0cabd588b2e39ec51a
If this code review was useful, please react with 👍. Otherwise, react with 👎.
2 participants
Spec
LLMO-5975: agree the ownership boundary for the shared Semrush client before the facade
build-out (LLMO-5977) lands, so subsequent PRs don't re-litigate it.
Boundary decided:
typed error taxonomy.
LLMO-5980 disposition: investigation confirmed transport behavior has already
diverged between spacecat-shared's client and spacecat-api-service's hand-built
Serenity transport (shared has retry/backoff, api-service has none). That divergence
is closed directly via a targeted retry-parity fix in api-service (tracked separately,
not gated on this ADR). Migrating api-service onto the shared client itself is recorded
as "revisit deliberately" — not funded now, not closed as a non-goal. It's a deliberate
future call, sequenced after the facade exists, not something the resilience gap should
stampede.
Implementation plan
docs/adr/0001-shared-semrush-client-ownership.md— status Accepted, starts adocs/adr/practice in this repo (previously no ADR file/directory existed anywherein spacecat-shared; tickets were tracked purely via inline external Jira links).
(typed errors + timeouts, one combined hardening PR), LLMO-5980 (deferred), LLMO-5976
(spec-verify CI gate — independent, already shipped in test(project-engine-client,user-manager-client): spec-verify checksum gate (LLMO-5976) #1777).