Skip to content

feat(github): add fetchPullRequestForBranch helper#2873

Merged
kilo-code-bot[bot] merged 2 commits intoconvoy/associated-pr-for-cloud-agent-next-sessi/dbccdbdf/headfrom
convoy/associated-pr-for-cloud-agent-next-sessi/dbccdbdf/gt/maple/39995660
Apr 28, 2026
Merged

feat(github): add fetchPullRequestForBranch helper#2873
kilo-code-bot[bot] merged 2 commits intoconvoy/associated-pr-for-cloud-agent-next-sessi/dbccdbdf/headfrom
convoy/associated-pr-for-cloud-agent-next-sessi/dbccdbdf/gt/maple/39995660

Conversation

@kilo-code-bot
Copy link
Copy Markdown
Contributor

@kilo-code-bot kilo-code-bot Bot commented Apr 28, 2026

Summary

  • Adds fetchPullRequestForBranch, AssociatedPullRequest, and GitHubRateLimitError exports in apps/web/src/lib/integrations/platforms/github/adapter.ts.
  • Helper returns the most-recently-updated PR whose head ref matches ${owner}:${branch}, preferring open PRs when multiple exist; maps merged_at != null to 'merged'.
  • On rate-limit (403/429 or x-ratelimit-remaining: 0) throws GitHubRateLimitError with the parsed x-ratelimit-reset timestamp; on 404 returns null with a warning log.
  • Intentionally no caching or in-flight dedup — caller (bead 3) is throttled to once per 10s per session.
  • Bead 1 of 5 in the 'Associated PR for cloud-agent-next session' convoy. Bead 3 will call this from the refreshAssociatedPullRequest mutation.

Verification

  • Sibling test file fetch-pull-request-for-branch.test.ts mocks @octokit/rest and @octokit/auth-app and covers: single open PR happy path, open-preferred-over-closed, merged state mapping, fallback to first when no open, empty result → null, 404 → null, 403/429 → GitHubRateLimitError with correct resetAt, header-only rate-limit signal, and passthrough of unexpected errors. Tests bypass the global adapter mock (moduleNameMapper entry) by importing via relative path.
  • Test run locally blocked: no Docker/Postgres in this environment, so pnpm test could not execute (globalSetup requires Postgres). Typecheck/lint not run per town policy ("Don't run the linters in the cloud repo … Don't run the typechecker unless you absolutely have to."). CI will validate.

Visual Changes

N/A

Reviewer Notes

  • The __mocks__ mirror at apps/web/src/tests/setup/__mocks__/lib/integrations/platforms/github/adapter.ts was updated with no-op stubs for the new exports so tests that import the mocked adapter still typecheck.
  • Rate-limit detection uses header-based fallback (x-ratelimit-remaining: 0) in addition to status codes, so GitHub's secondary-rate-limit quirks are covered. parseRateLimitResetAt falls back to now + 60s if the reset header is missing/invalid so callers always have a usable Date.
  • Relative-import trick in the test to bypass the moduleNameMapper adapter mock is the only way to exercise the real implementation without invalidating the mock for every other test file.

Adds a small Octokit helper that looks up the PR associated with a
(repo, branch) pair using an existing GitHub App installation token.
Used exclusively by the manual 'Refresh PR info' mutation — the common
case is webhook-driven. Intentionally no caching or in-flight dedup
since the caller is throttled to once per 10s per session.

- Returns most recently updated PR whose head ref matches, preferring
  open PRs when multiple exist.
- Maps merged_at != null to a 'merged' state.
- Throws GitHubRateLimitError on 403/429 or x-ratelimit-remaining=0,
  surfacing the parsed reset timestamp so callers can report it.
- Returns null on 404 (repo no longer accessible / deleted).

Bead 1 of 5 in the 'Associated PR for cloud-agent-next session' convoy.
@kilo-code-bot
Copy link
Copy Markdown
Contributor Author

kilo-code-bot Bot commented Apr 28, 2026

Refinery code review passed.

Scope check: Matches bead 1 acceptance — fetchPullRequestForBranch, AssociatedPullRequest, and GitHubRateLimitError all exported from the adapter with the documented behavior.

Correctness:

  • Head filter ${owner}:${branch} with state=all, sort=updated, direction=desc matches spec
  • Open-preferred selection: prs.find(pr => pr.state === 'open') ?? prs[0] is correct
  • State mapping handles the Octokit quirk (merged PRs come back as state: 'closed' with merged_at set)
  • Rate-limit detection covers 403, 429, and the header-only x-ratelimit-remaining: 0 case (sensible for GitHub's secondary-rate-limit behavior)
  • parseRateLimitResetAt fallback to now + 60s when the reset header is missing/invalid is a reasonable default
  • 404 returns null with a warn log; unknown errors are re-thrown unchanged

Tests: Sibling test file exercises all acceptance cases (single-open, open-preferred, merged mapping, no-open fallback, empty → null, 404 → null, 403/429/header-only rate-limit, unknown error passthrough). The relative-import trick to bypass the global moduleNameMapper adapter mock is clearly documented in the file header and is the only clean way to exercise the real implementation.

Mock adapter: Updated with no-op stubs for fetchPullRequestForBranch and matching type/class exports, so consumers using the mocked adapter continue to typecheck.

Minor (non-blocking): installationId: number parameter differs from sibling helpers in this file that take installationId: string. The String(installationId) conversion before the token call is correct; worth noting for consistency but not worth blocking on.

No security concerns — no logged tokens, no bypassed auth, no unsafe patterns. Good to merge once bead 3 consumes it.

Comment thread apps/web/src/lib/integrations/platforms/github/adapter.ts Outdated
@kilo-code-bot
Copy link
Copy Markdown
Contributor Author

kilo-code-bot Bot commented Apr 28, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • apps/web/src/lib/integrations/platforms/github/adapter.ts
  • apps/web/src/lib/integrations/platforms/github/fetch-pull-request-for-branch.test.ts
  • apps/web/src/tests/setup/__mocks__/lib/integrations/platforms/github/adapter.ts

Reviewed by gpt-5.5-20260423 · 494,973 tokens

Non-rate-limit 403 responses (e.g. installation missing pull request
permission) were being wrapped as GitHubRateLimitError, which would
incorrectly tell users to retry instead of surfacing the permission
problem. Now only treat 403 as rate-limited when the x-ratelimit
headers or the error message ('rate limit', 'secondary rate limit',
'abuse') indicate so. 429 remains unambiguously rate-limited, and
x-ratelimit-remaining: 0 still triggers the rate-limit path at any
status.
@kilo-code-bot kilo-code-bot Bot merged commit 3e32e34 into convoy/associated-pr-for-cloud-agent-next-sessi/dbccdbdf/head Apr 28, 2026
2 checks passed
@kilo-code-bot kilo-code-bot Bot deleted the convoy/associated-pr-for-cloud-agent-next-sessi/dbccdbdf/gt/maple/39995660 branch April 28, 2026 21:34
kilo-code-bot Bot added a commit that referenced this pull request Apr 29, 2026
* feat(github): add fetchPullRequestForBranch helper

Adds a small Octokit helper that looks up the PR associated with a
(repo, branch) pair using an existing GitHub App installation token.
Used exclusively by the manual 'Refresh PR info' mutation — the common
case is webhook-driven. Intentionally no caching or in-flight dedup
since the caller is throttled to once per 10s per session.

- Returns most recently updated PR whose head ref matches, preferring
  open PRs when multiple exist.
- Maps merged_at != null to a 'merged' state.
- Throws GitHubRateLimitError on 403/429 or x-ratelimit-remaining=0,
  surfacing the parsed reset timestamp so callers can report it.
- Returns null on 404 (repo no longer accessible / deleted).

Bead 1 of 5 in the 'Associated PR for cloud-agent-next session' convoy.

* fix(github): only classify 403 as rate limit when message indicates it

Non-rate-limit 403 responses (e.g. installation missing pull request
permission) were being wrapped as GitHubRateLimitError, which would
incorrectly tell users to retry instead of surfacing the permission
problem. Now only treat 403 as rate-limited when the x-ratelimit
headers or the error message ('rate limit', 'secondary rate limit',
'abuse') indicate so. 429 remains unambiguously rate-limited, and
x-ratelimit-remaining: 0 still triggers the rate-limit path at any
status.

---------

Co-authored-by: Maple (gastown) <Maple@gastown.local>
kilo-code-bot Bot added a commit that referenced this pull request Apr 30, 2026
* feat(github): add fetchPullRequestForBranch helper

Adds a small Octokit helper that looks up the PR associated with a
(repo, branch) pair using an existing GitHub App installation token.
Used exclusively by the manual 'Refresh PR info' mutation — the common
case is webhook-driven. Intentionally no caching or in-flight dedup
since the caller is throttled to once per 10s per session.

- Returns most recently updated PR whose head ref matches, preferring
  open PRs when multiple exist.
- Maps merged_at != null to a 'merged' state.
- Throws GitHubRateLimitError on 403/429 or x-ratelimit-remaining=0,
  surfacing the parsed reset timestamp so callers can report it.
- Returns null on 404 (repo no longer accessible / deleted).

Bead 1 of 5 in the 'Associated PR for cloud-agent-next session' convoy.

* fix(github): only classify 403 as rate limit when message indicates it

Non-rate-limit 403 responses (e.g. installation missing pull request
permission) were being wrapped as GitHubRateLimitError, which would
incorrectly tell users to retry instead of surfacing the permission
problem. Now only treat 403 as rate-limited when the x-ratelimit
headers or the error message ('rate limit', 'secondary rate limit',
'abuse') indicate so. 429 remains unambiguously rate-limited, and
x-ratelimit-remaining: 0 still triggers the rate-limit path at any
status.

---------

Co-authored-by: Maple (gastown) <Maple@gastown.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants