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

feat(vcs): extract interfaces and types from gitea/ (Phase 1) #78

New Issue
Closed
opened 2026-05-12 16:59:54 +00:00 by rodin · 3 comments
rodin commented 2026-05-12 16:59:54 +00:00
Owner
Copy Link
Copy Source

Architecture Reference

This issue is self-contained. For any design questions, the canonical reference is:
docs/DESIGN-vcs-abstraction.md
(Parts 1, 2, 3, and 6 are most relevant to this issue)

Base branch: feature/github-support — open all PRs against this branch.

Goal

Extract vcs.Client interface and shared types from the existing working gitea/ code. The interface is discovered from working code, not invented.

Background

review-bot currently only supports Gitea. The goal is to add GitHub and GitHub Enterprise support behind a shared vcs.Client interface. This issue is the foundation: define that interface by reading what gitea/client.go already does, then declare the shared types.

All ReviewComment types in vcs/ use GitHub diff-position convention (path + position + commit_id) — this is the canonical shape across all adapters. position is a 1-indexed offset from the @@ hunk line in the unified diff. Adapters translate to/from this format; the core never deals with platform-native formats.

ReviewRequest.Event uses GitHub's string values as canonical: "APPROVE", "REQUEST_CHANGES", "COMMENT". The Gitea adapter is responsible for translating these to Gitea-native event values before posting.

Work

vcs/interfaces.go

Role-based interfaces extracted from gitea/client.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)
    GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error)
    GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error)
}

type FileReader interface {
    GetFileContent(ctx context.Context, owner, repo, path string) (string, error)
    ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
}

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
    DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error
}

type Identity interface {
    GetAuthenticatedUser(ctx context.Context) (string, error)
}

type Client interface {
    PRReader
    FileReader
    Reviewer
    Identity
}

vcs/types.go

Extract or alias from gitea/. Explicit required fields listed below:

type PullRequest struct {
    // Number is not present on gitea.PullRequest today (main.go reads it from
    // the CLI flag). It is added here because the GitHub client (#80) returns
    // it in the API response and downstream code will need it to avoid
    // threading the CLI flag through every helper.
    Number int
    Title  string
    Body   string
    Head   struct {
        Ref string
        Sha string
    }
    // Base.Ref is not present on gitea.PullRequest today. Added for the
    // GitHub client (#80) which returns base branch info and may need it
    // for branch-targeting logic.
    Base struct {
        Ref string
    }
}

type ChangedFile struct {
    Filename string
    Status   string // "added", "modified", "removed"
    Patch    string // unified diff for this file; may be empty for binary files or when not returned by provider
}

type ContentEntry struct {
    Name string
    Path string
    Type string // "file" or "dir"
}

type CommitStatus struct {
    Context     string
    Status      string // "success", "failure", "pending", etc.
    Description string
    TargetURL   string
}

// Canonical Review.State values — all adapters must translate to these.
// GitHub adapter: "CHANGES_REQUESTED" → "REQUEST_CHANGES", "COMMENTED" → "COMMENT"
// Gitea adapter: already uses these values natively
const (
    ReviewStateApproved       = "APPROVED"
    ReviewStateRequestChanges = "REQUEST_CHANGES"
    ReviewStateComment        = "COMMENT"
    ReviewStateDismissed      = "DISMISSED"
)

type Review struct {
    ID       int64
    CommitID string
    Body     string
    State    string // one of the ReviewState* constants above
    User     struct {
        Login string
    }
}

type ReviewComment struct {
    Path     string
    Position int    // diff-position (GitHub convention): 1-indexed offset from @@ hunk line
    CommitID string // must be the PR head SHA; callers are responsible for setting this
    Body     string
}

// ReviewRequest.Event uses GitHub canonical values: "APPROVE", "REQUEST_CHANGES", "COMMENT"
// The Gitea adapter translates to Gitea-native event strings before posting.
type ReviewRequest struct {
    Event    string         // "APPROVE", "REQUEST_CHANGES", or "COMMENT"
    Body     string
    Comments []ReviewComment
}

vcs/util.go

Two utility functions — do not add either to the interface:

1. GetAllFilesInPath — recursive directory traversal:

// GetAllFilesInPath recursively lists all file paths under the given directory.
// Returns a map of path -> file content.
func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) (map[string]string, error)

Returns map[string]string (path → content). Implementation: call client.ListContents(ctx, owner, repo, path), recurse into Type == "dir" entries, fetch content for Type == "file" entries via client.GetFileContent.

2. BuildLineToPositionMap — inverts the diff-position map for use in main.go:

// BuildLineToPositionMap parses a unified diff and returns a per-file map
// of new-file line number → diff-position.
// Position is a 1-indexed offset from the @@ hunk line (GitHub convention).
// Only addition and context lines have new-file line numbers; deletion lines
// have a position but no new-file line, so they are not in the map.
// Returns: map[filename]map[newFileLine]diffPosition
func BuildLineToPositionMap(diff string) map[string]map[int]int

This is the inverse of the Gitea adapter's position → line map. Used in main.go to convert LLM-produced line numbers to diff-positions before constructing vcs.ReviewComment. Replaces gitea.ParseDiffNewLines + diffRanges.Contains entirely.

Counting rules are identical to the diff-position spec in issue #79:

  • The @@ hunk header is position 1 for the first hunk
  • Every line (context, addition, deletion) increments position
  • A second @@ hunk continues the count, does not reset
  • Addition and context lines map to new-file line numbers
  • Deletion lines: skip (no entry in the map)

Unit tests for BuildLineToPositionMap:

  • Single hunk with context, additions, deletions
  • Multi-hunk file (position counter continues across hunks)
  • Deletion lines not present in output map
  • Multiple files in one diff

Unit tests for GetAllFilesInPath: empty dir, flat dir, nested dirs, mixed.

Type alias strategy

When gitea/ types have identical fields to vcs/ types, prefer type aliases to avoid mapping boilerplate:

// In gitea/types.go (or wherever the type is currently defined)
type ContentEntry = vcs.ContentEntry  // alias: zero mapping needed
type CommitStatus = vcs.CommitStatus  // alias: fields are identical

Use an independent copy (no alias) only when the gitea type has fields that differ from or extend the vcs type (e.g. gitea.PullRequest has no Number field — it cannot be aliased to vcs.PullRequest). In that case, map field-by-field in the adapter.

This decision affects which methods are true pass-throughs vs. require mapping — the compile-time check will make divergences visible.

review.ContentEntry and review.GiteaClient cleanup

review/repo_persona.go defines review.ContentEntry (mirroring gitea.ContentEntry to avoid import cycles) and review.GiteaClient interface. Once vcs/types.go exists, review/ can safely import vcs/ (no cycle — vcs/ has no upward dependencies).

As part of this issue (#78), delete:

  • review.ContentEntry — replace with vcs.ContentEntry throughout review/
  • review.GiteaClient — replace with vcs.FileReader in review.LoadRepoPersonas signature
  • Update review/repo_persona_test.go: mockGiteaClient.ListContents return type changes from []review.ContentEntry to []vcs.ContentEntry

These deletions are owned by #78 and must be included in the same PR as vcs/types.go. They are not deferred — #82 depends on them already being gone.

Compile-time check

var _ vcs.Client = (*gitea.Client)(nil)

Expected to fail on PostReview, GetFileContentAtRef, GetCommitStatuses, DismissReview, ListContents, and ListReviews — this is intentional (return types differ: []gitea.ContentEntry/[]gitea.Review vs []vcs.ContentEntry/[]vcs.Review unless type-aliased). Each failure documents a gap the Gitea adapter (issue #79) must bridge. GetFileContent and GetAuthenticatedUser will pass without changes.

Exit criteria

  • vcs/interfaces.go, vcs/types.go, vcs/util.go compile
  • Compile-time check is present; failures document the gaps
  • No behavior changes to existing Gitea functionality
  • Types are extracted from gitea/, not invented
  • vcs.GetAllFilesInPath unit tests pass
  • vcs.BuildLineToPositionMap unit tests pass

Out of scope

  • Gitea adapter implementation (issue #79)
  • GitHub client (issues #80, #81)
  • CLI flag changes (issue #82)
## Architecture Reference > This issue is self-contained. For any design questions, the canonical reference is: > [`docs/DESIGN-vcs-abstraction.md`](https://gitea.weiker.me/rodin/review-bot/src/branch/feature/github-support/docs/DESIGN-vcs-abstraction.md) > (Parts 1, 2, 3, and 6 are most relevant to this issue) **Base branch:** `feature/github-support` — open all PRs against this branch. --- ## Goal Extract `vcs.Client` interface and shared types from the existing working `gitea/` code. The interface is discovered from working code, not invented. ## Background review-bot currently only supports Gitea. The goal is to add GitHub and GitHub Enterprise support behind a shared `vcs.Client` interface. This issue is the foundation: define that interface by reading what `gitea/client.go` already does, then declare the shared types. All `ReviewComment` types in `vcs/` use **GitHub diff-position convention** (`path` + `position` + `commit_id`) — this is the canonical shape across all adapters. `position` is a 1-indexed offset from the `@@` hunk line in the unified diff. Adapters translate to/from this format; the core never deals with platform-native formats. `ReviewRequest.Event` uses **GitHub's string values** as canonical: `"APPROVE"`, `"REQUEST_CHANGES"`, `"COMMENT"`. The Gitea adapter is responsible for translating these to Gitea-native event values before posting. ## Work ### `vcs/interfaces.go` Role-based interfaces extracted from `gitea/client.go`: ```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) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) } type FileReader interface { GetFileContent(ctx context.Context, owner, repo, path string) (string, error) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) } 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 DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error } type Identity interface { GetAuthenticatedUser(ctx context.Context) (string, error) } type Client interface { PRReader FileReader Reviewer Identity } ``` ### `vcs/types.go` Extract or alias from `gitea/`. Explicit required fields listed below: ```go type PullRequest struct { // Number is not present on gitea.PullRequest today (main.go reads it from // the CLI flag). It is added here because the GitHub client (#80) returns // it in the API response and downstream code will need it to avoid // threading the CLI flag through every helper. Number int Title string Body string Head struct { Ref string Sha string } // Base.Ref is not present on gitea.PullRequest today. Added for the // GitHub client (#80) which returns base branch info and may need it // for branch-targeting logic. Base struct { Ref string } } type ChangedFile struct { Filename string Status string // "added", "modified", "removed" Patch string // unified diff for this file; may be empty for binary files or when not returned by provider } type ContentEntry struct { Name string Path string Type string // "file" or "dir" } type CommitStatus struct { Context string Status string // "success", "failure", "pending", etc. Description string TargetURL string } // Canonical Review.State values — all adapters must translate to these. // GitHub adapter: "CHANGES_REQUESTED" → "REQUEST_CHANGES", "COMMENTED" → "COMMENT" // Gitea adapter: already uses these values natively const ( ReviewStateApproved = "APPROVED" ReviewStateRequestChanges = "REQUEST_CHANGES" ReviewStateComment = "COMMENT" ReviewStateDismissed = "DISMISSED" ) type Review struct { ID int64 CommitID string Body string State string // one of the ReviewState* constants above User struct { Login string } } type ReviewComment struct { Path string Position int // diff-position (GitHub convention): 1-indexed offset from @@ hunk line CommitID string // must be the PR head SHA; callers are responsible for setting this Body string } // ReviewRequest.Event uses GitHub canonical values: "APPROVE", "REQUEST_CHANGES", "COMMENT" // The Gitea adapter translates to Gitea-native event strings before posting. type ReviewRequest struct { Event string // "APPROVE", "REQUEST_CHANGES", or "COMMENT" Body string Comments []ReviewComment } ``` ### `vcs/util.go` Two utility functions — do **not** add either to the interface: **1. `GetAllFilesInPath`** — recursive directory traversal: ```go // GetAllFilesInPath recursively lists all file paths under the given directory. // Returns a map of path -> file content. func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) (map[string]string, error) ``` Returns `map[string]string` (path → content). Implementation: call `client.ListContents(ctx, owner, repo, path)`, recurse into `Type == "dir"` entries, fetch content for `Type == "file"` entries via `client.GetFileContent`. **2. `BuildLineToPositionMap`** — inverts the diff-position map for use in `main.go`: ```go // BuildLineToPositionMap parses a unified diff and returns a per-file map // of new-file line number → diff-position. // Position is a 1-indexed offset from the @@ hunk line (GitHub convention). // Only addition and context lines have new-file line numbers; deletion lines // have a position but no new-file line, so they are not in the map. // Returns: map[filename]map[newFileLine]diffPosition func BuildLineToPositionMap(diff string) map[string]map[int]int ``` This is the inverse of the Gitea adapter's `position → line` map. Used in `main.go` to convert LLM-produced line numbers to diff-positions before constructing `vcs.ReviewComment`. Replaces `gitea.ParseDiffNewLines` + `diffRanges.Contains` entirely. Counting rules are identical to the diff-position spec in issue #79: - The `@@` hunk header is position 1 for the first hunk - Every line (context, addition, deletion) increments position - A second `@@` hunk continues the count, does not reset - Addition and context lines map to new-file line numbers - Deletion lines: skip (no entry in the map) Unit tests for `BuildLineToPositionMap`: - Single hunk with context, additions, deletions - Multi-hunk file (position counter continues across hunks) - Deletion lines not present in output map - Multiple files in one diff Unit tests for `GetAllFilesInPath`: empty dir, flat dir, nested dirs, mixed. ### Type alias strategy When `gitea/` types have identical fields to `vcs/` types, prefer **type aliases** to avoid mapping boilerplate: ```go // In gitea/types.go (or wherever the type is currently defined) type ContentEntry = vcs.ContentEntry // alias: zero mapping needed type CommitStatus = vcs.CommitStatus // alias: fields are identical ``` Use an **independent copy** (no alias) only when the gitea type has fields that differ from or extend the vcs type (e.g. `gitea.PullRequest` has no `Number` field — it cannot be aliased to `vcs.PullRequest`). In that case, map field-by-field in the adapter. This decision affects which methods are true pass-throughs vs. require mapping — the compile-time check will make divergences visible. ### `review.ContentEntry` and `review.GiteaClient` cleanup `review/repo_persona.go` defines `review.ContentEntry` (mirroring `gitea.ContentEntry` to avoid import cycles) and `review.GiteaClient` interface. Once `vcs/types.go` exists, `review/` can safely import `vcs/` (no cycle — `vcs/` has no upward dependencies). As part of **this issue (#78)**, delete: - `review.ContentEntry` — replace with `vcs.ContentEntry` throughout `review/` - `review.GiteaClient` — replace with `vcs.FileReader` in `review.LoadRepoPersonas` signature - Update `review/repo_persona_test.go`: `mockGiteaClient.ListContents` return type changes from `[]review.ContentEntry` to `[]vcs.ContentEntry` These deletions are owned by #78 and must be included in the same PR as `vcs/types.go`. They are not deferred — #82 depends on them already being gone. ### Compile-time check ```go var _ vcs.Client = (*gitea.Client)(nil) ``` Expected to fail on `PostReview`, `GetFileContentAtRef`, `GetCommitStatuses`, `DismissReview`, `ListContents`, and `ListReviews` — this is intentional (return types differ: `[]gitea.ContentEntry`/`[]gitea.Review` vs `[]vcs.ContentEntry`/`[]vcs.Review` unless type-aliased). Each failure documents a gap the Gitea adapter (issue #79) must bridge. `GetFileContent` and `GetAuthenticatedUser` will pass without changes. ## Exit criteria - `vcs/interfaces.go`, `vcs/types.go`, `vcs/util.go` compile - Compile-time check is present; failures document the gaps - No behavior changes to existing Gitea functionality - Types are extracted from `gitea/`, not invented - `vcs.GetAllFilesInPath` unit tests pass - `vcs.BuildLineToPositionMap` unit tests pass ## Out of scope - Gitea adapter implementation (issue #79) - GitHub client (issues #80, #81) - CLI flag changes (issue #82)
rodin commented 2026-05-12 17:24:49 +00:00
Author
Owner
Copy Link
Copy Source

Update: Reviewer interface must include DismissReview (added in issue #81 design):

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
    DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error
}

Rationale: GitHub cannot delete submitted reviews; DismissReview is the cross-platform primitive for replacing a review. The Gitea adapter implements it by calling the underlying DeleteReview. See #81 for full details.

**Update:** `Reviewer` interface must include `DismissReview` (added in issue #81 design): ```go 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 DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error } ``` Rationale: GitHub cannot delete submitted reviews; `DismissReview` is the cross-platform primitive for replacing a review. The Gitea adapter implements it by calling the underlying `DeleteReview`. See #81 for full details.
rodin commented 2026-05-12 17:28:06 +00:00
Author
Owner
Copy Link
Copy Source

Update from codebase audit (issue #82): Two methods are missing from the interface design and must be added to vcs/interfaces.go:

  1. GetFileContentAtRef — the current code calls GetFileContentRef(ctx, owner, repo, path, ref) to fetch file content at a specific git ref (branch/SHA). The current GetFileContent in the design takes no ref parameter. Add:

    GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error)

  2. GetCommitStatuses — used for CI status checks. Add to PRReader:

    GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error)

    Add CommitStatus to vcs/types.go: { Context, Status, Description string }

The compile-time check var _ vcs.Client = (*gitea.Client)(nil) will fail on both of these — which is correct and expected, as they need adapter coverage.

**Update from codebase audit (issue #82):** Two methods are missing from the interface design and must be added to `vcs/interfaces.go`: 1. **`GetFileContentAtRef`** — the current code calls `GetFileContentRef(ctx, owner, repo, path, ref)` to fetch file content at a specific git ref (branch/SHA). The current `GetFileContent` in the design takes no `ref` parameter. Add: ```go GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) ``` 2. **`GetCommitStatuses`** — used for CI status checks. Add to `PRReader`: ```go GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) ``` Add `CommitStatus` to `vcs/types.go`: `{ Context, Status, Description string }` The compile-time check `var _ vcs.Client = (*gitea.Client)(nil)` will fail on both of these — which is correct and expected, as they need adapter coverage.
rodin commented 2026-05-12 17:29:49 +00:00
Author
Owner
Copy Link
Copy Source

Update: vcs/util.go — GetAllFilesInPath is recursive directory traversal, not a VCS primitive. Do not add it to the vcs.Client interface. Instead, add a utility function in this issue:

// vcs/util.go

// GetAllFilesInPath recursively lists all file paths under the given directory
// using the FileReader interface. Works for any provider.
func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) ([]string, error)

Implementation: call client.ListContents(ctx, owner, repo, path), recurse into entries where Type == "dir", collect entries where Type == "file".

Add a unit test for: empty dir, flat dir, nested dirs, mixed files and dirs.

**Update: `vcs/util.go`** — `GetAllFilesInPath` is recursive directory traversal, not a VCS primitive. Do **not** add it to the `vcs.Client` interface. Instead, add a utility function in this issue: ```go // vcs/util.go // GetAllFilesInPath recursively lists all file paths under the given directory // using the FileReader interface. Works for any provider. func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) ([]string, error) ``` Implementation: call `client.ListContents(ctx, owner, repo, path)`, recurse into entries where `Type == "dir"`, collect entries where `Type == "file"`. Add a unit test for: empty dir, flat dir, nested dirs, mixed files and dirs.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#78
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?