From 45d94e7c5312c5d70d05a71edb3128c8f358b34f Mon Sep 17 00:00:00 2001 From: Rodin Date: Mon, 11 May 2026 09:30:43 -0700 Subject: [PATCH] docs: add VCS abstraction design doc Outlines phased approach for GitHub support: - Phase 1: Port github/ package from strat fork - Phase 2: Add vcs/ interface with runtime detection - Phase 3: Wire up cmd/review-bot Issue: #76 --- docs/DESIGN-vcs-abstraction.md | 210 +++++++++++++++++++++++++++++++++ 1 file changed, 210 insertions(+) create mode 100644 docs/DESIGN-vcs-abstraction.md diff --git a/docs/DESIGN-vcs-abstraction.md b/docs/DESIGN-vcs-abstraction.md new file mode 100644 index 0000000..b280ba5 --- /dev/null +++ b/docs/DESIGN-vcs-abstraction.md @@ -0,0 +1,210 @@ +# VCS Abstraction Layer Design + +Issue: #76 + +## Problem + +review-bot currently only works with Gitea. We want it to work on GitHub (github.com and GHE) without maintaining two forks. + +## Constraints + +- Must be backward compatible — existing Gitea usage unchanged +- Runtime detection — no config flag needed +- Same types — gitea/ and github/ packages already use identical structs +- Minimal diff — don't rewrite, just abstract + +## Current State + +Two packages exist: +- `gitea/` in rodin/review-bot — Gitea API +- `github/` in strat/review-bot — GitHub API (forked from gitea/, adjusted paths) + +Method signatures are identical. Types are identical. Only API endpoints differ. + +## Proposed Approach + +### Phase 1: Port github/ package + +Copy strat/review-bot's `github/` package into rodin/review-bot. + +Files: +- `github/client.go` +- `github/client_test.go` +- `github/diff.go` + +No changes to gitea/ package. + +### Phase 2: Add vcs/ abstraction + +``` +vcs/ + interface.go # Client interface + client.go # NewClient() with detection + doc.go # package doc +``` + +**interface.go:** +```go +package vcs + +import "context" + +// Client abstracts VCS operations. Implemented by gitea.Client and github.Client. +type Client interface { + GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) + GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) + GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) + GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) + GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) + GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) + PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) + ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) + GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) + ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) + DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error + GetAuthenticatedUser(ctx context.Context) (string, error) +} + +// Types re-exported from gitea package (canonical source) +type ( + PullRequest = gitea.PullRequest + ChangedFile = gitea.ChangedFile + CommitStatus = gitea.CommitStatus + ReviewComment = gitea.ReviewComment + ContentEntry = gitea.ContentEntry + Review = gitea.Review +) +``` + +**client.go:** +```go +package vcs + +import ( + "strings" + + "gitea.weiker.me/rodin/review-bot/gitea" + "gitea.weiker.me/rodin/review-bot/github" +) + +// NewClient creates a VCS client based on the server URL. +// GitHub detection: github.com, api.github.com, or "github" in domain (GHE). +func NewClient(baseURL, token string) Client { + if isGitHub(baseURL) { + return github.NewClient(baseURL, token) + } + return gitea.NewClient(baseURL, token) +} + +func isGitHub(baseURL string) bool { + lower := strings.ToLower(baseURL) + if strings.Contains(lower, "github.com") { + return true + } + // GHE: github.mycompany.com but NOT gitea.mycompany.com + if strings.Contains(lower, "github") && !strings.Contains(lower, "gitea") { + return true + } + return false +} +``` + +### Phase 3: Update cmd/review-bot + +Change from: +```go +client := gitea.NewClient(baseURL, token) +``` + +To: +```go +client := vcs.NewClient(baseURL, token) +``` + +All callers already use the interface implicitly (same method names). This is a one-line change. + +## API Differences Handled in github/ + +| Operation | Gitea | GitHub | +|-----------|-------|--------| +| API prefix | `/api/v1/repos/` | `/repos/` (github.com uses api.github.com) | +| Get diff | `GET /pulls/{n}.diff` | `GET /pulls/{n}` + `Accept: application/vnd.github.diff` | +| Auth header | `Authorization: token X` | `Authorization: Bearer X` | + +The github/ package already handles these. No new code needed. + +## Methods NOT in interface (Gitea-only) + +These methods exist in gitea/ but aren't needed for core review flow: +- `GetTimelineReviewCommentID` — Gitea-specific timeline API +- `GetTimelineReviewCommentIDForReview` — Gitea-specific +- `EditComment` — supersede flow (Gitea-specific) +- `ResolveComment` — Gitea-specific + +If a caller needs these, they can type-assert: `if gc, ok := client.(*gitea.Client); ok { ... }` + +## Testing Strategy + +1. Unit tests for `isGitHub()` detection +2. Existing gitea/ tests unchanged +3. Existing github/ tests from strat fork +4. Integration test: mock server, verify correct client selected + +## Phases + +| PR | Content | Exit Criteria | +|----|---------|---------------| +| 1 | Port github/ package | `go test ./github/...` passes | +| 2 | Add vcs/ interface | `go build ./...` compiles | +| 3 | Update cmd/review-bot | All existing tests pass, GitHub detection works | + +## Open Questions + +1. **Type re-export vs duplicate** — Using type aliases (`type PullRequest = gitea.PullRequest`) means callers import vcs and get gitea types. Alternative: duplicate types in vcs/. Preference? + +2. **github.com API base URL** — For github.com, API is at `api.github.com`, not `github.com/api`. The github/ package needs to handle this. Does strat's version already? + +3. **GHE API path** — GHE uses `/api/v3/repos/`. Verify strat's github/ package handles this. + +## Answers to Open Questions + +### Q1: Type re-export vs duplicate + +**Answer: Type aliases.** +- Keeps one source of truth (gitea/ package) +- If types ever diverge, we'll know at compile time +- Callers importing `vcs` get clean namespace + +### Q2 & Q3: API URL handling + +**Answer: NewClient needs URL transformation.** + +The github/ package expects the API root: +- github.com → `https://api.github.com` +- GHE → `https://github.concur.com/api/v3` + +But callers pass server URLs (from `github.server_url` context): +- github.com → `https://github.com` +- GHE → `https://github.concur.com` + +**Updated client.go:** +```go +func NewClient(serverURL, token string) Client { + if isGitHub(serverURL) { + apiURL := toGitHubAPIURL(serverURL) + return github.NewClient(apiURL, token) + } + return gitea.NewClient(serverURL, token) +} + +func toGitHubAPIURL(serverURL string) string { + lower := strings.ToLower(serverURL) + if strings.Contains(lower, "github.com") && !strings.Contains(lower, "api.github.com") { + return "https://api.github.com" + } + // GHE: append /api/v3 + return strings.TrimRight(serverURL, "/") + "/api/v3" +} +``` + +This keeps the github/ package unchanged (it already handles the API URL format).