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

PR #112: vcs.ReviewRequest.CommitID field missing — issue #107's interface-level fix not implemented #115

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

What was missed

Issue #107 explicitly requested adding CommitID string to vcs.ReviewRequest so both platform adapters can thread the commit anchor through the abstraction layer. PR #112 only added commitID as a direct parameter to gitea.Client.PostReview (the low-level Gitea client), not to the shared vcs.ReviewRequest type.

The current vcs.ReviewRequest in vcs/types.go:

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

As a result:

  • github.Client.PostReview still sources CommitID only from req.Comments[i].CommitID (comment-by-comment), not from a top-level field. If a review has no inline comments, commit_id is empty on the GitHub side too.
  • gitea.Adapter.PostReview has no way to pass the commit anchor without this field.

This was the core request in issue #107: "Add a CommitID string field to vcs.ReviewRequest and pass it explicitly from the caller."

Source

  • PR: #112 — feat(gitea): pass commit_id explicitly in PostReview (#107)
  • Linked issue: #107 — PostReview: pass CommitID explicitly via ReviewRequest
  • Acceptance criterion: CommitID string added to vcs.ReviewRequest
  • File: vcs/types.go (ReviewRequest struct)

What needs to happen

  • Add CommitID string \json:"commit_id,omitempty"`tovcs.ReviewRequest`
  • Update gitea.Adapter.PostReview to pass req.CommitID to the underlying client
  • Update github.Client.PostReview to use req.CommitID as the primary commit anchor, falling back to comment-derived CommitID if empty and non-conflicting
  • Update callers of vcs.Client.PostReview (specifically cmd/review-bot/main.go) to populate ReviewRequest.CommitID with evaluatedSHA

References

## What was missed Issue #107 explicitly requested adding `CommitID string` to `vcs.ReviewRequest` so both platform adapters can thread the commit anchor through the abstraction layer. PR #112 only added `commitID` as a direct parameter to `gitea.Client.PostReview` (the low-level Gitea client), not to the shared `vcs.ReviewRequest` type. The current `vcs.ReviewRequest` in `vcs/types.go`: ```go type ReviewRequest struct { Body string `json:"body"` Event ReviewEvent `json:"event"` Comments []ReviewComment `json:"comments,omitempty"` // Missing: CommitID string } ``` As a result: - `github.Client.PostReview` still sources `CommitID` only from `req.Comments[i].CommitID` (comment-by-comment), not from a top-level field. If a review has no inline comments, `commit_id` is empty on the GitHub side too. - `gitea.Adapter.PostReview` has no way to pass the commit anchor without this field. This was the core request in issue #107: "Add a `CommitID string` field to `vcs.ReviewRequest` and pass it explicitly from the caller." ## Source - PR: #112 — feat(gitea): pass commit_id explicitly in PostReview (#107) - Linked issue: #107 — PostReview: pass CommitID explicitly via ReviewRequest - Acceptance criterion: `CommitID string` added to `vcs.ReviewRequest` - File: `vcs/types.go` (ReviewRequest struct) ## What needs to happen - Add `CommitID string \`json:"commit_id,omitempty"\`` to `vcs.ReviewRequest` - Update `gitea.Adapter.PostReview` to pass `req.CommitID` to the underlying client - Update `github.Client.PostReview` to use `req.CommitID` as the primary commit anchor, falling back to comment-derived CommitID if empty and non-conflicting - Update callers of `vcs.Client.PostReview` (specifically `cmd/review-bot/main.go`) to populate `ReviewRequest.CommitID` with `evaluatedSHA` ## 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:32 +00:00
rodin self-assigned this 2026-05-13 20:27:02 +00:00
rodin commented 2026-05-13 20:31:19 +00:00
Author
Owner
Copy Link
Copy Source

Plan: Add CommitID to vcs.ReviewRequest (#115)

Problem

PR #112 refactored the review path to use the vcs.ReviewRequest abstraction, but removed the commitID parameter from gitea.Client.PostReview without adding a corresponding CommitID field to vcs.ReviewRequest. This means:

  1. Gitea adapter has no way to pass the commit anchor — the commitID was previously a direct parameter to gitea.Client.PostReview (on origin/main) but got dropped during the refactoring.
  2. GitHub adapter only sources CommitID from req.Comments[i].CommitID — if a review has no inline comments, no commit_id is sent to GitHub either.
  3. Caller (cmd/review-bot/main.go) sets CommitID on each inline comment but doesn't set it at the review level.

Constraints

  • Must not break existing behavior for reviews with inline comments (GitHub still derives commit_id from comments)
  • Must work for both Gitea and GitHub adapters
  • Must be backward compatible (empty CommitID = existing behavior)
  • Branch from origin/feature/github-support (that's where vcs/ package exists)

Proposed Approach

4 changes, all minimal:

  1. vcs/types.go — Add CommitID string field to ReviewRequest:

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

  2. gitea/adapter.go — Pass req.CommitID to the underlying gitea.Client.PostReview. This requires adding the commitID parameter back to gitea.Client.PostReview.

  3. gitea/client.go — Re-add commitID string parameter to PostReview and include it in the JSON payload (matching origin/main's original signature).

  4. github/review.go — Use req.CommitID as primary commit anchor; fall back to comment-derived CommitID if req.CommitID is empty. If both exist and conflict, return error.

  5. cmd/review-bot/main.go — Set ReviewRequest.CommitID = evaluatedSHA (currently pr.Head.SHA) when constructing the review request.

State/Data Model

No new states. CommitID is a pass-through value from caller → adapter → API.

Error Cases

  • req.CommitID is empty: adapters behave as today (Gitea defaults to PR head, GitHub derives from comments or omits)
  • req.CommitID conflicts with a comment's CommitID (GitHub): return ErrConflictingCommitIDs
  • req.CommitID set but no comments: both APIs accept review-level commit_id without comments

Edge Cases

  • Review with no inline comments: req.CommitID is the only source → sent to both APIs
  • Review with comments that each have CommitID matching req.CommitID: no conflict
  • Review with comments that have different CommitID from req.CommitID (GitHub): error
  • Gitea adapter ignores per-comment CommitID (already documented) but passes review-level CommitID

Testing Strategy

  1. Unit test github/review_test.go: Verify req.CommitID is used as primary anchor
  2. Unit test github/review_test.go: Verify fallback to comment CommitID when req.CommitID empty
  3. Unit test github/review_test.go: Verify conflict detection between req.CommitID and comment CommitID
  4. Unit test gitea/adapter_test.go: Verify req.CommitID is forwarded to underlying client
  5. Unit test gitea/client_test.go: Verify commitID appears in JSON payload

Completion Checklist

  1. CommitID string added to vcs.ReviewRequest with json tag commit_id,omitempty?
  2. gitea.Client.PostReview accepts and forwards commitID parameter?
  3. gitea.Adapter.PostReview passes req.CommitID to underlying client?
  4. github.Client.PostReview uses req.CommitID as primary, falls back to comment-derived?
  5. Conflict detection works when req.CommitID != comment CommitID in GitHub adapter?
  6. cmd/review-bot/main.go sets ReviewRequest.CommitID = evaluatedSHA?
  7. All existing tests pass?
  8. New tests cover the added behavior?

Open Questions

  • The Gitea API's CreatePullReview endpoint has a commit_id field at the review level (per Gitea docs). PR #112 removed it from the client; this fix adds it back. Straightforward.
  • Should req.CommitID override comment-level CommitIDs in GitHub, or should they be required to match? I'm going with "required to match (error on conflict)" since that's safest and matches existing behavior for comment-to-comment conflicts.
## Plan: Add CommitID to vcs.ReviewRequest (#115) ### Problem PR #112 refactored the review path to use the `vcs.ReviewRequest` abstraction, but removed the `commitID` parameter from `gitea.Client.PostReview` without adding a corresponding `CommitID` field to `vcs.ReviewRequest`. This means: 1. **Gitea adapter** has no way to pass the commit anchor — the `commitID` was previously a direct parameter to `gitea.Client.PostReview` (on `origin/main`) but got dropped during the refactoring. 2. **GitHub adapter** only sources `CommitID` from `req.Comments[i].CommitID` — if a review has no inline comments, no `commit_id` is sent to GitHub either. 3. **Caller** (`cmd/review-bot/main.go`) sets `CommitID` on each inline comment but doesn't set it at the review level. ### Constraints - Must not break existing behavior for reviews with inline comments (GitHub still derives commit_id from comments) - Must work for both Gitea and GitHub adapters - Must be backward compatible (empty CommitID = existing behavior) - Branch from `origin/feature/github-support` (that's where `vcs/` package exists) ### Proposed Approach **4 changes, all minimal:** 1. **`vcs/types.go`** — Add `CommitID string` field to `ReviewRequest`: ```go type ReviewRequest struct { Body string `json:"body"` Event ReviewEvent `json:"event"` CommitID string `json:"commit_id,omitempty"` Comments []ReviewComment `json:"comments,omitempty"` } ``` 2. **`gitea/adapter.go`** — Pass `req.CommitID` to the underlying `gitea.Client.PostReview`. This requires adding the `commitID` parameter back to `gitea.Client.PostReview`. 3. **`gitea/client.go`** — Re-add `commitID string` parameter to `PostReview` and include it in the JSON payload (matching origin/main's original signature). 4. **`github/review.go`** — Use `req.CommitID` as primary commit anchor; fall back to comment-derived CommitID if `req.CommitID` is empty. If both exist and conflict, return error. 5. **`cmd/review-bot/main.go`** — Set `ReviewRequest.CommitID = evaluatedSHA` (currently `pr.Head.SHA`) when constructing the review request. ### State/Data Model No new states. `CommitID` is a pass-through value from caller → adapter → API. ### Error Cases - `req.CommitID` is empty: adapters behave as today (Gitea defaults to PR head, GitHub derives from comments or omits) - `req.CommitID` conflicts with a comment's CommitID (GitHub): return `ErrConflictingCommitIDs` - `req.CommitID` set but no comments: both APIs accept review-level commit_id without comments ### Edge Cases - Review with no inline comments: `req.CommitID` is the only source → sent to both APIs - Review with comments that each have `CommitID` matching `req.CommitID`: no conflict - Review with comments that have different `CommitID` from `req.CommitID` (GitHub): error - Gitea adapter ignores per-comment CommitID (already documented) but passes review-level CommitID ### Testing Strategy 1. **Unit test `github/review_test.go`**: Verify `req.CommitID` is used as primary anchor 2. **Unit test `github/review_test.go`**: Verify fallback to comment CommitID when `req.CommitID` empty 3. **Unit test `github/review_test.go`**: Verify conflict detection between `req.CommitID` and comment CommitID 4. **Unit test `gitea/adapter_test.go`**: Verify `req.CommitID` is forwarded to underlying client 5. **Unit test `gitea/client_test.go`**: Verify commitID appears in JSON payload ### Completion Checklist 1. `CommitID string` added to `vcs.ReviewRequest` with json tag `commit_id,omitempty`? 2. `gitea.Client.PostReview` accepts and forwards `commitID` parameter? 3. `gitea.Adapter.PostReview` passes `req.CommitID` to underlying client? 4. `github.Client.PostReview` uses `req.CommitID` as primary, falls back to comment-derived? 5. Conflict detection works when `req.CommitID` != comment CommitID in GitHub adapter? 6. `cmd/review-bot/main.go` sets `ReviewRequest.CommitID = evaluatedSHA`? 7. All existing tests pass? 8. New tests cover the added behavior? ### Open Questions - The Gitea API's CreatePullReview endpoint has a `commit_id` field at the review level (per Gitea docs). PR #112 removed it from the client; this fix adds it back. Straightforward. - Should `req.CommitID` override comment-level CommitIDs in GitHub, or should they be required to match? I'm going with "required to match (error on conflict)" since that's safest and matches existing behavior for comment-to-comment conflicts.
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#115
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?