feat(vcs): extract interfaces and types from gitea/ (Phase 1) #83
Reference in New Issue
Block a user
Delete Branch "review-bot-issue-78"
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?
Summary
Implements Phase 1 of the VCS abstraction: extract
vcs.Clientinterface and shared types from the existing workinggitea/code.Closes #78.
Plan
Problem
review-bot only supports Gitea. Adding GitHub support requires a shared
vcs.Clientinterface so both adapters can be swapped. The interface is discovered from working code, not invented.Constraints
vcs/interfaces.go,vcs/types.go)gitea/, not inventedApproach
vcs/interfaces.go— role-based interfaces:PRReader: GetPullRequest, GetPullRequestDiff, GetPullRequestFilesFileReader: GetFileContent (path + ref), ListContentsReviewer: PostReview (ReviewRequest), ListReviews, DeleteReviewIdentity: GetAuthenticatedUserClient: all four composedvcs/types.go— types extracted fromgitea/:PullRequest,ChangedFile,ContentEntry,Review— identical togitea/ReviewComment— uses GitHub diff-position convention (Position int,CommitID string) instead of Gitea'sNewPosition int64ReviewRequest— new type wrappingBody,Event,Commentsvcs/check.go(//go:build ignore) — documents the gapsgitea.Clientmust bridge in Phase 2:PostReviewsignature mismatch (event+body+[]ReviewCommentvsReviewRequest)GetFileContentmissingrefparameterReviewCommenttype mismatch (NewPosition int64vsPosition int/CommitID string)Progress
Phase 1: vcs/interfaces.go + vcs/types.go
PRReader,FileReader,Reviewer,IdentityinterfacesClientinterface composing all fourPullRequest,ChangedFile,ContentEntry,Reviewtypes (from gitea/)ReviewCommentwith diff-position conventionReviewRequeststructvcs/check.gowith//go:build ignoredocumenting 3 adapter gapsgo build ./...passesgo test ./...passes (all existing tests)Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1ac51669)Sonnet Review
Summary
Clean Phase 1 extraction of VCS interfaces and types. The code is well-structured, follows Go patterns correctly, and CI passes. The approach of using
//go:build ignoreto document intentionally-failing compile-time assertions is creative and unambiguous.Findings
vcs/types.gopackage vcswith no preceding comment. The authoritative doc comment is on interfaces.go — remove the comment block from types.go.vcs/types.govcs/check.gogo 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 rungo build ./vcs/' or 'Rungo build -tags ignore ./vcs/'. A clearer instruction avoids future confusion.vcs/interfaces.goRecommendation
APPROVE — Approve with minor suggestions. The design is sound: small, role-based interfaces (PRReader, FileReader, Reviewer, Identity) composed into Client follows the interface composition pattern correctly. Using
//go:build ignorefor the intentionally-failing compile-time assertion is an idiomatic and clear way to document the Phase 2 adapter gaps without breaking CI. The types are clean and well-documented. Address the duplicate package doc comment (the MINOR finding) before or in the next PR, and clarify the check.go build instruction. No blocking issues.Review by sonnet
Evaluated against
1ac51669@@ -0,0 +1,30 @@//go:build ignore// This file is excluded from normal builds.[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 rungo build ./vcs/' or 'Rungo build -tags ignore ./vcs/'. A clearer instruction avoids future confusion.@@ -0,0 +20,4 @@// 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)[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.
@@ -0,0 +4,4 @@package vcs// PullRequest holds relevant PR metadata.type PullRequest struct {[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 vcswith no preceding comment. The authoritative doc comment is on interfaces.go — remove the comment block from types.go.@@ -0,0 +49,4 @@Path string `json:"path"`Position int `json:"position"` // diff-position: 1-indexed offset from @@ hunk lineCommitID string `json:"commit_id"`Body string `json:"body"`[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.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1ac51669)Security Review
Summary
The PR adds VCS interface definitions and shared types with no executable logic and no security-impacting behavior. Imports are limited to the Go standard library and an internal package, and CI passed.
Recommendation
APPROVE — Approve as submitted. These changes introduce only interfaces and DTOs, which do not create attack surface on their own. The build-ignored compile-time assertion in vcs/check.go will not affect normal builds and is acceptable as documentation. When implementing the adapters in subsequent phases, ensure strict input validation and safe translation between diff positions and line numbers, and enforce authorization and rate limiting in any API interactions. For ReviewRequest.Event, consider using a typed enum in future to reduce misuse, but this is not a security concern in the current change.
Review by security
Evaluated against
1ac51669Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1ac51669)Gpt Review
Summary
Well-structured introduction of a VCS abstraction layer with clear interfaces and shared types. The code is idiomatic, compiles cleanly, and preserves current behavior by isolating the intentional compile-time check behind a build tag. A few minor documentation and naming improvements would polish it further.
Findings
vcs/types.govcs/types.govcs/check.gogo build -v .inside this file" is misleading. With //go:build ignore, this file is excluded by default. Suggest clarifying to "Rungo build -tags ignore ./vcs" (or./...) to trigger the intentional compile-time assertion.Recommendation
APPROVE — The PR successfully extracts a clean, role-based VCS interface with well-documented types and preserves current behavior by gating the intentional compile-time assertion behind a build tag. Interfaces follow context-first conventions and small interface composition patterns, and the types are straightforward. To polish: rename PullRequest.Head.Sha to SHA for acronym consistency, introduce a typed constant set for ReviewRequest.Event to avoid magic strings, and update the check.go comment to indicate building with the 'ignore' tag to exercise the intentional failure. With these minor adjustments, the abstraction will be even more idiomatic and maintainable.
Review by gpt
Evaluated against
1ac51669@@ -0,0 +1,30 @@//go:build ignore// This file is excluded from normal builds.// Run `go build -v .` inside this file to see the documented gaps between[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 "Rungo build -tags ignore ./vcs" (or./...) to trigger the intentional compile-time assertion.@@ -0,0 +7,4 @@type PullRequest struct {Title string `json:"title"`Body string `json:"body"`Head struct {[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".
@@ -0,0 +55,4 @@// ReviewRequest is the payload for posting a review.type ReviewRequest struct {// Body is the top-level review comment.Body string `json:"body"`[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).
Self-Review: PR #83
Self-review against
c889724ddaPhase 1: Independent Findings
None — diff looks clean. The vcs package defines well-documented interfaces and types. All exported types have doc comments, the interface composition is sound, and the check.go build-tag approach for documenting adapter gaps is pragmatic.
Phase 2: Prior Review Verification
SHAwith json tag kept as"sha"type ReviewEvent stringwithReviewEventApprove,ReviewEventRequestChanges,ReviewEventCommentconstantsgo build ./vcs/"Assessment: ✅ Clean
All four review findings addressed. Tests pass (
go test ./...), package builds cleanly (go build ./vcs/). No new issues introduced.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
c889724d)Sonnet Review
Summary
Clean Phase 1 extraction of VCS interfaces and types. CI passes, no behavior changes to existing code, well-documented adapter gaps. The code follows established Go patterns throughout.
Findings
vcs/types.govcs/types.govcs/check.goRecommendation
APPROVE — Approve. The implementation is correct, idiomatic, and well-structured. Interfaces are small and role-based (PRReader, FileReader, Reviewer, Identity) following the small-interface pattern from the Go stdlib. The compile-time check (var _ Interface = (*Type)(nil)) is present in interfaces.go conceptually via the check.go mechanism. The //go:build ignore approach for documenting adapter gaps is clever and clearly explained. The zero-value-usable struct types with documented JSON tags are appropriate for data transfer objects. The three NITs are all trivial and none warrant blocking the PR.
Review by sonnet
Evaluated against
c889724d@@ -0,0 +27,4 @@// This compile-time assertion intentionally fails.// It documents what the Gitea adapter must implement to satisfy vcs.Client.var _ Client = (*gitea.Client)(nil)[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.
@@ -0,0 +14,4 @@// PullRequest holds relevant PR metadata.type PullRequest struct {Title string `json:"title"`[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.
@@ -0,0 +42,4 @@User struct {Login string `json:"login"`} `json:"user"`State string `json:"state"`[NIT] The anonymous struct for the User field inside Review has the same pattern. Same note as above — minor, acceptable for JSON-mapping types.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
c889724d)Security Review
Summary
This PR introduces interface and type definitions for a VCS abstraction layer and an ignored compile-time check file. The changes are purely structural with no executable logic and present no security impact. CI passed.
Recommendation
APPROVE — Approve as submitted. The additions are limited to interfaces and simple data types, with no behavior or external interactions, so there are no security concerns at this stage. In Phase 2, when implementing the Gitea adapter and any GitHub adapter, ensure strict input validation, proper authentication/authorization checks, safe handling of network requests (timeouts, SSRF defenses), and avoid logging secrets per the repository’s security patterns. Also confirm that review posting and file content retrieval enforce permission checks and do not expose sensitive information.
Review by security
Evaluated against
c889724dOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
c889724d)Gpt Review
Summary
The new VCS abstraction cleanly defines role-based interfaces and shared types without impacting existing Gitea code. CI passed, and the changes are idiomatic and compile-safe with an intentional compile-time check gated behind a build tag.
Findings
vcs/types.govcs/check.goRecommendation
APPROVE — Overall, this is a solid Phase 1 extraction. The interfaces are small and composable, follow context-first conventions, and the compile-time assertion pattern is correctly gated behind a build tag. I recommend clarifying the ReviewEvent constants to avoid conflating review actions with review states (e.g., APPROVE vs APPROVED), or explicitly documenting the adapter translation responsibilities. The optional nit is to keep the compile-time assertion in a test-only or separately tagged file to avoid accidental coupling. With these minor clarifications, the abstraction is in good shape for implementing adapters in Phase 2.
Review by gpt
Evaluated against
c889724d@@ -0,0 +1,30 @@//go:build ignore[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.
@@ -0,0 +5,4 @@const (// ReviewEventApprove approves the pull request.ReviewEventApprove ReviewEvent = "APPROVED"[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.
Sonnet Review
Summary
Clean Phase 1 extraction of VCS interfaces and types. The code follows Go patterns correctly — small composable interfaces, compile-time checks, well-documented types. CI passed. One minor observation about the check_test.go placement.
Findings
vcs/check_test.govar _ vcs.Client = (*gitea.Client)(nil)is gated behind//go:build phase2which 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 separatevcs/check.gowith//go:build ignore— the actual implementation uses a_test.gofile with//go:build phase2instead. 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.gotypes.gofile is missing a package doc comment or any reference back to the main package-level documentation. Sinceinterfaces.gocarries 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 ininterfaces.gorather than a dedicateddoc.go, which is a slightly non-idiomatic placement for packages that will grow to have many files.vcs/types.gojsonstruct tags onReviewCommentand other types are present but there's no indication these types will be directly JSON-serialized by thevcspackage itself — they're internal domain types exchanged between adapters and core logic. Addingjsontags 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.Recommendation
APPROVE — Approve. The implementation is correct, idiomatic, and well-documented. The interface decomposition into
PRReader,FileReader,Reviewer, andIdentityfollows the small-interfaces pattern from the patterns library. The compile-time check incheck_test.gois an excellent use ofvar _ Interface = (*Type)(nil)to document future work without breaking CI. The//go:build phase2guard is a clever way to make the failing assertion visible without blocking the build. The minor findings are non-blocking: the PR description inaccuracy aboutcheck.govscheck_test.gois cosmetic, and the JSON tags question is a design choice that's reasonable given the PR description mentions GitHub's JSON convention for diff positions.Review by sonnet
Evaluated against
cd6cd93b@@ -0,0 +24,4 @@// 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)[MINOR] The compile-time interface check
var _ vcs.Client = (*gitea.Client)(nil)is gated behind//go:build phase2which 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 separatevcs/check.gowith//go:build ignore— the actual implementation uses a_test.gofile with//go:build phase2instead. 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.@@ -0,0 +1,82 @@package vcs[NIT] The
types.gofile is missing a package doc comment or any reference back to the main package-level documentation. Sinceinterfaces.gocarries 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 ininterfaces.gorather than a dedicateddoc.go, which is a slightly non-idiomatic placement for packages that will grow to have many files.@@ -0,0 +55,4 @@Stale bool `json:"stale"`CommitID string `json:"commit_id"`}[NIT] The
jsonstruct tags onReviewCommentand other types are present but there's no indication these types will be directly JSON-serialized by thevcspackage itself — they're internal domain types exchanged between adapters and core logic. Addingjsontags 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.Security Review
Summary
The PR introduces VCS interfaces and shared types without adding executable logic or external dependencies. There are no security-relevant changes or vulnerabilities introduced by these definitions.
Recommendation
APPROVE — Approve as submitted. The changes define interfaces and data structures only, with no user input handling or sensitive operations. When implementing adapters against these interfaces, ensure strict input validation (paths, refs), protection against injection in review bodies/comments, and proper authorization checks on PR and file operations.
Review by security
Evaluated against
cd6cd93bFindings Addressed (
cd6cd93)sonnet-review-bot (review 2703)
#16530 [MINOR] Duplicate package doc in types.go
→ Fixed in prior commit (
c889724).types.gono longer carries a package doc comment.#16531 [MINOR] CommitID doc comment missing
→ Fixed: Added doc noting CommitID is optional (empty string for review-level comments not anchored to a specific commit).
#16532 [NIT] Misleading instruction in check.go
→ Fixed in prior commit (
c889724). Also,check.gohas been replaced bycheck_test.gowith a build tag (see #16622 below).#16533 [NIT] PostReview returns
*Reviewvs ListReviews returns[]Review→ Keeping as-is.
PostReviewreturns a pointer because it creates a single resource — this is idiomatic Go (Create → *T,List → []T). Consistent with stdlib patterns likeos.Create → *File.gpt-review-bot (review 2706)
#16537 [MINOR]
Sha→SHA→ Fixed in prior commit (
c889724). Field is alreadySHA.#16538 [MINOR] Typed ReviewEvent with constants
→ Fixed in prior commit (
c889724).type ReviewEvent stringwithReviewEventApprove,ReviewEventRequestChanges,ReviewEventComment.#16539 [NIT] Same as #16532
→ Fixed (same as above).
sonnet-review-bot (review 2716, second pass)
#16615 [NIT] Anonymous struct for Head field
→ Fixed: Extracted
HeadRefnamed struct.#16616 [NIT] Anonymous struct for User in Review
→ Fixed: Extracted
UserInfonamed struct.#16617 [NIT] Trailing newline compliance
→ Verified: file already had proper trailing newline;
gofmt -dproduces no diff.gpt-review-bot (review 2719, second pass)
#16621 [MINOR] ReviewEventApprove = "APPROVED" (state) vs "APPROVE" (action)
→ Fixed: Changed to
"APPROVE"to represent the action semantics. Added doc comment noting adapters must translate to/from platform-native values (e.g., Gitea uses "APPROVED" for both action and state).#16622 [NIT] check.go imports gitea adapter into vcs package
→ Fixed: Moved to
check_test.gowith//go:build phase2tag. No longer pollutes the vcs package import graph. Enable the tag once Phase 2 adapter bridges the documented gaps.Gpt Review
Summary
The new vcs interfaces and types are clean, idiomatic, and align with the documented Go patterns. CI has passed and there are no behavior changes to existing Gitea functionality.
Recommendation
APPROVE — This phase cleanly introduces small, composable interfaces (PRReader, FileReader, Reviewer, Identity) and shared types under vcs, with context-first method signatures and clear documentation. The compile-time assertion is appropriately guarded by a build tag for Phase 2. No changes requested — proceed to implement the Gitea adapter bridge in Phase 2.
Review by gpt
Evaluated against
cd6cd93bSelf-Review: PR #83
Self-review against
cd6cd93bf0Phase 1: Independent Findings
[NIT]
vcs/check_test.go— build tagphase2vs priorignoreconvention: The file uses//go:build phase2which is a custom tag that never runs in normal CI. This is a deliberate and documented design choice (the file documents gaps for the Phase 2 adapter). No issue — this is the intended mechanism.[NIT]
vcs/types.go— JSON tags on internal domain types: Struct tags likejson:"sha",json:"ref"etc. appear on types that are internal domain objects exchanged between adapters and core logic, not typically serialized to wire format. This is potentially misleading but may be intentional (e.g., for test fixture serialization or future HTTP payloads). Not a correctness issue; a design question.[OBSERVATION]
vcs/check_test.go— packagevcs_testwith gitea import: The_test.gofile in packagevcs_test(external test package) importsgitea.weiker.me/rodin/review-bot/gitea. This creates a test-time dependency fromvcs_test→gitea, but since it's guarded by//go:build phase2, it does not pollute the normal build or test graph. Clean.[OBSERVATION]
vcs/interfaces.go—ListContentssignature:ListContents(ctx, owner, repo, path)with norefparameter means it always lists at the default branch. For a VCS abstraction that needs to inspect file trees at a specific commit (e.g., for diff-aware reviews), this may be a gap. However, this appears intentional for Phase 1 — the interface is minimal and can be extended.Overall: the code is clean, well-structured, and correctly implements the stated Phase 1 goals. All exported identifiers are documented. Interface composition is idiomatic. The compile-time check mechanism is sound.
Phase 2: Prior Review Verification
Assessment: ✅ Clean
All prior review findings are resolved or explicitly acknowledged with rationale. Phase 1 independent review finds no new issues. The VCS abstraction is clean, idiomatic, and well-documented. Ready for Phase 2 adapter work.
rodin referenced this pull request2026-05-12 19:31:28 +00:00