rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 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
committed by claw
parent f944f1cd45
commit a641395f6b
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 ## Non-Goals
- Auto-detection of platform (explicit is fine) - Auto-detection of platform (explicit `--provider` flag is fine)
- Single binary supporting both (two entry points OK) - Unifying into one abstraction layer for its own sake
- Unifying Gitea and GitHub into one abstraction layer
## Constraints ## Constraints
1. **Same features on both platforms** — anything review-bot does on Gitea should work on GitHub 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 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 ### Core Review Flow
| Feature | Description | Used In | | Feature | Description |
|---------|-------------|---------| |---------|-------------|
| Get PR metadata | Title, body, head SHA, base ref | Review context | | Get PR metadata | Title, body, head SHA, base ref |
| Get PR diff | Unified diff format | LLM input | | Get PR diff | Unified diff format |
| Get PR files | List of changed files with status | Filtering, context | | Get PR files | List of changed files with status |
| Get file content | Raw file at ref | Conventions, patterns | | Get file content | Raw file at ref |
| List directory | Enumerate files in path | Pattern repos | | List directory | Enumerate files in path |
| Post review | Body + inline comments + verdict | Output | | Post review | Body + inline comments + verdict |
### Review Management ### Review Management
| Feature | Description | Used In | | Feature | Description |
|---------|-------------|---------| |---------|-------------|
| List reviews | Get existing reviews on PR | Supersede check | | List reviews | Get existing reviews on PR |
| Delete review | Remove old review before re-posting | Supersede flow | | Delete review | Remove old review before re-posting |
| Get authenticated user | Who am I? | Filter own reviews | | Get authenticated user | Who am I? |
### Optional/Extended ### Platform-Specific (not in shared interface)
| Feature | Description | Used In | | Feature | Gitea | GitHub |
|---------|-------------|---------| |---------|-------|--------|
| Resolve comment | Mark inline comment resolved | Gitea-specific | | Resolve comment | Yes | No equivalent |
| Edit comment | Update existing comment | Supersede variant | | Timeline API | Yes | No equivalent |
| Request reviewer | Add reviewer to PR | Workflow integration |
| Get commit statuses | CI status | Gate logic | These stay on gitea.Client directly. Callers that need them type-assert.
--- ---
## Part 2: GitHub API Mapping ## Part 2: GitHub API Mapping
### Supported | Feature | Gitea API | GitHub API |
|---------|-----------|------------|
| Feature | Gitea API | GitHub API | Notes | | Get PR | `GET /api/v1/repos/.../pulls/{n}` | `GET /repos/.../pulls/{n}` |
|---------|-----------|------------|-------| | Get diff | `.diff` suffix | `Accept: application/vnd.github.diff` header |
| Get PR | `GET /api/v1/repos/{owner}/{repo}/pulls/{n}` | `GET /repos/{owner}/{repo}/pulls/{n}` | Same structure | | Get files | `GET .../pulls/{n}/files` | Same |
| Get diff | `GET .../pulls/{n}.diff` | `GET .../pulls/{n}` + `Accept: application/vnd.github.diff` | Different mechanism | | Get file content | `GET .../raw/{path}?ref=` | `GET .../contents/{path}?ref=` + base64 decode |
| Get files | `GET .../pulls/{n}/files` | `GET .../pulls/{n}/files` | Same | | List directory | `GET .../contents/{path}` | Same |
| Get file content | `GET .../raw/{path}?ref=` | `GET .../contents/{path}?ref=` + base64 decode | Different format | | Post review | `POST .../pulls/{n}/reviews` | Same (adapter handles comment schema) |
| List directory | `GET .../contents/{path}` | `GET .../contents/{path}` | Same | | List reviews | `GET .../pulls/{n}/reviews` | Same |
| Post review | `POST .../pulls/{n}/reviews` | `POST .../pulls/{n}/reviews` | Minor field differences | | Delete review | `DELETE .../pulls/{n}/reviews/{id}` | Same |
| List reviews | `GET .../pulls/{n}/reviews` | `GET .../pulls/{n}/reviews` | Same | | Get user | `GET /api/v1/user` | `GET /user` |
| 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 ## 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 ```go
// pr.go — PR operations // vcs/interfaces.go
type PRReader interface { type PRReader interface {
GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error)
GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error)
} }
// files.go — File/content operations
type FileReader interface { type FileReader interface {
GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error)
ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
} }
// review.go — Review operations
type Reviewer interface { type Reviewer interface {
PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error) PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error)
ListReviews(ctx context.Context, owner, repo string, number int) ([]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 DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
} }
// identity.go — Auth/identity
type Identity interface { type Identity interface {
GetAuthenticatedUser(ctx context.Context) (string, error) GetAuthenticatedUser(ctx context.Context) (string, error)
} }
```
### Composite // Client combines all for callers that need everything
```go
// Client combines all capabilities for callers that need everything
type Client interface { type Client interface {
PRReader PRReader
FileReader 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 ```go
type PullRequest struct { type PullRequest struct { ... } // from gitea.PullRequest
Number int type ChangedFile struct { ... } // from gitea.ChangedFile
Title string type ContentEntry struct { ... } // from gitea.ContentEntry
Body string type Review struct { ... } // from gitea.Review
HeadSHA string type ReviewRequest struct { ... } // new, for PostReview input
HeadRef string type ReviewComment struct { ... } // from gitea.ReviewComment
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
}
``` ```
### 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 ## Part 4: Test Plan
### Unit Tests (mock HTTP) ### Unit Tests (mock HTTP)
Each interface method gets a test against a mock HTTP server.
``` ```
github/ github/
pr_test.go # TestGetPullRequest, TestGetDiff, TestGetFiles pr_test.go # TestGetPullRequest, TestGetDiff, TestGetFiles
@@ -179,141 +141,128 @@ github/
identity_test.go # TestGetAuthenticatedUser identity_test.go # TestGetAuthenticatedUser
``` ```
Test cases per method: Per method: happy path, 404, 401, 429, malformed response.
- Happy path
- 404 not found
- 401 unauthorized
- Rate limited (429)
- Malformed response
### Integration Tests (real GitHub) ### Integration Tests
Against github.com/aweiker/ai-core-review-bot: Against github.com/aweiker/ai-core-review-bot:
- Fetch real PR
- Fetch real file
- Post + delete review (clean up)
``` ### End-to-End
integration/
github_test.go # +build integration
```
Requires `GITHUB_TOKEN` env var. Run manually or in CI with secrets. Open PR on test repo, run full review-bot, verify review appears.
| 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 ## Part 5: Implementation Phases
### Phase 1: Types + Interfaces ### Phase 1: Extract interfaces from gitea/
**Files:** **Work:**
- `vcs/types.go` — shared types - Create `vcs/interfaces.go` with interfaces extracted from gitea/client.go signatures
- `vcs/interfaces.go` — PRReader, FileReader, Reviewer, Identity, Client - 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. **Exit criteria:** `var _ vcs.Client = (*gitea.Client)(nil)` compiles.
**PR:** #1 on feature branch
--- ---
### 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/client.go` — struct, constructor, HTTP helpers
- `github/pr.go` — GetPullRequest, GetPullRequestDiff, GetPullRequestFiles - `github/pr.go` — GetPullRequest, GetPullRequestDiff, GetPullRequestFiles
- `github/pr_test.go` — unit tests - Unit tests
**Exit criteria:** `go test ./github/...` passes for PR methods. **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.go` — GetFileContent, ListContents
- `github/files_test.go` - Unit tests
**Exit criteria:** Unit tests pass. **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/review.go` — PostReview, ListReviews, DeleteReview
- `github/identity.go` — GetAuthenticatedUser - `github/identity.go` — GetAuthenticatedUser
- `github/review_test.go`, `github/identity_test.go` - Unit tests
**Exit criteria:** Unit tests pass. **Exit criteria:** Unit tests pass.
**PR:** #4
--- ---
### Phase 5: Integration Tests ### Phase 6: Integration tests
**Files:** **Work:**
- `integration/github_test.go` - `integration/github_test.go`
- Test against real GitHub
**Exit criteria:** All integration tests pass against real GitHub. **Exit criteria:** All integration tests pass.
**PR:** #5
--- ---
### Phase 6: Wire into cmd/review-bot ### Phase 7: Wire into cmd/review-bot
**Files:** **Work:**
- `cmd/review-bot/main.go` — add `--provider github` flag - Add `--provider github|gitea` flag (default: gitea for backward compat)
- Update to use vcs interfaces - Select client based on flag
- Update to use vcs interfaces where it makes sense
**Exit criteria:** **Exit criteria:**
- `./review-bot --provider github ...` works - `./review-bot --provider github ...` works
- Existing Gitea path unchanged - `./review-bot --provider gitea ...` works (same as before)
- Existing Gitea workflows unchanged
**PR:** #6
--- ---
### Phase 7: GitHub Actions Workflow ### Phase 8: GitHub Actions workflow + releases
**Files:** **Work:**
- `.github/workflows/ci.yml` - `.github/workflows/ci.yml` — test on PR
- `.github/workflows/review.yml` - `.github/workflows/release.yml` — publish binary to GitHub releases
- `.github/actions/review/action.yml` - `.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. **Exit criteria:**
- CI runs on github.com/aweiker/ai-core-review-bot
**PR:** #7 - 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. | Question | Decision |
|----------|----------|
2. **Rate limiting** — GitHub has stricter limits. Add retry with backoff? (Already have this for Gitea.) | Auth token | Workflow `GITHUB_TOKEN` (automatic) |
| Binary distribution | GitHub releases on aweiker/ai-core-review-bot |
3. **Pagination** — GitHub paginates at 100. Do we need >100 files/reviews? Probably not for MVP. | Comment schema | Adapter's job — translate ReviewComment to platform format |
| Default provider | `gitea` for backward compatibility |
4. **Comment threading** — GitHub review comments work differently. Test and document. | Shared types | vcs/types.go (extracted from gitea/) |
| Platform-specific features | Stay on concrete client, not interface |
5. **Draft reviews** — Use for integration tests to avoid noise?
--- ---
## Summary ## 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.