fix(cloud-manager-client): build BYOG PR URLs (Azure/Bitbucket) + hostname detection + retry-once#1793
Conversation
`createPullRequest` constructed a pullRequestUrl only for GitHub and
GitLab; for Azure DevOps and Bitbucket BYOG repos the provider detection
fell through to null, so `pullRequestUrl` was never set even though the CM
Repo API had actually created the PR (and returned its externalNumber).
Downstream (spacecat-autofix-worker CodeRepoManager) then stored an empty
`changeDetails.pullRequestUrl`, so the CWV "Deployed" tab showed no
"View PR" link for those repos.
Add path templates + host detection for:
- Azure DevOps (dev.azure.com / *.visualstudio.com) -> /pullrequest/{n}
- Bitbucket Cloud (bitbucket.org) -> /pull-requests/{n}
Unknown hosts still return null (unchanged). Adds unit tests for both new
providers and repoints the "unsupported provider" test at a generic host;
package stays at 100% coverage.
Co-Authored-By: Claude <noreply@anthropic.com>
|
This PR will trigger a patch release when merged. |
createPullRequest issued a single POST and threw on any non-2xx, with no retry — a brief 5xx/429 or network blip from the CM Repo API would fail the whole autofix PR. Add one retry after a short wait (1.5s) for transient failures only; permanent failures (4xx other than 429) still throw immediately. The transient failure + retry is logged (log.warn). Adds tests for transient-5xx-then-success, network-error-then-success, and two-consecutive-transient-failures (throws after 2 attempts). Package stays at 100% coverage. Co-Authored-By: Claude <noreply@anthropic.com>
ramboz
left a comment
ramboz
left a comment
There was a problem hiding this comment.
PR Review: fix(cloud-manager-client): build PR URL for Azure DevOps and Bitbucket
Summary
Adds Azure DevOps + Bitbucket Cloud provider detection and PR/MR path templates to #buildPullRequestUrl, and — not mentioned in the description — wraps the CM PR-creation fetch in a single-retry loop for transient (5xx / 429 / network) failures. The logic is correct, backward-compatible, and well-tested. Ready to merge; the one Should-Fix is doc-only.
This is one half of a cross-repo effort with adobe/spacecat-autofix-worker#666: this client must be released to npm first, then #666 bumps the dep. #666 relies on the new Azure/Bitbucket URL building here so the CWV "View PR" link renders for those BYOG repos, and the retry added here reduces how often #666's "branch pushed, PR failed" path is hit.
Strengths
- The retry loop is carefully written. Permanent 4xx (except 429) throw immediately; transient 5xx/429 and thrown fetches (network/timeout) retry once; exhaustion throws a distinct
...failed after N attemptsmessage. Theif (response && !transient)guard correctly distinguishes a re-thrown permanent HTTP error from a network throw, and every attempt-2 path eitherbreaks or throws — so the loop can never fall through toresponse.json()with an undefined/non-ok response. #PR_PATH_BY_PROVIDERis a clean frozen lookup table; unknown hosts still returnnull, so there is no regression for providers CM doesn't map.- Good, targeted test coverage: all four providers (incl. legacy
visualstudio.com), transient-5xx-then-success, network-error-then-success, and two-transient-then-throw.
Issues
Blockers (Must Fix Before Merge)
None.
Should Fix
S1. PR description + commit subject omit the retry behavior — packages/spacecat-shared-cloud-manager-client/src/index.js
The title/body only describe the provider URL additions, but the diff also adds a single-retry loop to createPullRequest. That is a real behavior change for all callers of this shared method (eds-csp, accessibility, vulnerabilities, cwv): up to ~1.5s extra latency on a transient failure, plus a changed thrown-error message format (HTTP <status> - ... and ...failed after 2 attempts: ...). Since merging triggers a patch release, the semantic-release changelog — generated from the commit subject — will not mention the retry. Please update the PR body/commit subject so the shipped behavior is captured.
Perspective: Engineering Practices / Product
Nice to Have
N1. The 422 test doesn't guard the no-retry-on-4xx contract — packages/spacecat-shared-cloud-manager-client/test/cloud-manager-client.test.js:1945
it('throws on failed PR creation') uses a single nock interceptor and asserts rejectedWith('Pull request creation failed'). That matcher is a prefix that also matches the retry-exhausted message Pull request creation failed after 2 attempts, and there's no scope.isDone() assertion — so a regression that retried a 4xx would still pass this test. Consider asserting expect(scope.isDone()).to.be.true (proving exactly one request) or matching the exact Pull request creation failed: HTTP 422 message.
Perspective: QA / Testing
N2. Host detection via substring includes() — see inline comment. Pre-existing pattern extended to the new providers; low risk. Noted inline.
Perspective: Architecture
Verdict
Ready to merge: Yes (approving) — address S1 (description) before/at merge so the release notes are accurate.
Reasoning: Correct, backward-compatible, well-tested; the only Should-Fix is documentation of a shipped behavior change.
Note: reviewed against the PR branch with full repo context. I did not execute the test suite locally (monorepo node_modules not installed); relying on CI + static analysis. No downstream code in the sibling repos string-matches the old error message, so the message-format change is safe.
…ot full URL Addresses review feedback (N2): matching provider domains via substring includes() on the whole repoUrl could misclassify a URL whose *path* contains another provider's domain (e.g. https://bitbucket.org/team/github.com-mirror). Parse the hostname with URL() and match on that instead. Behaviour is unchanged for normal https repo URLs; adds tests for the path false-positive and an unparseable repoUrl. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rpapani
changed the title
ramboz
left a comment
ramboz
left a comment
There was a problem hiding this comment.
Re-approving at e03cc82.
The hostname-detection refactor cleanly closes the earlier N2 (path false-positive): provider is now resolved from the parsed hostname via extractRepoHost/URL(), with tests for both the bitbucket.org/team/github.com-mirror path false-positive and an unparseable repoUrl. Behavior is unchanged for normal https repo URLs. I re-traced the retry control-flow as well — every loop exit lands on either a break (ok response) or a throw, so response.json() never runs on a non-ok/undefined response.
S1 (retry undocumented) is resolved: the PR body + title now describe the retry, and it has its own fix: commit so semantic-release will capture it in the changelog regardless of merge strategy.
No blockers. One optional, non-blocking test-robustness nit left inline (N1).
…R-creation test Addresses ramboz's re-review (N1) on #1793: the previous assertion matched the prefix 'Pull request creation failed', which also matches the retry- exhausted message, so a regression that retried a 4xx would still pass. Now assert the exact permanent-failure message ('Pull request creation failed: HTTP 422') and `scope.isDone()` — a single interceptor proves no second request (retry) was attempted on a permanent 4xx. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## [@adobe/spacecat-shared-cloud-manager-client-v1.4.6](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-cloud-manager-client-v1.4.5...@adobe/spacecat-shared-cloud-manager-client-v1.4.6) (2026-07-16) ### Bug Fixes * build BYOG PR URLs (Azure/Bitbucket) + hostname detection + retry-once ([#1793](#1793)) ([a6c12a2](a6c12a2))
|
🎉 This PR is included in version @adobe/spacecat-shared-cloud-manager-client-v1.4.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Problem
CloudManagerClient#createPullRequestonly constructed apullRequestUrlfor GitHub and GitLab. For Azure DevOps and Bitbucket BYOG repos it returnednull, sopullRequestUrlwas never set — even though the CM Repo API had created the PR (and returned itsexternalNumber). Downstream, the ASO UI showed no "View PR" link for those repos. Separately, PR creation had no resilience to transient CM-API blips.Changes
/pullrequest/{n}(dev.azure.com,*.visualstudio.com), Bitbucket Cloud/pull-requests/{n}(bitbucket.org). Unknown hosts still returnnull.extractRepoHostviaURL()), not a substring match on the whole URL — so a provider domain appearing in the repo path (e.g.bitbucket.org/team/github.com-mirror) can't be misclassified. (Addresses review feedback.)createPullRequestretries a single time after a short delay on a transient failure (5xx / 429 / network error); permanent 4xx failures throw immediately.Tests
Unit tests for all four providers (incl. legacy
visualstudio.com), the path-false-positive case, an unparseablerepoUrl, and the retry paths (transient-then-success, network error, two-consecutive-failures). Package stays at 100% coverage.Related PRs (cross-repo)
🤖 Generated with Claude Code