fix(vcs): thread CommitID through abstraction layer (#114)
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s
Add CommitID field to vcs.ReviewRequest so the commit anchor propagates through the vcs.Client interface to platform adapters. Changes: - vcs/types.go: Add CommitID string field to ReviewRequest - gitea/client.go: Add commitID parameter to PostReview, include in API payload - gitea/adapter.go: Pass req.CommitID to underlying client - github/review.go: Use req.CommitID as primary, fall back to comment-level - cmd/review-bot/main.go: Set CommitID on ReviewRequest from pr.Head.SHA Fixes #114
This commit is contained in:
+11
-7
@@ -82,21 +82,25 @@ func translateGitHubReviewState(state string) string {
|
||||
// (via the omitempty tag on postReviewRequest.Comments).
|
||||
//
|
||||
// The GitHub API accepts a single commit_id per review submission. PostReview
|
||||
// extracts it from the first comment with a non-empty CommitID. If any subsequent
|
||||
// comment specifies a different CommitID, PostReview returns ErrConflictingCommitIDs.
|
||||
// Comments with an empty CommitID are allowed and inherit the review-level value.
|
||||
// uses req.CommitID as the primary commit anchor. If req.CommitID is empty,
|
||||
// it falls back to extracting from the first comment with a non-empty CommitID.
|
||||
// If any subsequent comment specifies a different CommitID, PostReview returns
|
||||
// ErrConflictingCommitIDs. Comments with an empty CommitID are allowed and
|
||||
// inherit the review-level value.
|
||||
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
|
||||
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
|
||||
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||
|
||||
payload := postReviewRequest{
|
||||
Body: req.Body,
|
||||
Event: string(req.Event),
|
||||
Body: req.Body,
|
||||
Event: string(req.Event),
|
||||
CommitID: req.CommitID,
|
||||
}
|
||||
|
||||
// Build the payload in one pass. The GitHub API accepts a single commit_id
|
||||
// per review; we extract it from the first comment that supplies one and
|
||||
// reject the request if any other comment disagrees.
|
||||
// per review. req.CommitID is the primary source; if empty, we extract from
|
||||
// the first comment that supplies one. Reject if any comment disagrees with
|
||||
// the resolved commit_id.
|
||||
for _, comment := range req.Comments {
|
||||
if comment.CommitID != "" {
|
||||
if payload.CommitID == "" {
|
||||
|
||||
@@ -389,3 +389,96 @@ func TestPostReview_ConflictingCommitIDs(t *testing.T) {
|
||||
t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPostReview_RequestCommitID_TakesPriority(t *testing.T) {
|
||||
var gotPayload struct {
|
||||
CommitID string `json:"commit_id"`
|
||||
Body string `json:"body"`
|
||||
}
|
||||
|
||||
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
|
||||
json.NewDecoder(r.Body).Decode(&gotPayload)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(map[string]any{
|
||||
"id": 42,
|
||||
"body": "LGTM",
|
||||
"state": "APPROVED",
|
||||
"commit_id": "req-level-sha",
|
||||
"user": map[string]any{"login": "bot"},
|
||||
})
|
||||
})
|
||||
|
||||
review, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
|
||||
Body: "LGTM",
|
||||
Event: vcs.ReviewEventApprove,
|
||||
CommitID: "req-level-sha",
|
||||
Comments: []vcs.ReviewComment{
|
||||
{Path: "a.go", Position: 1, CommitID: "req-level-sha", Body: "looks good"},
|
||||
},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if gotPayload.CommitID != "req-level-sha" {
|
||||
t.Errorf("sent commit_id = %q, want %q", gotPayload.CommitID, "req-level-sha")
|
||||
}
|
||||
if review.CommitID != "req-level-sha" {
|
||||
t.Errorf("review.CommitID = %q, want %q", review.CommitID, "req-level-sha")
|
||||
}
|
||||
}
|
||||
|
||||
func TestPostReview_RequestCommitID_ConflictsWithComment(t *testing.T) {
|
||||
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
|
||||
t.Fatal("request should not be sent when commit IDs conflict")
|
||||
})
|
||||
|
||||
// req.CommitID is set, and a comment has a different CommitID → conflict
|
||||
_, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
|
||||
Body: "Review",
|
||||
Event: vcs.ReviewEventComment,
|
||||
CommitID: "req-sha",
|
||||
Comments: []vcs.ReviewComment{
|
||||
{Path: "a.go", Position: 1, CommitID: "different-sha", Body: "nit"},
|
||||
},
|
||||
})
|
||||
if err == nil {
|
||||
t.Fatal("expected error for conflicting commit IDs")
|
||||
}
|
||||
if !errors.Is(err, ErrConflictingCommitIDs) {
|
||||
t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPostReview_RequestCommitID_FallbackToComment(t *testing.T) {
|
||||
var gotPayload struct {
|
||||
CommitID string `json:"commit_id"`
|
||||
}
|
||||
|
||||
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
|
||||
json.NewDecoder(r.Body).Decode(&gotPayload)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(map[string]any{
|
||||
"id": 43,
|
||||
"body": "ok",
|
||||
"state": "COMMENTED",
|
||||
"commit_id": "comment-sha",
|
||||
"user": map[string]any{"login": "bot"},
|
||||
})
|
||||
})
|
||||
|
||||
// req.CommitID is empty, so it falls back to the comment's CommitID
|
||||
_, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
|
||||
Body: "ok",
|
||||
Event: vcs.ReviewEventComment,
|
||||
// CommitID intentionally empty
|
||||
Comments: []vcs.ReviewComment{
|
||||
{Path: "a.go", Position: 1, CommitID: "comment-sha", Body: "note"},
|
||||
},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if gotPayload.CommitID != "comment-sha" {
|
||||
t.Errorf("sent commit_id = %q, want %q (fallback from comment)", gotPayload.CommitID, "comment-sha")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user