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

docs: rewrite design doc — feature-first, testable, phased

Browse Source 
- 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
This commit is contained in:
Rodin
2026-05-11 09:43:51 -07:00
parent 3abb611baf
commit 2b611dbd0b
1 changed files with 280 additions and 171 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/DESIGN-vcs-abstraction.md
+280 -171
View File
@@ -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.