Repository navigation
🏢 feat: Look Up Pull Requests on a GitHub Enterprise Host - #16985
Conversation
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: c1f6acac2d
ℹ️ 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".
| branch: lane.branch, | ||
| head: lane.head, | ||
| ...(lane.pullRequest != null ? { pullRequest: lane.pullRequest } : {}), | ||
| host: resolvePullRequestHost(settings.host) ?? undefined, |
There was a problem hiding this comment.
When the configured host is changed while a batch lookup is still running, a subsequent batch request using the same token, repository, branch, and policy joins that old request because the shareKey below omits input.host. It can therefore return a pull request URL and status from the previous GitHub installation even though the inner lookup cache correctly partitions by host. Include the normalized host in the batch shared-lookup identity.
Useful? React with 👍 / 👎.
| .join('|'), | ||
| ].join(','); | ||
| const key = `${scope}\0${repo}#${branch}\0${head ?? ''}\0${pullRequest ?? ''}\0${policy}`; | ||
| const key = `${scope}\0${host ?? ''}\0${repo}#${branch}\0${head ?? ''}\0${pullRequest ?? ''}\0${policy}`; |
There was a problem hiding this comment.
When an installation's configured hostname changes while retaining the same token, this host-aware cache key prevents result reuse, but cooldowns is still indexed only by the token-derived scope. A rate-limit response from the old host can therefore make lookups against the newly configured host return RATE_LIMITED without contacting it for up to an hour. Include the normalized host in the cooldown scope as well as the result key.
Useful? React with 👍 / 👎.
| const PULL_REQUEST_URL = | ||
| /https:\/\/github\.com\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/(\d+)(?![\w-])/g; | ||
| /https:\/\/[A-Za-z0-9.-]+(?::\d{1,5})?\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/(\d+)(?![\w-])/g; |
There was a problem hiding this comment.
When output contains https://github.com/o/r/pull/7 but the configured host is an Enterprise installation (or when the configured host changes after the lane was stored), this matcher persists only 7 and discards the source host. The lookup then reads PR 7 from the configured host and, for a same-repository PR whose head branch matches, accepts it without checking the recorded commit, so ordinary cross-host collisions in repository name, PR number, and branch can display an unrelated pull request instead of falling back to the correct branch search. Persist and validate the candidate's host, or require commit containment when its source host is unavailable or differs.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
c1f6aca to
efc238d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efc238db77
ℹ️ 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".
| .string() | ||
| .trim() | ||
| .max(253) | ||
| .regex(/^(?=[a-z0-9])[a-z0-9.-]*[a-z0-9](?::[0-9]{1,5})?$/i, { |
There was a problem hiding this comment.
When an operator mistypes a host such as foo..example.com or foo.-bar.com, this pattern accepts it, and resolvePullRequestHost repeats the same permissive check. The configuration therefore passes validation and enables the feature, but every lookup targets an invalid or unresolvable API hostname and returns UPSTREAM_ERROR. Validate each dot-separated DNS label as nonempty, at most 63 characters, and without leading or trailing hyphens.
Useful? React with 👍 / 👎.
Pull Request
Summary
Pull request lookups only ever talked to github.com: the API base, the page URL check and the link detection were all hard-coded, so a deployment on GitHub Enterprise Server or a
*.ghe.comdata residency tenant could not use the feature at all. This addsendpoints.agents.pullRequests.host, which moves every request of a lookup, the accepted page URLs and the cache key to that host (/api/v3on Enterprise Server,api.<host>onghe.com). Link detection now accepts a pull request URL on any host, since the number is only a candidate the lookup verifies on the configured host.The host decides where the deployment's token is sent, and pull request settings can be overridden per role, group or user from the database.
hostis therefore read from the operator's YAML only (the base config), never from an override, and a value that is not a plain host name turns the feature off instead of falling back to github.com.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:
Automated tests:
packages/api:npx jest src/pulls src/code/lane.spec.tspasses, including new cases for the API base per host type, every request staying on the configured host, a github.com page rejected under an Enterprise host, host keying in the cache, and the host taken from the base config when an override names another.packages/data-provider:npx jest src/config.spec.tspasses with accepted and rejected host values.npx tsc --noEmitinpackages/apiandpackages/data-provider, andnpm run buildinpackages/api, pass.Screenshots / recordings
No user-facing change
Risk / compatibility
Security-sensitive: an override can no longer redirect the token to another host. Without
hostthe behavior is unchanged.Checklist
pullRequestsfields are documented inlibrechat.example.yaml