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
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
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
cmd/review-bot/main.go — Pass pr.Head.Sha to PostReview
To: giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
evaluatedSHA is already computed from pr.Head.Sha at line ~382
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
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
PostReview signature includes commitID string
commit_id appears in JSON payload when non-empty
commit_id is omitted from JSON when empty (omitempty)
## 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
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Context
In
github/reviews.go,PostReviewsources theCommitIDfor the review payload from the first inline comment that has one. Ifreq.Commentsis empty (comment-only review with no inline annotations),payload.CommitIDis empty and GitHub defaults to HEAD.Problem
This implicit behavior works but is fragile:
Proposed Fix
Add a
CommitID stringfield tovcs.ReviewRequestand pass it explicitly from the caller (which haspr.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
Plan: PostReview — pass CommitID explicitly
Problem
gitea.Client.PostReview()does not include acommit_idfield 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 supportscommit_idinCreatePullReviewOptions— we just don't pass it.Constraints
""(omitempty drops it from JSON)origin/mainusesgitea.Clientdirectly (novcspackage yet)Proposed Approach
gitea/client.go— AddcommitID stringparameter toPostReviewCommitID string \json:"commit_id,omitempty"``commitIDas a new parameter (afterbody, beforecomments)payload.CommitID = commitIDcmd/review-bot/main.go— Passpr.Head.ShatoPostReviewgiteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)evaluatedSHAis already computed frompr.Head.Shaat line ~382Update all test callers — add
""or appropriate SHA to existingPostReviewcalls in:gitea/client_test.gogitea/post_review_comments_test.gocmd/review-bot/integration_test.goVerify commit_id appears in the wire payload — add assertion in existing happy-path test
Error Cases
omitemptydrops it from JSON, Gitea defaults to HEAD (backward-compatible)Edge Cases
evaluatedSHAvscurrentSHA; if HEAD moved, we skip posting entirely. So by the time we reachPostReview,evaluatedSHAis still current.pr.Head.Shacould theoretically be empty (API quirk) —omitemptyhandles this gracefullyTesting Strategy
""commitID (backward compat)TestPostReviewhappy path to verifycommit_idappears in the serialized payloadOpen Questions
commitIDbetweenbodyandcommentsfeels natural (matches JSON field order). Alternative: aftercomments. Going with before-comments since it's a top-level review property, not comment-level.Completion Checklist
PostReviewsignature includescommitID stringcommit_idappears in JSON payload when non-emptycommit_idis omitted from JSON when empty (omitempty)evaluatedSHA(which equalspr.Head.Sha)commit_idin request bodygo build ./...cleango test ./...green