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

Merged
aweiker merged 4 commits from review-bot-issue-78 into feature/github-support 2026-05-12 19:14:45 +00:00
3 changed files with 149 additions and 0 deletions
+27
View File
@@ -0,0 +1,27 @@
//go:build phase2
package vcs_test
import (
"gitea.weiker.me/rodin/review-bot/gitea"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Compile-time assertion: documents the gap between gitea.Client and vcs.Client.
// Guarded by the "phase2" build tag — enable once the Gitea adapter bridges these gaps:
//
// 1. PostReview signature mismatch:
// gitea.Client: PostReview(ctx, owner, repo, number, event, body string, comments []gitea.ReviewComment)
// vcs.Reviewer: PostReview(ctx, owner, repo, number, req vcs.ReviewRequest)
//
// 2. GetFileContent signature mismatch:
// gitea.Client: GetFileContent(ctx, owner, repo, filepath string) [no ref; uses default branch]
// vcs.FileReader: GetFileContent(ctx, owner, repo, path, ref string)
// (gitea.Client has GetFileContentRef for the ref variant)
//
// 3. ReviewComment type mismatch:
// gitea.ReviewComment uses NewPosition int64 (Gitea line-number convention)
// vcs.ReviewComment uses Position int (GitHub diff-position convention)
//
// The Gitea adapter (Phase 2) will wrap gitea.Client to bridge these gaps.
var _ vcs.Client = (*gitea.Client)(nil)
Review

[MINOR] The compile-time interface check var _ vcs.Client = (*gitea.Client)(nil) is gated behind //go:build phase2 which means it never runs in normal CI. Its purpose is to document the gap, which is fine, but the comment in the PR description mentions a separate vcs/check.go with //go:build ignore — the actual implementation uses a _test.go file with //go:build phase2 instead. This is a reasonable choice (test files are safer than build-ignored files), but the PR description is slightly inaccurate about the mechanism. Not a code problem, just a documentation inconsistency between the PR description and the actual implementation.

**[MINOR]** The compile-time interface check `var _ vcs.Client = (*gitea.Client)(nil)` is gated behind `//go:build phase2` which means it never runs in normal CI. Its purpose is to document the gap, which is fine, but the comment in the PR description mentions a separate `vcs/check.go` with `//go:build ignore` — the actual implementation uses a `_test.go` file with `//go:build phase2` instead. This is a reasonable choice (test files are safer than build-ignored files), but the PR description is slightly inaccurate about the mechanism. Not a code problem, just a documentation inconsistency between the PR description and the actual implementation.
+40
View File
@@ -0,0 +1,40 @@
// Package vcs defines the shared VCS client interface and supporting types.
// Platform adapters (gitea, github) implement these interfaces so the core
// review logic can work with any VCS platform without platform-specific code.
package vcs
import "context"
// PRReader can fetch pull request metadata, diffs, and changed files.
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)
}
// FileReader can fetch file contents and list directory entries.
type FileReader interface {
GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error)
ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
}
// Reviewer can post, list, and delete pull request reviews.
type Reviewer interface {
PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error)
Review

[NIT] PostReview returns (*Review, error). Posting a review on most platforms (including Gitea and GitHub) returns a created review object, so this is reasonable. However, ListReviews returns []Review (value slice) while PostReview returns *Review (pointer). Consider making the return consistent — either []Review and Review, or []*Review and *Review — to avoid callers having to dereference in one case but not the other.

**[NIT]** PostReview returns (*Review, error). Posting a review on most platforms (including Gitea and GitHub) returns a created review object, so this is reasonable. However, ListReviews returns []Review (value slice) while PostReview returns *Review (pointer). Consider making the return consistent — either []Review and Review, or []*Review and *Review — to avoid callers having to dereference in one case but not the other.
ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error)
DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
}
// Identity can report who the authenticated user is.
type Identity interface {
GetAuthenticatedUser(ctx context.Context) (string, error)
}
// Client is the full VCS interface: PR reads, file reads, review management, and identity.
// Platform adapters (gitea, github) implement this interface.
type Client interface {
PRReader
FileReader
Reviewer
Identity
}
+82
View File
@@ -0,0 +1,82 @@
package vcs
Review

[NIT] The types.go file is missing a package doc comment or any reference back to the main package-level documentation. Since interfaces.go carries the package doc (// Package vcs defines...), this is fine per Go convention (only one file needs the package comment), but it's worth noting the package doc lives in interfaces.go rather than a dedicated doc.go, which is a slightly non-idiomatic placement for packages that will grow to have many files.

**[NIT]** The `types.go` file is missing a package doc comment or any reference back to the main package-level documentation. Since `interfaces.go` carries the package doc (`// Package vcs defines...`), this is fine per Go convention (only one file needs the package comment), but it's worth noting the package doc lives in `interfaces.go` rather than a dedicated `doc.go`, which is a slightly non-idiomatic placement for packages that will grow to have many files.
// ReviewEvent is the event type for a pull request review action.
// Adapters must translate these action constants to/from platform-native values.
// For example, Gitea uses "APPROVED" as both action and state, while GitHub
// uses "APPROVE" for the action and returns "approved" as the state.
type ReviewEvent string
Outdated
Review

[MINOR] The package doc comment in types.go duplicates the one in interfaces.go almost verbatim. Only one file in a package should carry the package doc comment; the others should have no package comment or just package vcs with no preceding comment. The authoritative doc comment is on interfaces.go — remove the comment block from types.go.

**[MINOR]** The package doc comment in types.go duplicates the one in interfaces.go almost verbatim. Only one file in a package should carry the package doc comment; the others should have no package comment or just `package vcs` with no preceding comment. The authoritative doc comment is on interfaces.go — remove the comment block from types.go.
Review

[MINOR] ReviewEventApprove is set to "APPROVED", which matches a review state rather than an action. If these constants represent actions to post (as implied by ReviewRequest.Event), consider using "APPROVE" to align with GitHub semantics, or clearly document that adapters must translate these abstracted values to provider-specific event strings.

**[MINOR]** ReviewEventApprove is set to "APPROVED", which matches a review state rather than an action. If these constants represent actions to post (as implied by ReviewRequest.Event), consider using "APPROVE" to align with GitHub semantics, or clearly document that adapters must translate these abstracted values to provider-specific event strings.
const (
// ReviewEventApprove approves the pull request.
Outdated
Review

[MINOR] Acronym casing: field name "Sha" should be "SHA" to follow Go acronym capitalization conventions (see style pattern: Acronyms Are All-Caps). The JSON tag can remain "sha".

**[MINOR]** Acronym casing: field name "Sha" should be "SHA" to follow Go acronym capitalization conventions (see style pattern: Acronyms Are All-Caps). The JSON tag can remain "sha".
ReviewEventApprove ReviewEvent = "APPROVE"
// ReviewEventRequestChanges requests changes to the pull request.
ReviewEventRequestChanges ReviewEvent = "REQUEST_CHANGES"
// ReviewEventComment posts a review comment without approval or rejection.
ReviewEventComment ReviewEvent = "COMMENT"
)
Outdated
Review

[NIT] The anonymous struct for the Head field works but embedding a named struct would be slightly cleaner and more testable (allows construction via named literal without field-name lookup). This is a minor style preference; anonymous embedded structs in this pattern are common in JSON-mapping code.

**[NIT]** The anonymous struct for the Head field works but embedding a named struct would be slightly cleaner and more testable (allows construction via named literal without field-name lookup). This is a minor style preference; anonymous embedded structs in this pattern are common in JSON-mapping code.
// HeadRef identifies the source branch and latest commit of a pull request.
type HeadRef struct {
SHA string `json:"sha"`
Ref string `json:"ref"`
}
// UserInfo identifies a user by login name.
type UserInfo struct {
Login string `json:"login"`
}
// PullRequest holds relevant PR metadata.
type PullRequest struct {
Title string `json:"title"`
Body string `json:"body"`
Head HeadRef `json:"head"`
}
// ChangedFile represents a file modified in a PR.
type ChangedFile struct {
Filename string `json:"filename"`
Status string `json:"status"`
}
// ContentEntry represents a file or directory entry from the contents API.
type ContentEntry struct {
Name string `json:"name"`
Path string `json:"path"`
Review

[NIT] The anonymous struct for the User field inside Review has the same pattern. Same note as above — minor, acceptable for JSON-mapping types.

**[NIT]** The anonymous struct for the User field inside Review has the same pattern. Same note as above — minor, acceptable for JSON-mapping types.
Type string `json:"type"` // "file" or "dir"
}
// Review represents a pull request review.
type Review struct {
ID int64 `json:"id"`
Body string `json:"body"`
Outdated
Review

[MINOR] ReviewComment.CommitID is described as 'the commit SHA the comment is anchored to', but for Gitea (Phase 2 adapter) this will always need to be translated/populated. There is no enforcement that CommitID is non-empty. Consider noting in the doc comment whether CommitID is required or optional, and what the zero-value means, so adapter authors know what invariant to maintain.

**[MINOR]** ReviewComment.CommitID is described as 'the commit SHA the comment is anchored to', but for Gitea (Phase 2 adapter) this will always need to be translated/populated. There is no enforcement that CommitID is non-empty. Consider noting in the doc comment whether CommitID is required or optional, and what the zero-value means, so adapter authors know what invariant to maintain.
User UserInfo `json:"user"`
State string `json:"state"`
Stale bool `json:"stale"`
CommitID string `json:"commit_id"`
}
Review

[MINOR] Event is documented as a couple of magic strings ("APPROVED", "REQUEST_CHANGES"). Consider introducing a typed alias (e.g., type ReviewEvent string) with exported constants for the allowed values to provide compile-time safety and self-documenting usage (see style pattern on typed constants for enumerations).

**[MINOR]** Event is documented as a couple of magic strings ("APPROVED", "REQUEST_CHANGES"). Consider introducing a typed alias (e.g., type ReviewEvent string) with exported constants for the allowed values to provide compile-time safety and self-documenting usage (see style pattern on typed constants for enumerations).
Review

[NIT] The json struct tags on ReviewComment and other types are present but there's no indication these types will be directly JSON-serialized by the vcs package itself — they're internal domain types exchanged between adapters and core logic. Adding json tags suggests these might be serialized to wire format, which could be misleading. If these types are purely internal, the tags add no value and could be omitted. This is a design question rather than a bug.

**[NIT]** The `json` struct tags on `ReviewComment` and other types are present but there's no indication these types will be directly JSON-serialized by the `vcs` package itself — they're internal domain types exchanged between adapters and core logic. Adding `json` tags suggests these might be serialized to wire format, which could be misleading. If these types are purely internal, the tags add no value and could be omitted. This is a design question rather than a bug.
// ReviewComment represents an inline comment in a review.
// All adapters use GitHub diff-position convention:
// - Position is a 1-indexed offset from the @@ hunk line in the unified diff.
// - CommitID identifies the commit the comment is anchored to.
// It is optional; omit (empty string) for review-level comments that are
// not attached to a specific commit.
//
// Adapters are responsible for translating to/from platform-native formats
// (e.g. Gitea uses line numbers; GitHub uses diff positions natively).
type ReviewComment struct {
Path string `json:"path"`
Position int `json:"position"` // diff-position: 1-indexed offset from @@ hunk line
CommitID string `json:"commit_id"`
Body string `json:"body"`
}
// ReviewRequest is the payload for posting a review.
type ReviewRequest struct {
// Body is the top-level review comment.
Body string `json:"body"`
// Event is the review action (approve, request changes, or comment).
Event ReviewEvent `json:"event"`
Comments []ReviewComment `json:"comments,omitempty"`
}