rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

feat(github): PRReader + FileReader client (Phase 3) #80

New Issue
Closed
opened 2026-05-12 16:59:56 +00:00 by rodin · 2 comments
rodin commented 2026-05-12 16:59:56 +00:00
Owner
Copy Link
Copy Source

Architecture Reference

This issue is self-contained. For any design questions, the canonical reference is:
docs/DESIGN-vcs-abstraction.md
(Parts 2, 3, and 4 are most relevant to this issue)

Base branch: feature/github-support — open all PRs against this branch.

Goal

Implement PRReader and FileReader for the GitHub client, with a configurable base URL to support both github.com and GitHub Enterprise (github.concur.com).

Background

review-bot needs to post reviews on GitHub and GitHub Enterprise PRs. The vcs.Client interface and all canonical types live in vcs/ (created in issue #78).

The GitHub client uses diff-position natively — ReviewComment.Position maps directly to the GitHub API position field with no translation needed.

Auth: Authorization: Bearer <token>
Base URL: configurable; empty string defaults to https://api.github.com. For GitHub Enterprise (github.concur.com), the base URL is https://github.concur.com/api/v3.

Work

github/client.go

func NewClient(token, baseURL string) *Client
// empty baseURL defaults to https://api.github.com

HTTP helpers: set Authorization: Bearer <token>, Accept, Content-Type headers. Handle 429 with Retry-After.

github/pr.go

  • GetPullRequest — GET /repos/{owner}/{repo}/pulls/{number}

  • GetPullRequestDiff — same URL (GET /repos/{owner}/{repo}/pulls/{number}) with Accept: application/vnd.github.diff set per-request (overrides the default application/vnd.github+json). The response body is raw unified diff text — do not JSON-unmarshal it.

  • GetPullRequestFiles — GET /repos/{owner}/{repo}/pulls/{number}/files. Paginate through all pages (default 30/page, use ?per_page=100&page=N). For each file object, populate vcs.ChangedFile{Filename, Status, Patch} where Patch is the patch field from the response (the per-file unified diff hunk). Patch may be absent for binary files — set to "" in that case.

  • GetFileContentAtRef — GET /repos/{owner}/{repo}/contents/{path}?ref={ref}, base64-decode the content field. An empty ref omits the query parameter (uses default branch).

  • GetCommitStatuses — fetches both commit statuses and check runs, merges them into []vcs.CommitStatus:

    Commit statuses: GET /repos/{owner}/{repo}/commits/{sha}/status returns a JSON envelope {state, statuses: [...]} — unmarshal the wrapper object and extract the statuses array. Map each entry: Context→context, Status→state, Description→description, TargetURL→target_url.

    Check runs: GET /repos/{owner}/{repo}/commits/{sha}/check-runs (paginated). For each check run, map to vcs.CommitStatus{Context: name, Status: <mapped>, Description: conclusion, TargetURL: html_url}. Map check run conclusion to vcs.CommitStatus.Status as follows:

    • "success" → "success"
    • "failure", "action_required", "timed_out" → "failure"
    • "cancelled", "skipped", "neutral" → "success" (non-blocking)
    • nil / "in_progress" / "queued" → "pending"

    Return the merged slice. Deduplication is not required (different context names).

github/files.go

  • GetFileContent — delegates to GetFileContentAtRef(ctx, owner, repo, path, "") (empty ref = default branch)
  • ListContents — GET /repos/{owner}/{repo}/contents/{path} (no ref), returns array when path is a directory

Unit tests

For each method: happy path, 404, 401, 429 (with retry), malformed response.

Compile-time check

var _ vcs.PRReader = (*Client)(nil)
var _ vcs.FileReader = (*Client)(nil)

(Full vcs.Client check happens after #81 adds Reviewer + Identity.)

Exit criteria

  • go test ./github/... passes for all PRReader + FileReader methods
  • NewClient(token, "") uses https://api.github.com
  • NewClient(token, "https://github.concur.com/api/v3") targets GHE correctly
  • GetFileContent delegates to GetFileContentAtRef with empty ref
  • GetPullRequestFiles returns all files for PRs with >30 changed files (pagination tested)
  • GetPullRequestFiles populates Patch from the GitHub response patch field
  • GetCommitStatuses returns results from both commit statuses and check-runs endpoints

Out of scope

  • Reviewer + Identity methods (issue #81)
  • CLI flag wiring (issue #82)
  • Gitea adapter (issue #79)
## Architecture Reference > This issue is self-contained. For any design questions, the canonical reference is: > [`docs/DESIGN-vcs-abstraction.md`](https://gitea.weiker.me/rodin/review-bot/src/branch/feature/github-support/docs/DESIGN-vcs-abstraction.md) > (Parts 2, 3, and 4 are most relevant to this issue) **Base branch:** `feature/github-support` — open all PRs against this branch. --- ## Goal Implement `PRReader` and `FileReader` for the GitHub client, with a configurable base URL to support both `github.com` and GitHub Enterprise (`github.concur.com`). ## Background review-bot needs to post reviews on GitHub and GitHub Enterprise PRs. The `vcs.Client` interface and all canonical types live in `vcs/` (created in issue #78). The GitHub client uses diff-position natively — `ReviewComment.Position` maps directly to the GitHub API `position` field with no translation needed. Auth: `Authorization: Bearer <token>` Base URL: configurable; empty string defaults to `https://api.github.com`. For GitHub Enterprise (`github.concur.com`), the base URL is `https://github.concur.com/api/v3`. ## Work ### `github/client.go` ```go func NewClient(token, baseURL string) *Client // empty baseURL defaults to https://api.github.com ``` HTTP helpers: set `Authorization: Bearer <token>`, `Accept`, `Content-Type` headers. Handle 429 with `Retry-After`. ### `github/pr.go` - `GetPullRequest` — `GET /repos/{owner}/{repo}/pulls/{number}` - `GetPullRequestDiff` — same URL (`GET /repos/{owner}/{repo}/pulls/{number}`) with `Accept: application/vnd.github.diff` set **per-request** (overrides the default `application/vnd.github+json`). The response body is raw unified diff text — do not JSON-unmarshal it. - `GetPullRequestFiles` — `GET /repos/{owner}/{repo}/pulls/{number}/files`. Paginate through all pages (default 30/page, use `?per_page=100&page=N`). For each file object, populate `vcs.ChangedFile{Filename, Status, Patch}` where `Patch` is the `patch` field from the response (the per-file unified diff hunk). `Patch` may be absent for binary files — set to `""` in that case. - `GetFileContentAtRef` — `GET /repos/{owner}/{repo}/contents/{path}?ref={ref}`, base64-decode the `content` field. An empty `ref` omits the query parameter (uses default branch). - `GetCommitStatuses` — fetches both commit statuses and check runs, merges them into `[]vcs.CommitStatus`: **Commit statuses:** `GET /repos/{owner}/{repo}/commits/{sha}/status` returns a JSON envelope `{state, statuses: [...]}` — unmarshal the wrapper object and extract the `statuses` array. Map each entry: `Context→context`, `Status→state`, `Description→description`, `TargetURL→target_url`. **Check runs:** `GET /repos/{owner}/{repo}/commits/{sha}/check-runs` (paginated). For each check run, map to `vcs.CommitStatus{Context: name, Status: <mapped>, Description: conclusion, TargetURL: html_url}`. Map check run `conclusion` to `vcs.CommitStatus.Status` as follows: - `"success"` → `"success"` - `"failure"`, `"action_required"`, `"timed_out"` → `"failure"` - `"cancelled"`, `"skipped"`, `"neutral"` → `"success"` (non-blocking) - `nil` / `"in_progress"` / `"queued"` → `"pending"` Return the merged slice. Deduplication is not required (different context names). ### `github/files.go` - `GetFileContent` — delegates to `GetFileContentAtRef(ctx, owner, repo, path, "")` (empty ref = default branch) - `ListContents` — `GET /repos/{owner}/{repo}/contents/{path}` (no ref), returns array when path is a directory ### Unit tests For each method: happy path, 404, 401, 429 (with retry), malformed response. ## Compile-time check ```go var _ vcs.PRReader = (*Client)(nil) var _ vcs.FileReader = (*Client)(nil) ``` (Full `vcs.Client` check happens after #81 adds Reviewer + Identity.) ## Exit criteria - `go test ./github/...` passes for all PRReader + FileReader methods - `NewClient(token, "")` uses `https://api.github.com` - `NewClient(token, "https://github.concur.com/api/v3")` targets GHE correctly - `GetFileContent` delegates to `GetFileContentAtRef` with empty ref - `GetPullRequestFiles` returns all files for PRs with >30 changed files (pagination tested) - `GetPullRequestFiles` populates `Patch` from the GitHub response `patch` field - `GetCommitStatuses` returns results from both commit statuses and check-runs endpoints ## Out of scope - Reviewer + Identity methods (issue #81) - CLI flag wiring (issue #82) - Gitea adapter (issue #79)
rodin commented 2026-05-12 17:28:06 +00:00
Author
Owner
Copy Link
Copy Source

Update from codebase audit (issue #82): Two additional methods needed in the GitHub client:

  1. GetFileContentAtRef(ctx, owner, repo, path, ref string) (string, error) — same as GetFileContent but with an explicit ref. GitHub: GET /repos/{owner}/{repo}/contents/{path}?ref={ref} + base64 decode (same endpoint as GetFileContent, just with the ref param populated).

  2. GetCommitStatuses(ctx, owner, repo, sha string) ([]CommitStatus, error) — GitHub: GET /repos/{owner}/{repo}/commits/{sha}/status. Map the statuses array to []vcs.CommitStatus.

Add unit tests for both.

**Update from codebase audit (issue #82):** Two additional methods needed in the GitHub client: 1. `GetFileContentAtRef(ctx, owner, repo, path, ref string) (string, error)` — same as `GetFileContent` but with an explicit ref. GitHub: `GET /repos/{owner}/{repo}/contents/{path}?ref={ref}` + base64 decode (same endpoint as `GetFileContent`, just with the ref param populated). 2. `GetCommitStatuses(ctx, owner, repo, sha string) ([]CommitStatus, error)` — GitHub: `GET /repos/{owner}/{repo}/commits/{sha}/status`. Map the `statuses` array to `[]vcs.CommitStatus`. Add unit tests for both.
rodin self-assigned this 2026-05-12 22:10:20 +00:00
rodin commented 2026-05-13 04:13:09 +00:00
Author
Owner
Copy Link
Copy Source

Split into sub-issues for focused review:

Closing in favour of the sub-issues.

Split into sub-issues for focused review: - #98 — Client Foundation (`vcs/types.go`, `github/client.go`, `github/client_test.go`) → PR #101 - #99 — PRReader interface (`github/pr.go`, `github/pr_test.go`) → PR #102 - #100 — FileReader interface (`github/files.go`, `github/files_test.go`, `github/conformance_test.go`) → PR #103 Closing in favour of the sub-issues.
rodin closed this issue 2026-05-13 04:13:12 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees rodin
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#80
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?