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

PR #112: gitea.Adapter.PostReview bypasses commitID — abstraction layer drops commit anchor #114

New Issue
Closed
opened 2026-05-13 19:07:20 +00:00 by rodin · 1 comment
rodin commented 2026-05-13 19:07:20 +00:00
Owner
Copy Link
Copy Source

What was missed

PR #112 added a commitID string parameter to gitea.Client.PostReview and updated cmd/review-bot/main.go to pass evaluatedSHA directly to the client. However, gitea.Adapter.PostReview (the vcs.Client implementation) was not updated — it still calls a.client.PostReview with the old 7-argument signature, omitting the new commitID parameter. Since the review-bot is designed to operate through the vcs.Client abstraction, the commit anchoring added by this PR is silently dropped whenever a caller uses the adapter interface.

In gitea/adapter.go, line 181:

review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, giteaComments)

Should be:

review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, req.CommitID, giteaComments)

But vcs.ReviewRequest has no CommitID field (see related issue), so the adapter has no way to thread it through without the ReviewRequest fix.

Source

  • PR: #112 — feat(gitea): pass commit_id explicitly in PostReview (#107)
  • Linked issue: #107 — PostReview: pass CommitID explicitly via ReviewRequest
  • Code quality rule: Acceptance criterion not met — fix must propagate through the abstraction layer
  • File: gitea/adapter.go line 181

What needs to happen

  1. Add CommitID string field to vcs.ReviewRequest (see related issue)
  2. Update gitea.Adapter.PostReview to pass req.CommitID to a.client.PostReview
  3. Update github.Client.PostReview to use req.CommitID as the primary commit anchor, falling back to comment-derived CommitID if empty (as originally specified in issue #107)
  4. All callers that currently use the low-level gitea.Client.PostReview directly should migrate to the adapter

References

## What was missed PR #112 added a `commitID string` parameter to `gitea.Client.PostReview` and updated `cmd/review-bot/main.go` to pass `evaluatedSHA` directly to the client. However, `gitea.Adapter.PostReview` (the `vcs.Client` implementation) was **not updated** — it still calls `a.client.PostReview` with the old 7-argument signature, omitting the new `commitID` parameter. Since the review-bot is designed to operate through the `vcs.Client` abstraction, the commit anchoring added by this PR is silently dropped whenever a caller uses the adapter interface. In `gitea/adapter.go`, line 181: ```go review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, giteaComments) ``` Should be: ```go review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, req.CommitID, giteaComments) ``` But `vcs.ReviewRequest` has no `CommitID` field (see related issue), so the adapter has no way to thread it through without the `ReviewRequest` fix. ## Source - PR: #112 — feat(gitea): pass commit_id explicitly in PostReview (#107) - Linked issue: #107 — PostReview: pass CommitID explicitly via ReviewRequest - Code quality rule: Acceptance criterion not met — fix must propagate through the abstraction layer - File: `gitea/adapter.go` line 181 ## What needs to happen 1. Add `CommitID string` field to `vcs.ReviewRequest` (see related issue) 2. Update `gitea.Adapter.PostReview` to pass `req.CommitID` to `a.client.PostReview` 3. Update `github.Client.PostReview` to use `req.CommitID` as the primary commit anchor, falling back to comment-derived CommitID if empty (as originally specified in issue #107) 4. All callers that currently use the low-level `gitea.Client.PostReview` directly should migrate to the adapter ## References - [PR #112](https://gitea.weiker.me/rodin/review-bot/pulls/112) - [Issue #107](https://gitea.weiker.me/rodin/review-bot/issues/107)
rodin added the ai-reviewbug labels 2026-05-13 19:07:20 +00:00
rodin self-assigned this 2026-05-13 20:22:05 +00:00
rodin commented 2026-05-13 20:24:39 +00:00
Author
Owner
Copy Link
Copy Source

Plan: Thread CommitID through the VCS abstraction layer

Problem

PR #112 added commitID support to gitea.Client.PostReview at the transport level but never connected it to the vcs.Client abstraction. The vcs.ReviewRequest struct has no CommitID field, so gitea.Adapter.PostReview cannot thread a commit anchor through to the underlying Gitea API. Callers using the vcs.Client interface (the intended path) silently lose commit anchoring.

The GitHub client extracts CommitID from individual ReviewComment.CommitID fields, but there's no top-level request-scoped CommitID — meaning body-only reviews (no inline comments) can't be anchored either.

Constraints

  • Must not break the existing vcs.Client interface contract for the GitHub adapter
  • gitea.Client.PostReview currently takes positional parameters — adding commitID as a new positional param changes the signature for all callers
  • The Gitea API accepts commit_id as a top-level field in the CreatePullReview request body
  • GitHub API accepts commit_id as a top-level field in the review creation payload
  • Both cmd/review-bot/main.go (uses vcs.Client interface) and gitea/adapter.go must work correctly after the change

Proposed Approach

1. Add CommitID field to vcs.ReviewRequest

type ReviewRequest struct {
    Body     string          `json:"body"`
    Event    ReviewEvent     `json:"event"`
    CommitID string          `json:"commit_id,omitempty"`
    Comments []ReviewComment `json:"comments,omitempty"`
}

This is the canonical way to pass a commit anchor through the abstraction layer.

2. Update gitea.Client.PostReview to accept and send commitID

Add commitID string parameter to the function signature. Include commit_id in the JSON payload sent to the Gitea API:

func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error)

Payload struct gains: CommitID string \x60json:"commit_id,omitempty"\x60

3. Update gitea.Adapter.PostReview to pass req.CommitID

review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, req.CommitID, giteaComments)

4. Update github.Client.PostReview to prefer req.CommitID

The GitHub client should use req.CommitID as the primary commit anchor. If empty, fall back to extracting from the first comment's CommitID (existing behavior). This matches the issue's specification.

5. Update cmd/review-bot/main.go to set ReviewRequest.CommitID

reviewReq := vcs.ReviewRequest{
    Body:     reviewBody,
    Event:    event,
    CommitID: pr.Head.SHA,
    Comments: inlineComments,
}

6. Update tests

  • gitea/adapter_test.go: Verify that commit_id appears in the API payload when ReviewRequest.CommitID is set
  • gitea/client_test.go: Verify the new parameter is serialized correctly
  • github/review_test.go: Verify req.CommitID takes priority over comment-level CommitID

Error Cases

  • Empty CommitID: Omitted from JSON payload via omitempty — API falls back to default behavior (Gitea pins to PR head)
  • Invalid CommitID: API returns an error — bubbles up as-is through the adapter (no special handling needed)
  • Conflicting CommitID: For GitHub, req.CommitID takes priority. If req.CommitID is empty and comments disagree, existing ErrConflictingCommitIDs logic applies (in github/review.go)

Edge Cases

  • Reviews with no inline comments but with CommitID — must still anchor correctly
  • Reviews where req.CommitID differs from ReviewComment.CommitID — req-level wins (it's the intended anchor)
  • Backward compatibility: existing code that doesn't set CommitID continues to work (zero-value string → omitempty → not sent)

Testing Strategy

  • Unit tests for gitea.Client.PostReview verifying commit_id in payload
  • Unit tests for gitea.Adapter.PostReview verifying end-to-end threading
  • Unit tests for github.Client.PostReview verifying req-level priority
  • Integration test verification that existing cmd/review-bot/main.go flow works

Open Questions

  • The github/review.go file has duplicate type declarations with github/reviews.go (both define reviewResponse, dismissReviewRequest). This appears to be an existing issue — I will NOT attempt to fix it in this PR as it's out of scope. However, it may cause compilation errors that I'll need to investigate.
  • Should ReviewComment.CommitID be deprecated now that we have request-level CommitID? I think no — per-comment CommitID is still useful for the GitHub adapter's fallback behavior and for mixed-commit scenarios. But the adapter comment in gitea/adapter.go (line 170-172) that says "intentionally not forwarded" should be updated to reference the request-level field.

Completion Checklist

  1. vcs.ReviewRequest has CommitID string field with correct JSON tag
  2. gitea.Client.PostReview includes commit_id in API payload when non-empty
  3. gitea.Adapter.PostReview passes req.CommitID to underlying client
  4. github.Client.PostReview uses req.CommitID as primary, comments as fallback
  5. cmd/review-bot/main.go sets CommitID: pr.Head.SHA on the ReviewRequest
  6. All existing tests pass
  7. New tests cover the commit anchoring path
  8. Code compiles cleanly
## Plan: Thread CommitID through the VCS abstraction layer ## Problem PR #112 added `commitID` support to `gitea.Client.PostReview` at the transport level but never connected it to the `vcs.Client` abstraction. The `vcs.ReviewRequest` struct has no `CommitID` field, so `gitea.Adapter.PostReview` cannot thread a commit anchor through to the underlying Gitea API. Callers using the `vcs.Client` interface (the intended path) silently lose commit anchoring. The GitHub client extracts CommitID from individual `ReviewComment.CommitID` fields, but there's no top-level request-scoped CommitID — meaning body-only reviews (no inline comments) can't be anchored either. ## Constraints - Must not break the existing `vcs.Client` interface contract for the GitHub adapter - `gitea.Client.PostReview` currently takes positional parameters — adding `commitID` as a new positional param changes the signature for all callers - The Gitea API accepts `commit_id` as a top-level field in the CreatePullReview request body - GitHub API accepts `commit_id` as a top-level field in the review creation payload - Both `cmd/review-bot/main.go` (uses `vcs.Client` interface) and `gitea/adapter.go` must work correctly after the change ## Proposed Approach ### 1. Add `CommitID` field to `vcs.ReviewRequest` ```go type ReviewRequest struct { Body string `json:"body"` Event ReviewEvent `json:"event"` CommitID string `json:"commit_id,omitempty"` Comments []ReviewComment `json:"comments,omitempty"` } ``` This is the canonical way to pass a commit anchor through the abstraction layer. ### 2. Update `gitea.Client.PostReview` to accept and send `commitID` Add `commitID string` parameter to the function signature. Include `commit_id` in the JSON payload sent to the Gitea API: ```go func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) ``` Payload struct gains: `CommitID string \x60json:"commit_id,omitempty"\x60` ### 3. Update `gitea.Adapter.PostReview` to pass `req.CommitID` ```go review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, req.CommitID, giteaComments) ``` ### 4. Update `github.Client.PostReview` to prefer `req.CommitID` The GitHub client should use `req.CommitID` as the primary commit anchor. If empty, fall back to extracting from the first comment's `CommitID` (existing behavior). This matches the issue's specification. ### 5. Update `cmd/review-bot/main.go` to set `ReviewRequest.CommitID` ```go reviewReq := vcs.ReviewRequest{ Body: reviewBody, Event: event, CommitID: pr.Head.SHA, Comments: inlineComments, } ``` ### 6. Update tests - `gitea/adapter_test.go`: Verify that `commit_id` appears in the API payload when `ReviewRequest.CommitID` is set - `gitea/client_test.go`: Verify the new parameter is serialized correctly - `github/review_test.go`: Verify `req.CommitID` takes priority over comment-level CommitID ## Error Cases - **Empty CommitID**: Omitted from JSON payload via `omitempty` — API falls back to default behavior (Gitea pins to PR head) - **Invalid CommitID**: API returns an error — bubbles up as-is through the adapter (no special handling needed) - **Conflicting CommitID**: For GitHub, `req.CommitID` takes priority. If `req.CommitID` is empty and comments disagree, existing `ErrConflictingCommitIDs` logic applies (in `github/review.go`) ## Edge Cases - Reviews with no inline comments but with CommitID — must still anchor correctly - Reviews where `req.CommitID` differs from `ReviewComment.CommitID` — req-level wins (it's the intended anchor) - Backward compatibility: existing code that doesn't set `CommitID` continues to work (zero-value string → omitempty → not sent) ## Testing Strategy - Unit tests for `gitea.Client.PostReview` verifying `commit_id` in payload - Unit tests for `gitea.Adapter.PostReview` verifying end-to-end threading - Unit tests for `github.Client.PostReview` verifying req-level priority - Integration test verification that existing `cmd/review-bot/main.go` flow works ## Open Questions - The `github/review.go` file has duplicate type declarations with `github/reviews.go` (both define `reviewResponse`, `dismissReviewRequest`). This appears to be an existing issue — I will NOT attempt to fix it in this PR as it's out of scope. However, it may cause compilation errors that I'll need to investigate. - Should `ReviewComment.CommitID` be deprecated now that we have request-level `CommitID`? I think no — per-comment CommitID is still useful for the GitHub adapter's fallback behavior and for mixed-commit scenarios. But the adapter comment in `gitea/adapter.go` (line 170-172) that says "intentionally not forwarded" should be updated to reference the request-level field. ## Completion Checklist 1. `vcs.ReviewRequest` has `CommitID string` field with correct JSON tag 2. `gitea.Client.PostReview` includes `commit_id` in API payload when non-empty 3. `gitea.Adapter.PostReview` passes `req.CommitID` to underlying client 4. `github.Client.PostReview` uses `req.CommitID` as primary, comments as fallback 5. `cmd/review-bot/main.go` sets `CommitID: pr.Head.SHA` on the ReviewRequest 6. All existing tests pass 7. New tests cover the commit anchoring path 8. Code compiles cleanly
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label ai-review bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees rodin
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#114
Delete Branch "%!s()"

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?