From a641395f6bfb0cab6d83df7678132eecfa112dcc Mon Sep 17 00:00:00 2001 From: Rodin Date: Mon, 11 May 2026 10:11:13 -0700 Subject: [PATCH] =?UTF-8?q?docs:=20flip=20design=20=E2=80=94=20extract=20i?= =?UTF-8?q?nterfaces=20from=20working=20gitea/=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/DESIGN-vcs-abstraction.md | 291 ++++++++++++++------------------- 1 file changed, 120 insertions(+), 171 deletions(-) diff --git a/docs/DESIGN-vcs-abstraction.md b/docs/DESIGN-vcs-abstraction.md index e1c2443..85f6b4b 100644 --- a/docs/DESIGN-vcs-abstraction.md +++ b/docs/DESIGN-vcs-abstraction.md @@ -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.