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

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
Conversation 13 Commits 4 Files Changed 3
+38 -30
2 changed files with 38 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit cd6cd93bf0 - Show all commits
vcs/check.go → vcs/check_test.go
+11 -14
View File
@@ -1,10 +1,14 @@
//go:build ignore
//go:build phase2
// This file is excluded from normal builds.
// Remove the //go:build ignore tag and run `go build ./vcs/` to see the documented gaps between
// gitea.Client and vcs.Client that Phase 2 (Gitea adapter) must bridge.
//
// Known gaps (as of Phase 1):
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)
@@ -20,11 +24,4 @@
// 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)
var _ vcs.Client = (*gitea.Client)(nil)
sonnet-review-bot commented 2026-05-12 19:07:24 +00:00
Review
Copy Link
Copy Source

[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.
vcs/types.go
+27 -16
View File
@@ -1,25 +1,36 @@
package vcs
sonnet-review-bot commented 2026-05-12 19:07:24 +00:00
Review
Copy Link
Copy Source

[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
sonnet-review-bot commented 2026-05-12 17:06:39 +00:00
Outdated
Review
Copy Link
Copy Source

[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.
gpt-review-bot commented 2026-05-12 19:02:18 +00:00
Review
Copy Link
Copy Source

[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.
gpt-review-bot commented 2026-05-12 17:07:56 +00:00
Outdated
Review
Copy Link
Copy Source

[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 = "APPROVED"
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"
)
sonnet-review-bot commented 2026-05-12 19:01:15 +00:00
Outdated
Review
Copy Link
Copy Source

[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 struct {
SHA string `json:"sha"`
Ref string `json:"ref"`
} `json:"head"`
Title string `json:"title"`
Body string `json:"body"`
Head HeadRef `json:"head"`
}
// ChangedFile represents a file modified in a PR.
1
 @@ -37,20 +48,20 @@ type ContentEntry struct {
// 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"`
ID int64 `json:"id"`
Body string `json:"body"`
sonnet-review-bot commented 2026-05-12 17:06:39 +00:00
Outdated
Review
Copy Link
Copy Source

[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"`
}
gpt-review-bot commented 2026-05-12 17:07:56 +00:00
Review
Copy Link
Copy Source

[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).
sonnet-review-bot commented 2026-05-12 19:07:24 +00:00
Review
Copy Link
Copy Source

[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 is the commit SHA the comment is anchored to.
// - 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).