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 129 additions and 0 deletions
Showing only changes of commit 2e6f46f28d - Show all commits
+30
View File
@@ -0,0 +1,30 @@
//go:build ignore
Outdated
Review

[NIT] The compile-time assertion file imports the gitea adapter directly from within package vcs. While gated by //go:build ignore, consider placing this assertion in a separate build-tagged test or a _test.go file to further reduce the chance of accidental inclusion creating an unwanted dependency.

**[NIT]** The compile-time assertion file imports the gitea adapter directly from within package vcs. While gated by //go:build ignore, consider placing this assertion in a separate build-tagged test or a _test.go file to further reduce the chance of accidental inclusion creating an unwanted dependency.
// This file is excluded from normal builds.
Outdated
Review

[NIT] The instruction 'Run go build -v . inside this file to see the documented gaps' is slightly misleading — you can't run a build inside a file. The intended instruction is probably 'Remove the //go:build ignore tag and run go build ./vcs/' or 'Run go build -tags ignore ./vcs/'. A clearer instruction avoids future confusion.

**[NIT]** The instruction 'Run `go build -v .` inside this file to see the documented gaps' is slightly misleading — you can't run a build inside a file. The intended instruction is probably 'Remove the //go:build ignore tag and run `go build ./vcs/`' or 'Run `go build -tags ignore ./vcs/`'. A clearer instruction avoids future confusion.
// Run `go build -v .` inside this file to see the documented gaps between
Outdated
Review

[NIT] The instruction "Run go build -v . inside this file" is misleading. With //go:build ignore, this file is excluded by default. Suggest clarifying to "Run go build -tags ignore ./vcs" (or ./...) to trigger the intentional compile-time assertion.

**[NIT]** The instruction "Run `go build -v .` inside this file" is misleading. With //go:build ignore, this file is excluded by default. Suggest clarifying to "Run `go build -tags ignore ./vcs`" (or `./...`) to trigger the intentional compile-time assertion.
// gitea.Client and vcs.Client that Phase 2 (Gitea adapter) must bridge.
//
// Known gaps (as of Phase 1):
//
// 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.
package vcs
import "gitea.weiker.me/rodin/review-bot/gitea"
// This compile-time assertion intentionally fails.
// It documents what the Gitea adapter must implement to satisfy vcs.Client.
var _ Client = (*gitea.Client)(nil)
Outdated
Review

[NIT] The file has no trailing newline after the last line (var _ Client = (*gitea.Client)(nil)). gofmt enforces a trailing newline; this may cause a gofmt check failure if one is run. Verify the file ends with a newline character.

**[NIT]** The file has no trailing newline after the last line (var _ Client = (*gitea.Client)(nil)). gofmt enforces a trailing newline; this may cause a gofmt check failure if one is run. Verify the file ends with a newline character.
+37
View File
@@ -0,0 +1,37 @@
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)
ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error)
DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) 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.
// 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
}
+62
View File
@@ -0,0 +1,62 @@
// Package vcs defines the shared interface and types for VCS platform clients.
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.
// Adapters (gitea, github) implement these interfaces; the core review logic
// uses them without knowing the underlying platform.
package vcs
// PullRequest holds relevant PR metadata.
type PullRequest struct {
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.
Title string `json:"title"`
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.
Body string `json:"body"`
Head struct {
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".
Sha string `json:"sha"`
Ref string `json:"ref"`
} `json:"head"`
}
// ChangedFile represents a file modified in a PR.
type ChangedFile struct {
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.
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"`
Type string `json:"type"` // "file" or "dir"
}
// Review represents a pull request review.
type Review struct {
ID int64 `json:"id"`
Body string `json:"body"`
User struct {
Login string `json:"login"`
} `json:"user"`
State string `json:"state"`
Stale bool `json:"stale"`
CommitID string `json:"commit_id"`
}
// 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 is the commit SHA the comment is anchored to.
//
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.
// 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"`
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.
}
// ReviewRequest is the payload for posting a review.
type ReviewRequest struct {
// Body is the top-level review comment.
Body string `json:"body"`
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.
// Event is "APPROVED" or "REQUEST_CHANGES".
Event string `json:"event"`
Comments []ReviewComment `json:"comments,omitempty"`
}