From f944f1cd4583da6fc386a3306e41c09901053430 Mon Sep 17 00:00:00 2001 From: Rodin Date: Mon, 11 May 2026 09:43:51 -0700 Subject: [PATCH] =?UTF-8?q?docs:=20rewrite=20design=20doc=20=E2=80=94=20fe?= =?UTF-8?q?ature-first,=20testable,=20phased?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Goal: AI code reviews on GitHub with AI Core - Feature inventory with API mapping - Small interfaces (PRReader, FileReader, Reviewer, Identity) - Test plan: unit (mock HTTP) + integration (real GitHub) - 7 implementation phases with exit criteria Issue: #76 --- docs/DESIGN-vcs-abstraction.md | 451 ++++++++++++++++++++------------- 1 file changed, 280 insertions(+), 171 deletions(-) diff --git a/docs/DESIGN-vcs-abstraction.md b/docs/DESIGN-vcs-abstraction.md index b280ba5..e1c2443 100644 --- a/docs/DESIGN-vcs-abstraction.md +++ b/docs/DESIGN-vcs-abstraction.md @@ -1,210 +1,319 @@ -# VCS Abstraction Layer Design +# GitHub Support for review-bot -Issue: #76 +## Goal -## Problem +AI code reviews on GitHub PRs using SAP AI Core as the LLM provider. -review-bot currently only works with Gitea. We want it to work on GitHub (github.com and GHE) without maintaining two forks. +## Non-Goals + +- Auto-detection of platform (explicit is fine) +- Single binary supporting both (two entry points OK) +- Unifying Gitea and GitHub into one abstraction layer ## 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 +1. **Same features on both platforms** — anything review-bot does on Gitea should work on GitHub +2. **Testable** — small interfaces, dependency injection, no global state +3. **Verified** — the strat/review-bot github/ code was never tested; we build from scratch with tests -## Current State +--- -Two packages exist: -- `gitea/` in rodin/review-bot — Gitea API -- `github/` in strat/review-bot — GitHub API (forked from gitea/, adjusted paths) +## Part 1: Feature Inventory -Method signatures are identical. Types are identical. Only API endpoints differ. +What does review-bot actually do? -## Proposed Approach +### Core Review Flow -### Phase 1: Port github/ package +| Feature | Description | Used In | +|---------|-------------|---------| +| Get PR metadata | Title, body, head SHA, base ref | Review context | +| Get PR diff | Unified diff format | LLM input | +| Get PR files | List of changed files with status | Filtering, context | +| Get file content | Raw file at ref | Conventions, patterns | +| List directory | Enumerate files in path | Pattern repos | +| Post review | Body + inline comments + verdict | Output | -Copy strat/review-bot's `github/` package into rodin/review-bot. +### Review Management -Files: -- `github/client.go` -- `github/client_test.go` -- `github/diff.go` +| Feature | Description | Used In | +|---------|-------------|---------| +| List reviews | Get existing reviews on PR | Supersede check | +| Delete review | Remove old review before re-posting | Supersede flow | +| Get authenticated user | Who am I? | Filter own reviews | -No changes to gitea/ package. +### Optional/Extended -### Phase 2: Add vcs/ abstraction +| Feature | Description | Used In | +|---------|-------------|---------| +| Resolve comment | Mark inline comment resolved | Gitea-specific | +| Edit comment | Update existing comment | Supersede variant | +| Request reviewer | Add reviewer to PR | Workflow integration | +| Get commit statuses | CI status | Gate logic | -``` -vcs/ - interface.go # Client interface - client.go # NewClient() with detection - doc.go # package doc -``` +--- + +## Part 2: GitHub API Mapping + +### Supported + +| Feature | Gitea API | GitHub API | Notes | +|---------|-----------|------------|-------| +| Get PR | `GET /api/v1/repos/{owner}/{repo}/pulls/{n}` | `GET /repos/{owner}/{repo}/pulls/{n}` | Same structure | +| Get diff | `GET .../pulls/{n}.diff` | `GET .../pulls/{n}` + `Accept: application/vnd.github.diff` | Different mechanism | +| Get files | `GET .../pulls/{n}/files` | `GET .../pulls/{n}/files` | Same | +| Get file content | `GET .../raw/{path}?ref=` | `GET .../contents/{path}?ref=` + base64 decode | Different format | +| List directory | `GET .../contents/{path}` | `GET .../contents/{path}` | Same | +| Post review | `POST .../pulls/{n}/reviews` | `POST .../pulls/{n}/reviews` | Minor field differences | +| List reviews | `GET .../pulls/{n}/reviews` | `GET .../pulls/{n}/reviews` | Same | +| Delete review | `DELETE .../pulls/{n}/reviews/{id}` | `DELETE .../pulls/{n}/reviews/{id}` | Same | +| Get user | `GET /api/v1/user` | `GET /user` | Same | + +### Gaps + +| Feature | Gitea | GitHub | Workaround | +|---------|-------|--------|------------| +| Resolve comment | `POST .../pulls/comments/{id}/resolve` | No equivalent | Skip (document limitation) | +| Timeline API | `GET .../issues/{n}/timeline` | No equivalent | Use events API or skip | + +--- + +## Part 3: Interface Design + +Small, role-based interfaces. Test what you use. -**interface.go:** ```go -package vcs - -import "context" - -// Client abstracts VCS operations. Implemented by gitea.Client and github.Client. -type Client interface { +// pr.go — PR operations +type PRReader 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) +} + +// files.go — File/content operations +type FileReader interface { + GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) - GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) +} + +// review.go — Review operations +type Reviewer interface { + PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error +} + +// identity.go — Auth/identity +type Identity interface { 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:** +### Composite + ```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 +// Client combines all capabilities for callers that need everything +type Client interface { + PRReader + FileReader + Reviewer + Identity } ``` -### Phase 3: Update cmd/review-bot +### Types (shared) -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) +type PullRequest struct { + Number int + Title string + Body string + HeadSHA string + HeadRef string + BaseRef string } -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" +type ChangedFile struct { + Filename string + Status string // added, modified, removed, renamed +} + +type ContentEntry struct { + Name string + Path string + Type string // file, dir +} + +type ReviewRequest struct { + Event string // APPROVE, REQUEST_CHANGES, COMMENT + Body string + Comments []ReviewComment +} + +type ReviewComment struct { + Path string + Line int + Body string +} + +type Review struct { + ID int64 + User string + State string + Body string } ``` -This keeps the github/ package unchanged (it already handles the API URL format). +--- + +## Part 4: Test Plan + +### Unit Tests (mock HTTP) + +Each interface method gets a test against a mock HTTP server. + +``` +github/ + pr_test.go # TestGetPullRequest, TestGetDiff, TestGetFiles + files_test.go # TestGetFileContent, TestListContents + review_test.go # TestPostReview, TestListReviews, TestDeleteReview + identity_test.go # TestGetAuthenticatedUser +``` + +Test cases per method: +- Happy path +- 404 not found +- 401 unauthorized +- Rate limited (429) +- Malformed response + +### Integration Tests (real GitHub) + +Against github.com/aweiker/ai-core-review-bot: + +``` +integration/ + github_test.go # +build integration +``` + +Requires `GITHUB_TOKEN` env var. Run manually or in CI with secrets. + +| Test | Verifies | +|------|----------| +| Fetch PR #1 | PRReader works | +| Fetch README.md | FileReader works | +| Post + delete draft review | Reviewer works (clean up after) | +| Get authenticated user | Identity works | + +### End-to-End Test + +Open a real PR on the test repo, run review-bot against it, verify review appears. + +--- + +## Part 5: Implementation Phases + +### Phase 1: Types + Interfaces + +**Files:** +- `vcs/types.go` — shared types +- `vcs/interfaces.go` — PRReader, FileReader, Reviewer, Identity, Client + +**Exit criteria:** Compiles. No implementation yet. + +**PR:** #1 on feature branch + +--- + +### Phase 2: GitHub Client — PRReader + +**Files:** +- `github/client.go` — struct, constructor, HTTP helpers +- `github/pr.go` — GetPullRequest, GetPullRequestDiff, GetPullRequestFiles +- `github/pr_test.go` — unit tests + +**Exit criteria:** `go test ./github/...` passes for PR methods. + +**PR:** #2 + +--- + +### Phase 3: GitHub Client — FileReader + +**Files:** +- `github/files.go` — GetFileContent, ListContents +- `github/files_test.go` + +**Exit criteria:** Unit tests pass. + +**PR:** #3 + +--- + +### Phase 4: GitHub Client — Reviewer + Identity + +**Files:** +- `github/review.go` — PostReview, ListReviews, DeleteReview +- `github/identity.go` — GetAuthenticatedUser +- `github/review_test.go`, `github/identity_test.go` + +**Exit criteria:** Unit tests pass. + +**PR:** #4 + +--- + +### Phase 5: Integration Tests + +**Files:** +- `integration/github_test.go` + +**Exit criteria:** All integration tests pass against real GitHub. + +**PR:** #5 + +--- + +### Phase 6: Wire into cmd/review-bot + +**Files:** +- `cmd/review-bot/main.go` — add `--provider github` flag +- Update to use vcs interfaces + +**Exit criteria:** +- `./review-bot --provider github ...` works +- Existing Gitea path unchanged + +**PR:** #6 + +--- + +### Phase 7: GitHub Actions Workflow + +**Files:** +- `.github/workflows/ci.yml` +- `.github/workflows/review.yml` +- `.github/actions/review/action.yml` + +**Exit criteria:** CI runs on github.com/aweiker/ai-core-review-bot, review posts successfully. + +**PR:** #7 + +--- + +## Part 6: Open Questions + +1. **Auth token format** — GitHub PAT vs fine-grained token vs GitHub App? Start with PAT. + +2. **Rate limiting** — GitHub has stricter limits. Add retry with backoff? (Already have this for Gitea.) + +3. **Pagination** — GitHub paginates at 100. Do we need >100 files/reviews? Probably not for MVP. + +4. **Comment threading** — GitHub review comments work differently. Test and document. + +5. **Draft reviews** — Use for integration tests to avoid noise? + +--- + +## Summary + +7 phases, each with clear exit criteria and a PR. Build from types up, test at each layer, integrate last.