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 |
…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.
Reverts everything that wasn't Bitbucket's to change: - deleteWebhook() is gone entirely. It had no caller anywhere -- not in Appwrite (RepositoryWebhooks only ever creates), not in cloud, and not in the suite except the tests written to exercise it. Test cleanup didn't need it either, since discardRepositories() deletes the whole repository. With it goes the uuid-recovery fallback: createWebhook() throws again when Bitbucket omits the uuid, as GitHub.php does, and Base loses $supportsWebhookCreation and the create/delete test, which existed only to drive it. - createWebhook() moves back to Git.php where it was; only its return type widens to int|string, which Bitbucket needs to satisfy the existing contract at all. - GitHub.php, GitLab.php, Gitea.php and GitHubTest.php are untouched again, as is the README table for the other providers. - Dropped testUpdateCommitStatusDefaultsUrlToCommit rather than keeping it or moving it into Base behind a hook. Base::testGetCommitStatuses already writes a status with an empty target_url and reads it back, so it already covers the defaulting for Bitbucket; the extra test only pinned which url was substituted, at the cost of repeating the whole repository round trip. What still touches shared files is what Bitbucket cannot run without: getEvents() on Adapter (its push batches refs), and in Base the repositoryIdOf() hook, the $reportsAffectedFilesInPushEvent flag, the self:: -> static:: reads so an adapter can restate an EVENT_* fact, and two skips for capabilities Bitbucket lacks.
An OAuth consumer is free where a workspace access token may not be, and the token it hands back lasts two hours, so CI mints one before starting the stack rather than keeping a long-lived credential around. The value goes straight into GITHUB_ENV, masked, and is never stored as a secret -- only the consumer's client id and secret are, since minting has to authenticate as the consumer. With no consumer configured the step is a no-op and the suite skips, the same as it does today. Also documents in CONTRIBUTING why an Atlassian account API token (the ATATT kind) answers 401 here: it authenticates as email:token over HTTP Basic, and the adapter sends the token as a Bearer credential.
curl -sf discards the error body, so a refused token request surfaced as a bare "could not mint" with nothing to act on. Captures the status and body instead and echoes Bitbucket's own error_description.
createRepository() reported only the status code, so a 400 gave nothing to act on. createWebhook() already includes the response body; this matches it.
CI mints its own token, so the manual steps were setup lore rather than something a contributor needs in the repo.
EVENT_REPOSITORY_ID is the one EVENT_* fact an adapter overrides, so it is the only one that needs late static binding to resolve. The other eight were changed for consistency and are churn this PR doesn't need.
createFile() defaulted an empty repository's branch to 'main' on the belief that Bitbucket names the first branch after whatever the commit asks for. It doesn't -- the commit lands on Bitbucket's own default and 'main' never exists, so every later call that named it failed: 404 from getLatestCommit(), 400 from createBranch() resolving it as a target, and empty results from listSource(), which swallows the 404 into []. It only ever surfaced now because the suite skipped for want of credentials until this run. Omits the branch when there is none to name, letting Bitbucket create its default, and declares that default as 'master' in BitbucketTest the way GogsTest already does for Gogs.
The adapter only spoke Bearer, so an Atlassian account API token -- the ATATT kind, which authenticates as email:token over HTTP Basic -- answered 401 on every call, and testing meant standing up an OAuth consumer with the client-credentials grant enabled. An email:token pair always carries a colon and a bare token never does, so the credential names its own scheme and initializeVariables() keeps its signature. Centralising the header also collapses 26 copies of 'Bearer ' . $this->accessToken into one place, and clone/archive URLs reuse the same distinction: an email:token pair is already valid userinfo, where a bare token needs the x-token-auth username Bitbucket pairs it with. CI drops the minting step it needed for client credentials and passes TESTS_BITBUCKET_ACCESS_TOKEN straight through again.
A bare token and an email:token pair fail very differently, and the 401 they produce looks the same from the outside. Reports the shape -- never the value -- so a misformed secret is obvious rather than inferred.
The endpoint answers with workspace_access objects that carry the
workspace under its own key:
{"type":"workspace_access","administrator":true,
"workspace":{"slug":"utopiavcs", ...}}
getOwnerName() read $values[0]['slug'], which is never set, so it always
fell through to the account handle -- and Bitbucket's repository API
does not accept a handle as a workspace, so every call built from it
answered 404. The nested read was there originally; I removed it while
trimming comments, on the mistaken belief that it covered an endpoint
this code never calls. It is the only shape this endpoint returns.
A minting step was overwriting TESTS_BITBUCKET_ACCESS_TOKEN with a token minted from the OAuth consumer, so the configured credential was never used: CI authenticated as the consumer, resolved the consumer's workspace, and answered 403 against any other. Removes it, along with the credential-shape check that existed only to debug that confusion.
The endpoint rejects a page parameter -- "Invalid page", HTTP 400 -- and pages instead by handing back the url of the next page. listSource() asked for ?pagelen=100&page=1, so every listing failed, and because it read any error as an empty listing the failure surfaced as getRepositoryTree() and listRepositoryContents() quietly returning nothing at all. Follows the next url as given, and reports a failed listing rather than answering it with an empty array: only a 404, meaning the ref or path genuinely isn't there, is still an empty result. Silently swallowing the 400 is what kept this hidden.
getRepositoryTree() answered nothing for a branch like feature/test, because sourceUrl() percent-encoded the ref and Bitbucket matches a nested branch name against the path as written -- feature%2Ftest names no ref. getRepositoryPresignedUrl() already kept its slashes for exactly this reason, so the three places a ref reaches a path now share one encodeRef() rather than two of them disagreeing. getLatestCommit() had the same fault with no test to catch it.
getRepositoryTree() answered nothing for a branch like feature/test. Bitbucket takes the ref as one path segment, so the name read as the ref `feature` and the path `test`; percent-encoding the slash didn't separate them either, as the endpoint matches the ref against the path as written. The three places a ref reaches a url -- source listings, latest commit and the archive url -- now resolve a nested name to its hash first, which names the same commit with nothing left to misread. Names without a slash are passed through untouched, so the usual case still costs one request.
| public function getEvents(string $event, string $payload): array | ||
| { | ||
| $parsed = $this->getEvent($event, $payload); | ||
|
|
||
| // An event the adapter doesn't report describes nothing, so report | ||
| // nothing rather than one empty event | ||
| return $parsed === [] ? [] : [$parsed]; | ||
| } |
There was a problem hiding this comment.
Separate PR, Rename everything to "getEvents". So getEvent no longer exists
There was a problem hiding this comment.
FOllowup appwrite PR to consume it as array and loop it
Bitbucket has no checks api, but it has the build statuses a check run is reported through, so a run maps onto one: the key identifies it, the name names it, and the state carries how it went. A check run was addressed by an int, which Bitbucket has nothing to answer with -- a status is identified by its key under a commit, and carries no number. The id is a string now, and Bitbucket writes the commit into the one it hands out so a run stays reachable from the id alone. GitHub reports the number it already had as that string, so nothing it returns moves. Bitbucket holds four states where a check run has seven verdicts, so neutral, skipped and cancelled all stop a run and a run read back reports its state's verdict rather than the one it was written with.
The id came from a hook the adapters overrode, which named a pattern two call sites don't need. Both read it off the repository as it comes.
Trims the prose around the check runs and the ref lookup to the one or two lines the rest of the file spends, dropping what restated the code and the url note updateCommitStatus already carries.
The id names a workspace and a slug either side of a slash, and travels as one path segment, so callers encode that slash. Nothing decoded it, leaving getRepositoryName() to look for a separator that was no longer there and report every Bitbucket repository as missing. An id that arrives with its slash intact carries no escapes, so it comes back through unchanged.
main now asks every adapter for the events a delivery describes, so the wrapper this branch carried and the single-event read Bitbucket kept beside it both describe what the contract already says.
| private function parseCheckRun(array $checkRun): array | ||
| { | ||
| if (isset($checkRun['id'])) { | ||
| $checkRun['id'] = (string) $checkRun['id']; | ||
| } | ||
|
|
||
| return $checkRun; | ||
| } |
There was a problem hiding this comment.
Same as we dont want random classes from AI, we dont want random methods.
This we can cast to string in-line, this helper is overkill.
A method to set one key said less than the line it replaced.
2 participants
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.