Repository navigation
🪣 feat: Look Up Bitbucket Cloud Pull Requests - #16987
Conversation
296f5b6 to
31b1eb9
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31b1eb9288
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const newestOnly = closed && recorded == null; | ||
| const pageLength = newestOnly | ||
| ? 1 | ||
| : Math.min(Math.max(lookup.maxCandidatePullRequests, 1), MAX_PAGE_LENGTH); | ||
| const pages = newestOnly ? 1 : lookup.maxCandidatePages; |
There was a problem hiding this comment.
When head is omitted or invalid, this fetches only the newest closed pull request, even though matches() rejects fork-sourced candidates without a recorded commit. Bitbucket's filtering documentation confirms that a repository's pull-request collection includes incoming fork pull requests, so a newer closed fork PR with the same branch name hides an older matching same-repository PR and the lookup incorrectly returns null. Read the configured candidate page before choosing, as the GitLab source does.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6a7820a: closed history is read a full candidate page, so a rejected forked candidate no longer hides an older valid one.
| const pull = parsePullRequest(detail, repo); | ||
| /** A push between the list and the detail can move the head off the recorded commit. */ | ||
| if (!isSameCommit(pull.hash, chosen.hash) && !(await contains(pull.hash))) return null; |
There was a problem hiding this comment.
If an open pull request is mutated between the list and detail requests, this only revalidates a changed hash. Bitbucket explicitly permits changing an open pull request's branches through its update endpoint, so a pull request can move to another source branch or from the repository to a fork while retaining the same head hash; the lookup then reports it even though it no longer matches branch, and can bypass the fork rule above. Recheck sourceBranch and fromFork against the selected candidate and use matches() when the head changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6a7820a: the detail must keep the lane branch and fork status, and a moved head goes through the same fork rule.
| const commit = isRecord(source.commit) ? source.commit.hash : null; | ||
| if (typeof commit !== 'string' || !COMMIT_HASH.test(commit)) { | ||
| throw new PullRequestSourceError('UPSTREAM_ERROR'); |
There was a problem hiding this comment.
Bitbucket can retain an open pull request after its source branch is deleted, while the source commits may become unavailable or be garbage-collected, as documented in BCLOUD-21850 and BCLOUD-22730. Such a valid list item can therefore have no source.commit.hash; throwing here makes one stale pull request abort the entire branch lookup with UPSTREAM_ERROR, even when a later candidate is valid. Treat a missing source commit as an unmatchable candidate and continue the search.
AGENTS.md reference: AGENTS.md:L104-L110
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6a7820a: a pull request without a source commit is skipped as unmatchable; a malformed hash still fails the lookup.
c4718c2 to
481d77b
Compare
481d77b to
bc944ce
Compare
A role, group, user or tenant override can no longer change or remove pullRequests.host, so feature gating and link recording see the operator's host. Links are recorded only from the configured host, and the batch share key and the rate limit cooldown are scoped by host.
…are Key A role, group, user or tenant override can no longer change pullRequests.provider, so feature gating and link recording agree with the lookup, and a batch lookup for one provider is never joined by a request for another.
Merged and closed merge requests are listed on their own, so open fork merge requests on the branch name cannot crowd them out. Rate limit cooldowns are scoped by provider, and a workspace repository with a dot segment is rejected.
…e Commit Read a full closed page so a newer closed fork cannot hide an older pull request from the repository, recheck the source branch and fork status on the detail, and treat a pull request without a source commit as unmatchable instead of failing the lookup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc944ce58b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pages = newestOnly ? 1 : lookup.maxCandidatePages; | ||
| const all: ListItem[] = []; | ||
| for (let page = 1; page <= pages; page++) { | ||
| const items = await list(states, pageLength, page); |
There was a problem hiding this comment.
When maxCandidatePages is greater than 1 and the matching pull request is beyond the first page, this loop fabricates page=N URLs and list() discards the parsed next value. Bitbucket's pagination contract explicitly says that next is opaque, must be followed rather than constructed, and that only its absence marks the end; consequently an opaque cursor or a short page that still has next can make the search skip or stop before the matching candidate. Carry the validated next URL through list() and iterate it up to the configured page bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b47fc73: listAll now follows the validated next link up to maxCandidatePages and stops only when next is absent; covered by bitbucket.spec 'follows the next link Bitbucket returns instead of building page numbers'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a7820a81b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const [merged, closed] = await Promise.all([listAll('merged'), listAll('closed')]); | ||
| return [...(merged ?? []), ...(closed ?? [])].sort((a, b) => b.iid - a.iid); |
There was a problem hiding this comment.
When the matching GitLab merge request is in the locked state, it is omitted because GitLab treats locked as a distinct state filter rather than returning it for state=closed. The parser still explicitly accepts and maps locked merge requests, so replacing the prior state=all history query with only merged and closed makes these conversations incorrectly report no merge request; query the locked state as well or retain an all-state history query.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6192655: locked merge requests are now listed with the open ones, newest first, ahead of merged and closed history; covered by gitlab.spec 'finds a merge request while GitLab is merging it, ahead of older history'.
| if (fs.existsSync(gitHostKeyPath) && fs.existsSync(gitHostCertPath)) { | ||
| return; |
There was a problem hiding this comment.
After the generated certificate has existed for more than 30 days in a persistent checkout, this returns solely because both files exist even though the certificate is created with -days 30. The fake host then serves an expired certificate: Playwright's health probe ignores TLS errors, but the LibreChat process performing the pull-request lookup does not, so the enterprise-host scenario times out until the developer manually deletes .generated; check certificate validity and regenerate an expired pair.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a0c0803: the main process regenerates the fake-host key and certificate when openssl x509 -checkend reports they expire within 7 days, and workers never swap a pair under a running server.
Pull Request
Summary
With
provider: bitbucket, lookups had no source and failed as not configured. This adds a Bitbucket Cloud source behind the provider router: linked pull requests are read directly, the branch search uses BBQL (open first, then merged, declined and superseded), forks count only with the recorded commit, line counts sum the diffstat pages and checks come from the pull request's statuses for the head commit. Bitbucket returns 12-character commit hashes, so identity andmerge-basecontainment compare by prefix. Requests only ever go toapi.bitbucket.org, and a paginationnextlink anywhere else fails the lookup rather than being followed. Bitbucket Data Center is not supported; the host is pinned to Cloud.#16985 and #16986 merged before their last review fixes were pushed, so this PR also carries them:
providerandhostare restored from the YAML for every principal (BASE_ONLY_CONFIG_PATHS), batch share keys and rate limit cooldowns are scoped by provider and host, recorded links are matched through the host resolver, GitLab history reads merged and closed merge requests apart from open ones, and Playwright scenarios run against a fake GitHub Enterprise host. The Bitbucket source also rechecks the branch and fork on the pull request detail and skips a pull request whose source commit is gone.Part of a stack on #16982, merge from the bottom up: host (1), GitLab (2), Bitbucket (3), shared cache (4), user OAuth backend (5), connected account setting (6). Each PR is based on the one below it, so its diff is only its own link.
Type of change
Testing
Tested environments/configuration:
merge-basewith short and fork hashes, the statuscommit.hashfield, and the diffstat redirect.Automated tests:
packages/api:npx jest src/pullspasses (516 tests on the top link); breaking the fork rule fails three tests (checked).packages/data-schemas:resolution.spec.tscovers a role, user and tenant override ofhostandproviderbeing ignored.e2e/specs/mock/scenarios/pull-request-enterprise-host.spec.ts: the header shows a pull request from the fake enterprise host, and a principal override cannot move the host.npx tsc --noEmitandnpm run buildinpackages/apipass.Screenshots / recordings
No user-facing change
Risk / compatibility
A conflict state is not exposed by Bitbucket's API, so
mergeableis alwaysunknownfor Bitbucket.Checklist
pullRequestsfields are documented inlibrechat.example.yaml