Add Bitbucket adapter - #125
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryAdds a Bitbucket Cloud adapter and extends the shared VCS contract to support provider-specific webhook identifiers and batched webhook events.
Confidence Score: 2/5This PR is not safe to merge until webhook recovery stops returning identifiers from incomplete listings and the outstanding webhook event-loss, orphaning, and credential-exposure failures are addressed. A failed later webhook-list page can make UUID recovery select the wrong hook, unresolved UUID recovery can leave active hooks unmanaged, getEvent still discards later refs in multi-ref pushes, and archive and clone outputs still expose the access token. Files Needing Attention: src/VCS/Adapter/Git/Bitbucket.php Important Files Changed
Prompt To Fix All With AI### Issue 1
src/VCS/Adapter/Git/Bitbucket.php:1035-1037
**Partial hook list selects wrong UUID**
When a later page of the hook listing fails after an earlier page contains one matching URL, this branch treats that partial result as complete and returns the older hook's UUID. Subsequent cleanup deletes the older webhook while the newly created webhook remains active and unmanaged.
```suggestion
if (($responseHeaders['status-code'] ?? 0) >= 400) {
return null;
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (26): Last reviewed commit: "fix: return the routable slug and a real..." | Re-trigger Greptile |
|
@greptile fixed the first three in aaa8801.
|
|
All three fixes look correct from the diff and tests. 1 — 2 — 3 — 4 — Clone URL: Fair point, and I agree it should be a single cross-adapter change if pursued. Happy to drop that comment. No remaining blockers from my side on these four items. |
Bitbucket identifies a hook by UUID, which an int return cannot carry, so createWebhook() threw and a separate createRepositoryWebhook() returned the UUID. Widening the return to int|string lets Bitbucket implement the same method every other adapter does.
Bitbucket declares the parts of the contract it does not offer - presigned archive urls, check runs, namespace listing, computed languages, user lookup by handle, webhook delivery to the catcher, and file lists in a push event - and keeps only the tests describing itself: workspaces, uuid webhooks and user lookups, multi-ref pushes, and its action mapping. The event payload builders replace hand-written getEvent tests, so the shared assertions cover them, and Base's repository id is now overridable because Bitbucket routes on "workspace/slug" rather than a numeric id. getEvent() returned an empty event for a malformed payload rather than throwing, as the other adapters do.
aaa8801 to
fe8e115
Compare
|
@greptile This matches the existing pattern in GitHub::generateCloneCommand() and GitLab::generateCloneCommand() both embed the access token in the git remote add origin URL the same way, so this isn't a regression specific to Bitbucket. Fixing it properly (e.g. via a credential helper instead of an embedded URL) would need to happen across all three adapters together worth tracking as a separate follow-up rather than a Bitbucket-only patch here. |
|
That's a fair point. The token-in-URL pattern is consistent across GitHub, GitLab, and now Bitbucket — this PR isn't introducing anything new. A proper fix (credential helper, Agreed — worth tracking as a repo-level follow-up rather than blocking here. |
|
Re the two Greptile findings: Clone command exposes the access token — this matches the existing pattern in
public function getEvents(string $event, string $payload): array
{
return [$this->getEvent($event, $payload)];
}Every other adapter gets this for free with no behavior change; Bitbucket already overrides it correctly. |
Same bug as the OAuth2 side, independently: username/nickname aren't workspace identifiers for accounts migrated to Atlassian's unified identity, and the account UUID doesn't double as the workspace UUID. Silently returned zero repositories rather than an error, since searchRepositories() treats any 4xx as an empty result. Resolves the workspace via /user/workspaces -- the endpoint Atlassian's migration guidance names as the replacement for the cross-workspace /workspaces listing CHANGE-2770 removed -- falling back to the old username/nickname behavior only if that call fails.
resolveRef() above already catches Exception and converts it to FileNotFound() for the expected "file/ref doesn't exist" case, but the two call() invocations below it didn't -- so a 404 whose response body doesn't decode as JSON (a real, common case: not every repo has package.json) propagated as an uncaught fatal, crashing the whole Swoole worker (confirmed live) rather than resulting in a normal FileNotFound for this one lookup.
Was missing entirely, throwing the base Git class's "not supported" default -- confirmed live, this crashed VCS site/function deployments outright (Compute/Base.php calls it unconditionally for every provider). Bitbucket's archive download lives on the browser host, not the API host, and only supports zip/gz/bz2 (no "tarball" extension distinct from gz). Auth is URL-embedded the same way generateCloneCommand() already does it, since this URL is handed off for a plain download rather than called with a bearer header.
…igned urls Follow-ups from review of the presigned-url commit: - Archive extension was `.gz`; verified against a live repo that `.tar.gz` serves the same gzipped tarball and matches the shared contract's default $presignedTarballFragment, so no override is needed. - Ref is now encoded keeping slashes, as Gitea and GitHub do, so nested branch names (feature/foo) resolve. - Documented why the credential travels as basic userinfo rather than the query parameter GitLab and Gitea use: Bitbucket answers this directly instead of redirecting to a signed url (so GitHub's redirect-following approach isn't available), and its ?access_token= form was removed in CHANGE-3052. - createWebhook() threw away a missing uuid, returning '' and leaving the caller unable to delete the hook it just created. Now throws, mirroring GitHub::createWebhook()'s guard. - Dropped $supportsPresignedUrls = false so the two shared contract tests run instead of asserting the method throws -- it no longer does. Folded the tag-only push case into the multi-ref test rather than keeping a separate test for the same rule.
The method name collided with Base::testGetUser(), so PHPUnit ran only Bitbucket's override and never reached Base's skipUnlessSupported() check -- meaning $supportsUserLookup = false correctly skipped testGetUserWithInvalidUsername but silently did nothing for testGetUser. Renamed to testGetUserByUuid so it no longer overrides a Base test by name, matching every other adapter's tests (none of which redefine a Base method -- they only add new ones or override the hook methods Base already exposes for this).
|
@greptile Both flagged points are intentional, not gaps: getEvent() only returning the first batched event — this is deliberate, not truncation. Bitbucket batches multiple ref changes into one webhook delivery; getEvent() keeps returning just the first to preserve the existing single-event contract every other adapter and all current callers rely on. The new getEvents() (see Adapter.php) returns the full set. This split is explicitly covered by BitbucketTest::testGetEventsReportsEveryPushedBranch, which asserts getEvent() returns $events[0] while getEvents() returns all of them. Access token embedded in clone/archive URLs — this matches the pattern already established by the other adapters, not a new exposure: generateCloneCommand: GitHub embeds :{token}@github.com, GitLab embeds oauth2:{token}@GitLab.... Bitbucket's x-token-auth:{token}@... is the same convention with Bitbucket's required auth username. |
|
Both points are well-taken — you're right on both counts.
Token in URLs: You're correct that this isn't a new exposure — GitLab's |
|
@greptile re-review |
|
@greptile Both flagged points were just withdrawn in the thread above — the getEvent()/getEvents() split is confirmed correct and covered by tests, and the token-in-URL pattern is confirmed to match GitHub/GitLab's existing behavior exactly, not a Bitbucket-specific regression. Given both P1 findings are now withdrawn, can you re-run the review and update the confidence score? Curious what's still holding it down — if there's another concern behind the number, let's get it out in the open rather than leave it implicit in a stale score. |
…ract Three things a consumer would have had to special-case by provider: - listWorkspaces() was a new, Bitbucket-only public method instead of an override of the existing generic listNamespaces(). Removed rather than mapped onto it: the shared test asserts both 'user' and 'group' kinds, which Bitbucket workspaces have no way to distinguish. Left unsupported, same as GitHub and Gitea already declare. - getAuthenticatedUser() was public for no reason but its own test calling it from outside the class. Made protected, matching Gitea's getAuthenticatedUserLogin() -- both used only internally by getOwnerName(). Its test (testGetUserByUuid) is dropped along with it; $supportsUserLookup = false already skips the shared user-lookup tests the same way it does for any other unsupported capability. - deleteWebhook() existed only on Bitbucket, so a caller wanting to clean up a webhook it created would have had no generic way to do it for any other provider. Promoted to the abstract Git contract alongside createWebhook()'s int|string return type, and implemented for GitHub, GitLab and Gitea (Gogs and Forgejo inherit Gitea's). Base gets a new testCreateAndDeleteWebhook() covering creation and deletion through the API alone, independent of $supportsWebhookDelivery, replacing Bitbucket's bespoke version.
Ran unconditionally on every adapter alongside testWebhookPushEvent, which already creates a webhook -- doubling create-webhook calls per suite run and tripping GitHub's secondary rate limit (HTTP 403 on CI). testWebhookPushEvent now deletes the webhook it already created instead of leaving it behind; the standalone API-only test only runs for an adapter that skips webhook delivery, so it never runs alongside it. Also dropped the redundant "Delete a webhook from a repository." docblocks added to each concrete deleteWebhook() -- GitHub.php, GitLab.php and Gitea.php don't add one-line restating comments on equally simple methods like deleteRepository() or createComment(), so deleteWebhook() shouldn't either.
Used a raw if/markTestSkipped with its own message instead of the skipUnlessSupported() helper every other capability check in this file already goes through. Also dropped an inline comment restating what the test's own docblock already says.
testCreateAndDeleteWebhookWithoutDelivery ran for GitHub because it shares GitHub's $supportsWebhookDelivery = false, but that flag is about delivery reachability, not whether creating a webhook works at all -- a GitHub App installation token gets 403 "Resource not accessible by integration" from the classic per-repo webhook endpoint, a permission limitation this library has apparently never had a test exercise before (testWebhookPushEvent/PullRequestEvent were already skipped for GitHub for the same pre-existing flag). Split the capability in two: $supportsWebhookDelivery (delivery reaches the test catcher) and the new $supportsWebhookCreation (creating a webhook through the API works at all). GitHub declares the latter false; Bitbucket keeps the default true, since it creates a webhook fine and only fails to have Bitbucket Cloud reach a local address.
Git is Adapter's only subclass -- every concrete adapter extends Git, so the split has no current purpose. It also already put the other webhook methods (getEventHeaderName, getSignatureHeaderName, getSupportedWebhookScopes) on Adapter while these two sat on Git alone. Moves both next to getSupportedWebhookScopes so every webhook-related abstract lives on the same class. No behavior change -- every adapter already implements both concretely.
createWebhook() threw once Bitbucket confirmed creation without a uuid in the body, but by then the hook was already live and delivering events with no id left to call deleteWebhook() with -- an unrecoverable leak. Falls back to listing the repository's webhooks and matching by url to recover the uuid before giving up.
findWebhookUuid()'s url match could return an older webhook sharing the same delivery url instead of the one just created, silently handing the caller the wrong uuid to manage. Picks the match with the latest created_at instead of the first one found. Also extracted two blocks duplicated verbatim elsewhere in this file: the x-token-auth credential embedding shared by getRepositoryPresignedUrl() and generateCloneCommand(), and the linked-vs-raw author name resolution shared by parseCommit() and parsePushChange().
created_at can tie or race with a concurrent creation, so picking the "newest" match was still a guess -- and a wrong guess is worse than the original problem: the caller would later delete an unrelated webhook while the one it actually just created stays orphaned and unmanaged. findSingleWebhookByUrl() only returns a uuid when exactly one webhook has that url; any ambiguity or none at all now surfaces a clear error naming the repository and url to check manually, rather than silently resolving to a webhook that might not be the right one.
…adapter An exhaustive pass over the diff turned up more of what earlier rounds already fixed piecemeal: Duplication: - resolveRef() and resolveDefaultBranch() repeated the same mainbranch lookup, differing only in the empty case -> mainBranchName(). - listSource() and getRepositoryContent() repeated the resolveRef/ normalize/build-/src/-url preamble verbatim -> sourceUrl(). - normalizeRepository() and getEventRepositoryOwner() both derived the workspace slug with a full_name fallback -> workspaceSlugOf(). - searchRepositories() re-implemented the id/private/pushed_at mapping normalizeRepository() exists to centralize; it now runs through it. - getRepositoryName() re-issued the request getRepository() already makes; it now splits the "workspace/slug" id and delegates. - The "no numeric repository ids" note was stated six times; it is now stated once, in normalizeRepository() where the id is minted. Dead code: - setBitbucketUrl() had no caller anywhere. - $refreshToken was assigned and never read. - getEventRepositoryId() wrapped a single strval(); inlined. - Base's $supportsPresignedUrls was never set false by any adapter, so both branches it guarded were unreachable. - BitbucketTest read TESTS_BITBUCKET_ENDPOINT, which no compose file, workflow or doc sets. Consistency with the sibling adapters: - $body -> $responseBody, $SHA -> $commitHash. - listRepositoryLanguages() returns [] for a missing repository rather than throwing, matching the other list-style getters. - Dropped defensive is_array() ternaries where plain ?? [] is what GitHub/GitLab/Gitea write, and docblocks that restated the method name or the contract in Adapter.php. - The inverted skipUnlessSupported(!$flag, ...) in Base now reads as a plain markTestSkipped with a message that says what is happening.
…us urls
Two correctness bugs the shared suite can't currently catch, because the
bitbucket CI job skips every test for want of credentials:
- getRepositoryName() returned Bitbucket's free-form display `name`, not
the `slug` its API routes on and every other method here takes as
$repositoryName. GitLab has the same split and deliberately returns
`path`; on GitHub and Gitea the two are the same value, so Bitbucket
was the one adapter that picked the non-routable field. Any repository
whose display name isn't already slug-form ("My Site" vs "my-site")
would 404 clone, branch and commit-status calls downstream. Base can't
see it: createRepository() posts name == slug, so the two never differ
for anything the suite creates.
- getRepositoryContent() reported the last-touching commit hash as `sha`,
where Base::testGetRepositoryContentReportsBlobSha (ungated, shared)
asserts it is the git blob id, and GitHub/GitLab/Gitea all return a
real one. The adapter has the bytes, so it now computes the blob id
git itself stores rather than substituting a different hash.
Also:
- Adapter::getEvents() default returned [[]] for an event the adapter
doesn't report, where an overriding adapter returns []. It now drops
the empty event so the two agree.
- Base asserted a numeric webhook id in one of the three places it
checks one, left over from before the contract widened to int|string.
- Bitbucket's bespoke commit-status test re-ran Base::testGetCommitStatuses
verbatim to change one assertion; Base now exposes an
assertCommitStatusUrl() hook (no-op default) that BitbucketTest fills
in, dropping a whole repository round trip.
- Inlined the last single-use helpers (resolveRef and encodeRepositoryPath
into sourceUrl, resolveDefaultBranch into createFile,
getEventRepositoryOwner into its two callers).
| if (($responseHeaders['status-code'] ?? 0) >= 400) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
When a later page of the hook listing fails after an earlier page contains one matching URL, this branch treats that partial result as complete and returns the older hook's UUID. Subsequent cleanup deletes the older webhook while the newly created webhook remains active and unmanaged.
| if (($responseHeaders['status-code'] ?? 0) >= 400) { | |
| break; | |
| } | |
| if (($responseHeaders['status-code'] ?? 0) >= 400) { | |
| return null; | |
| } |
Knowledge Base Used: VCS Core Adapter Framework
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/VCS/Adapter/Git/Bitbucket.php
Line: 1035-1037
Comment:
**Partial hook list selects wrong UUID**
When a later page of the hook listing fails after an earlier page contains one matching URL, this branch treats that partial result as complete and returns the older hook's UUID. Subsequent cleanup deletes the older webhook while the newly created webhook remains active and unmanaged.
```suggestion
if (($responseHeaders['status-code'] ?? 0) >= 400) {
return null;
}
```
**Knowledge Base Used:** [VCS Core Adapter Framework](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/vcs/-/docs/vcs-core-adapter.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
Adds a Bitbucket Cloud adapter (API 2.0), implementing every abstract on
Adapter/Git: repositories, source/tree/content, branches, tags, commits, build statuses, pull requests, comments, webhooks, clone commands and webhook event parsing. Authenticates with a Bearer access token (OAuth 2.0, workspace or repository token), passed throughinitializeVariables(accessToken:)like the GitLab and Gitea adapters.Endpoint shapes were taken from Bitbucket's published OpenAPI spec, including the
POST /srcform-field contract (file paths are sent as/-prefixed field names so a file namedmessageisn't read as commit metadata) and its documented empty-repo / new-branch behavior.Where Bitbucket doesn't fit the shared interface
Each of these is implemented and documented in place:
idis normalized toworkspace/slug, which is what its API routes on and whatgetRepositoryName()accepts.getOwnerName()therefore ignores$repositoryIdand resolves the token's own workspace.Git::createWebhook()now returnsint|stringanddeleteWebhook()takes what it returned.languagefield, reported when present.affectedFilesis always empty.getRepositoryPresignedUrl()embeds the credential as HTTP basic userinfo. GitHub returns its redirect target instead, and the?access_token=query form GitLab and Gitea use was removed from Bitbucket in CHANGE-3052.Responses are normalized onto the keys the other adapters report:
private,pushed_at,numberon pull requests, lowercased PR state, and commit states mapped both ways between Bitbucket's vocabulary and the shared one.Tests
BitbucketTestfollows the patternmainnow uses: the shared contract lives inBase, and an adapter's own file declares what it is and only tests what is true of it alone. It declares 7 tests of its own and runs 108 in total — 101 of them inherited.Bitbucket declares the parts of the contract it does not offer, rather than overriding tests to say so:
$supportsCheckRuns,$supportsNamespaceListing,$supportsRepositoryLanguages,$supportsUserLookup,$supportsWebhookDelivery,$supportsInstallationRepository,$resolvesOwnerFromRepositoryIdand$reportsAffectedFilesInPushEvent. Those skip the shared tests for the parts it does not offer.What stays Bitbucket's own: workspaces (its grouping in place of namespaces), UUID-keyed webhooks, user lookup by UUID, multi-ref pushes through
getEvents(), its event-to-action mapping, tag pushes not being reported as branches, the linked-vs-raw commit author, and a build status written without a URL. The hand-writtengetEventassertions are gone — the class suppliespushPayload()andpullRequestPayload()builders and the shared assertions cover them.Three additions to
Base, all defaulting to full support so the gap is the adapter's to declare:$supportsPresignedUrlsand$reportsAffectedFilesInPushEvent, new capability flags (Bitbucket declares only the latter -- it does support presigned urls).repositoryIdOf(), overridable, becauseBaseotherwise asserts a repository id is numeric.EVENT_*payload facts are read throughstatic::so an adapter can restate one; Bitbucket restates the repository id asworkspace/slug.testWebhookPullRequestEventalso now skips on$supportsWebhookDeliveryrather than only on pull request support, which is what actually stops Bitbucket Cloud from reaching the local request-catcher.One adapter fix came out of running the shared tests:
getEvent()returned an empty event for a malformed payload instead of throwing, as the other adapters do.composer lintandcomposer check(PHPStan level 8) pass. The suite is still credential-gated and the secrets are not set on this repo, so thebitbucketCI job currently skips all 112 tests and passes without asserting anything — it needsTESTS_BITBUCKET_ACCESS_TOKEN, and optionallyTESTS_BITBUCKET_WORKSPACE, whose workspace needs at least one project since Bitbucket assigns every new repository to one.