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

docs: flip design — extract interfaces from working gitea/ code

Browse Source 
Key changes:
- Interface discovered from gitea/, not invented
- Gitea adapter first (Phase 1-2), GitHub second (Phase 3-5)
- Removed 'Open Questions' — all resolved:
  - Token: workflow GITHUB_TOKEN
  - Binary: GitHub releases on aweiker/ai-core-review-bot
  - Comment schema: adapter responsibility
- 8 phases with clear exit criteria
- Platform-specific features (resolve, timeline) stay on concrete client

Issue: #76
This commit is contained in:
Rodin
2026-05-11 10:11:13 -07:00
parent 2b611dbd0b
commit 3fc31c0822
1 changed files with 120 additions and 171 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/DESIGN-vcs-abstraction.md
+120 -171
View File
@@ -6,15 +6,14 @@ AI code reviews on GitHub PRs using SAP AI Core as the LLM provider.
## 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
- Auto-detection of platform (explicit `--provider` flag is fine)
- Unifying into one abstraction layer for its own sake
## Constraints
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
3. **Interface from working code** — extract from gitea/, don't invent in vacuum
---
@@ -24,94 +23,81 @@ What does review-bot actually do?
### Core Review Flow
| 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 |
| Feature | Description |
|---------|-------------|
| Get PR metadata | Title, body, head SHA, base ref |
| Get PR diff | Unified diff format |
| Get PR files | List of changed files with status |
| Get file content | Raw file at ref |
| List directory | Enumerate files in path |
| Post review | Body + inline comments + verdict |
### Review Management
| 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 |
| Feature | Description |
|---------|-------------|
| List reviews | Get existing reviews on PR |
| Delete review | Remove old review before re-posting |
| Get authenticated user | Who am I? |
### Optional/Extended
### Platform-Specific (not in shared interface)
| 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 |
| Feature | Gitea | GitHub |
|---------|-------|--------|
| Resolve comment | Yes | No equivalent |
| Timeline API | Yes | No equivalent |
These stay on gitea.Client directly. Callers that need them type-assert.
---
## 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 |
| Feature | Gitea API | GitHub API |
|---------|-----------|------------|
| Get PR | `GET /api/v1/repos/.../pulls/{n}` | `GET /repos/.../pulls/{n}` |
| Get diff | `.diff` suffix | `Accept: application/vnd.github.diff` header |
| Get files | `GET .../pulls/{n}/files` | Same |
| Get file content | `GET .../raw/{path}?ref=` | `GET .../contents/{path}?ref=` + base64 decode |
| List directory | `GET .../contents/{path}` | Same |
| Post review | `POST .../pulls/{n}/reviews` | Same (adapter handles comment schema) |
| List reviews | `GET .../pulls/{n}/reviews` | Same |
| Delete review | `DELETE .../pulls/{n}/reviews/{id}` | Same |
| Get user | `GET /api/v1/user` | `GET /user` |
---
## Part 3: Interface Design
Small, role-based interfaces. Test what you use.
**Principle:** Extract from working gitea/ code. The interface is discovered, not invented.
### Small, role-based interfaces
```go
// pr.go — PR operations
// vcs/interfaces.go
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)
}
// 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)
}
// 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)
}
```
### Composite
```go
// Client combines all capabilities for callers that need everything
// Client combines all for callers that need everything
type Client interface {
PRReader
FileReader
@@ -120,57 +106,33 @@ type Client interface {
}
```
### Types (shared)
### Types
Use what gitea/ already has. Move to vcs/types.go or re-export.
```go
type PullRequest struct {
Number int
Title string
Body string
HeadSHA string
HeadRef string
BaseRef string
}
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
}
type PullRequest struct { ... } // from gitea.PullRequest
type ChangedFile struct { ... } // from gitea.ChangedFile
type ContentEntry struct { ... } // from gitea.ContentEntry
type Review struct { ... } // from gitea.Review
type ReviewRequest struct { ... } // new, for PostReview input
type ReviewComment struct { ... } // from gitea.ReviewComment
```
### Adapter responsibilities
Each adapter (gitea, github) handles:
- API URL construction
- Auth header format (`token` vs `Bearer`)
- Request/response mapping
- Comment schema translation (line numbers, commit IDs, etc.)
---
## 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
@@ -179,141 +141,128 @@ github/
identity_test.go # TestGetAuthenticatedUser
```
Test cases per method:
- Happy path
- 404 not found
- 401 unauthorized
- Rate limited (429)
- Malformed response
Per method: happy path, 404, 401, 429, malformed response.
### Integration Tests (real GitHub)
### Integration Tests
Against github.com/aweiker/ai-core-review-bot:
- Fetch real PR
- Fetch real file
- Post + delete review (clean up)
```
integration/
github_test.go # +build integration
```
### End-to-End
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.
Open PR on test repo, run full review-bot, verify review appears.
---
## Part 5: Implementation Phases
### Phase 1: Types + Interfaces
### Phase 1: Extract interfaces from gitea/
**Files:**
- `vcs/types.go` — shared types
- `vcs/interfaces.go` — PRReader, FileReader, Reviewer, Identity, Client
**Work:**
- Create `vcs/interfaces.go` with interfaces extracted from gitea/client.go signatures
- Create `vcs/types.go` — move or alias types from gitea/
- Verify gitea.Client satisfies vcs.Client (compile-time check)
**Exit criteria:** Compiles. No implementation yet.
**PR:** #1 on feature branch
**Exit criteria:** `var _ vcs.Client = (*gitea.Client)(nil)` compiles.
---
### Phase 2: GitHub Client — PRReader
### Phase 2: Gitea adapter (if needed)
**Files:**
**Work:**
- If gitea.Client method signatures don't match exactly, create wrapper
- Keep gitea/ working exactly as before
**Exit criteria:** Existing tests pass. No behavior change.
---
### Phase 3: GitHub client — PRReader
**Work:**
- `github/client.go` — struct, constructor, HTTP helpers
- `github/pr.go` — GetPullRequest, GetPullRequestDiff, GetPullRequestFiles
- `github/pr_test.go` — unit tests
- Unit tests
**Exit criteria:** `go test ./github/...` passes for PR methods.
**PR:** #2
---
### Phase 3: GitHub Client — FileReader
### Phase 4: GitHub client — FileReader
**Files:**
**Work:**
- `github/files.go` — GetFileContent, ListContents
- `github/files_test.go`
- Unit tests
**Exit criteria:** Unit tests pass.
**PR:** #3
---
### Phase 4: GitHub Client — Reviewer + Identity
### Phase 5: GitHub client — Reviewer + Identity
**Files:**
**Work:**
- `github/review.go` — PostReview, ListReviews, DeleteReview
- `github/identity.go` — GetAuthenticatedUser
- `github/review_test.go`, `github/identity_test.go`
- Unit tests
**Exit criteria:** Unit tests pass.
**PR:** #4
---
### Phase 5: Integration Tests
### Phase 6: Integration tests
**Files:**
**Work:**
- `integration/github_test.go`
- Test against real GitHub
**Exit criteria:** All integration tests pass against real GitHub.
**PR:** #5
**Exit criteria:** All integration tests pass.
---
### Phase 6: Wire into cmd/review-bot
### Phase 7: Wire into cmd/review-bot
**Files:**
- `cmd/review-bot/main.go` — add `--provider github` flag
- Update to use vcs interfaces
**Work:**
- Add `--provider github|gitea` flag (default: gitea for backward compat)
- Select client based on flag
- Update to use vcs interfaces where it makes sense
**Exit criteria:**
- `./review-bot --provider github ...` works
- Existing Gitea path unchanged
**PR:** #6
- `./review-bot --provider gitea ...` works (same as before)
- Existing Gitea workflows unchanged
---
### Phase 7: GitHub Actions Workflow
### Phase 8: GitHub Actions workflow + releases
**Files:**
- `.github/workflows/ci.yml`
- `.github/workflows/review.yml`
- `.github/actions/review/action.yml`
**Work:**
- `.github/workflows/ci.yml` — test on PR
- `.github/workflows/release.yml` — publish binary to GitHub releases
- `.github/actions/review/action.yml` — composite action
- Action downloads binary from github.com/aweiker/ai-core-review-bot releases
**Exit criteria:** CI runs on github.com/aweiker/ai-core-review-bot, review posts successfully.
**PR:** #7
**Exit criteria:**
- CI runs on github.com/aweiker/ai-core-review-bot
- Release creates downloadable binary
- Review action posts review successfully
---
## Part 6: Open Questions
## Part 6: Decisions
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?
| Question | Decision |
|----------|----------|
| Auth token | Workflow `GITHUB_TOKEN` (automatic) |
| Binary distribution | GitHub releases on aweiker/ai-core-review-bot |
| Comment schema | Adapter's job — translate ReviewComment to platform format |
| Default provider | `gitea` for backward compatibility |
| Shared types | vcs/types.go (extracted from gitea/) |
| Platform-specific features | Stay on concrete client, not interface |
---
## Summary
7 phases, each with clear exit criteria and a PR. Build from types up, test at each layer, integrate last.
8 phases. Start by extracting interfaces from working gitea/ code, not inventing them. GitHub implements the same interfaces. Each phase has clear exit criteria.