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

PostReview: pass CommitID explicitly via ReviewRequest #107

New Issue
Closed
opened 2026-05-13 09:36:16 +00:00 by rodin · 1 comment
rodin commented 2026-05-13 09:36:16 +00:00
Owner
Copy Link
Copy Source

Context

In github/reviews.go, PostReview sources the CommitID for the review payload from the first inline comment that has one. If req.Comments is empty (comment-only review with no inline annotations), payload.CommitID is empty and GitHub defaults to HEAD.

Problem

This implicit behavior works but is fragile:

  • The review is not anchored to the specific commit that was evaluated
  • If the PR head advances between analysis and posting, the review targets a different commit
  • The intent ("review this specific SHA") is not expressed in the API call

Proposed Fix

Add a CommitID string field to vcs.ReviewRequest and pass it explicitly from the caller (which has pr.Head.SHA). The GitHub adapter should use it as the primary source, falling back to comment-derived CommitID only if the field is empty.

References

  • Flagged in PR #106 self-review (Phase 1, finding #2)
  • Low severity but improves correctness guarantees
## Context In `github/reviews.go`, `PostReview` sources the `CommitID` for the review payload from the first inline comment that has one. If `req.Comments` is empty (comment-only review with no inline annotations), `payload.CommitID` is empty and GitHub defaults to HEAD. ## Problem This implicit behavior works but is fragile: - The review is not anchored to the specific commit that was evaluated - If the PR head advances between analysis and posting, the review targets a different commit - The intent ("review this specific SHA") is not expressed in the API call ## Proposed Fix Add a `CommitID string` field to `vcs.ReviewRequest` and pass it explicitly from the caller (which has `pr.Head.SHA`). The GitHub adapter should use it as the primary source, falling back to comment-derived CommitID only if the field is empty. ## References - Flagged in PR #106 self-review (Phase 1, finding #2) - Low severity but improves correctness guarantees
rodin self-assigned this 2026-05-13 17:19:36 +00:00
rodin commented 2026-05-13 17:25:22 +00:00
Author
Owner
Copy Link
Copy Source

Plan: PostReview — pass CommitID explicitly

Problem

gitea.Client.PostReview() does not include a commit_id field in the review payload. The review is not explicitly anchored to the evaluated commit SHA. If the PR head advances between analysis and posting, the review may be associated with the wrong commit. The Gitea API supports commit_id in CreatePullReviewOptions — we just don't pass it.

Constraints

  • Backward-compatible: callers that don't care can pass "" (omitempty drops it from JSON)
  • No new packages — origin/main uses gitea.Client directly (no vcs package yet)
  • All existing tests must continue to pass
  • Must not break the CI action or integration test

Proposed Approach

  1. gitea/client.go — Add commitID string parameter to PostReview

    • Add to the anonymous struct payload: CommitID string \json:"commit_id,omitempty"``
    • Add commitID as a new parameter (after body, before comments)
    • Set payload.CommitID = commitID

  2. cmd/review-bot/main.go — Pass pr.Head.Sha to PostReview

    • Change: giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
    • To: giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
    • evaluatedSHA is already computed from pr.Head.Sha at line ~382

  3. Update all test callers — add "" or appropriate SHA to existing PostReview calls in:

    • gitea/client_test.go
    • gitea/post_review_comments_test.go
    • cmd/review-bot/integration_test.go

  4. Verify commit_id appears in the wire payload — add assertion in existing happy-path test

Error Cases

  • Empty commitID → omitempty drops it from JSON, Gitea defaults to HEAD (backward-compatible)
  • Invalid SHA → Gitea API returns 422; existing error handling surfaces this

Edge Cases

  • Stale check already exists (lines ~380-395) comparing evaluatedSHA vs currentSHA; if HEAD moved, we skip posting entirely. So by the time we reach PostReview, evaluatedSHA is still current.
  • pr.Head.Sha could theoretically be empty (API quirk) — omitempty handles this gracefully

Testing Strategy

  • Existing tests pass with "" commitID (backward compat)
  • Add/modify TestPostReview happy path to verify commit_id appears in the serialized payload
  • Integration test passes with the SHA from a real PR

Open Questions

  • Parameter ordering: commitID between body and comments feels natural (matches JSON field order). Alternative: after comments. Going with before-comments since it's a top-level review property, not comment-level.

Completion Checklist

  1. PostReview signature includes commitID string
  2. commit_id appears in JSON payload when non-empty
  3. commit_id is omitted from JSON when empty (omitempty)
  4. Caller passes evaluatedSHA (which equals pr.Head.Sha)
  5. All existing tests updated and passing
  6. Test asserts commit_id in request body
  7. go build ./... clean
  8. go test ./... green
## Plan: PostReview — pass CommitID explicitly ### Problem `gitea.Client.PostReview()` does not include a `commit_id` field in the review payload. The review is not explicitly anchored to the evaluated commit SHA. If the PR head advances between analysis and posting, the review may be associated with the wrong commit. The Gitea API supports `commit_id` in `CreatePullReviewOptions` — we just don't pass it. ### Constraints - Backward-compatible: callers that don't care can pass `""` (omitempty drops it from JSON) - No new packages — `origin/main` uses `gitea.Client` directly (no `vcs` package yet) - All existing tests must continue to pass - Must not break the CI action or integration test ### Proposed Approach 1. **`gitea/client.go` — Add `commitID string` parameter to `PostReview`** - Add to the anonymous struct payload: `CommitID string \`json:"commit_id,omitempty"\`` - Add `commitID` as a new parameter (after `body`, before `comments`) - Set `payload.CommitID = commitID` 2. **`cmd/review-bot/main.go` — Pass `pr.Head.Sha` to `PostReview`** - Change: `giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)` - To: `giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)` - `evaluatedSHA` is already computed from `pr.Head.Sha` at line ~382 3. **Update all test callers** — add `""` or appropriate SHA to existing `PostReview` calls in: - `gitea/client_test.go` - `gitea/post_review_comments_test.go` - `cmd/review-bot/integration_test.go` 4. **Verify commit_id appears in the wire payload** — add assertion in existing happy-path test ### Error Cases - Empty commitID → `omitempty` drops it from JSON, Gitea defaults to HEAD (backward-compatible) - Invalid SHA → Gitea API returns 422; existing error handling surfaces this ### Edge Cases - Stale check already exists (lines ~380-395) comparing `evaluatedSHA` vs `currentSHA`; if HEAD moved, we skip posting entirely. So by the time we reach `PostReview`, `evaluatedSHA` is still current. - `pr.Head.Sha` could theoretically be empty (API quirk) — `omitempty` handles this gracefully ### Testing Strategy - Existing tests pass with `""` commitID (backward compat) - Add/modify `TestPostReview` happy path to verify `commit_id` appears in the serialized payload - Integration test passes with the SHA from a real PR ### Open Questions - Parameter ordering: `commitID` between `body` and `comments` feels natural (matches JSON field order). Alternative: after `comments`. Going with before-comments since it's a top-level review property, not comment-level. ### Completion Checklist 1. `PostReview` signature includes `commitID string` 2. `commit_id` appears in JSON payload when non-empty 3. `commit_id` is omitted from JSON when empty (omitempty) 4. Caller passes `evaluatedSHA` (which equals `pr.Head.Sha`) 5. All existing tests updated and passing 6. Test asserts `commit_id` in request body 7. `go build ./...` clean 8. `go test ./...` green
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
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#107
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?