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

feat(gitea): pass commit_id explicitly in PostReview (#107) #112

Merged
aweiker merged 2 commits from review-bot-issue-107 into main 2026-05-13 18:07:39 +00:00
Conversation 11 Commits 2 Files Changed 5
+41 -13
rodin commented 2026-05-13 17:29:20 +00:00
Owner
Copy Link
Copy Source

Summary

Add commitID parameter to gitea.Client.PostReview so the review is anchored to the specific commit SHA that was evaluated, rather than relying on Gitea to default to HEAD.

Changes

  1. gitea/client.go — Added commitID string parameter to PostReview. Included as commit_id,omitempty in the JSON payload so empty values are not sent.
  2. cmd/review-bot/main.go — Pass evaluatedSHA (already computed from pr.Head.Sha) to PostReview.
  3. Tests — Updated all callers, added TestPostReview_EmptyCommitID_OmittedFromPayload to verify omitempty behavior.

Why

Without explicit commit anchoring, if the PR head advances between analysis and posting, the review targets a different commit than what was evaluated. The stale check prevents most races, but explicit anchoring expresses intent clearly in the API call.

Closes #107

## Summary Add `commitID` parameter to `gitea.Client.PostReview` so the review is anchored to the specific commit SHA that was evaluated, rather than relying on Gitea to default to HEAD. ## Changes 1. **`gitea/client.go`** — Added `commitID string` parameter to `PostReview`. Included as `commit_id,omitempty` in the JSON payload so empty values are not sent. 2. **`cmd/review-bot/main.go`** — Pass `evaluatedSHA` (already computed from `pr.Head.Sha`) to `PostReview`. 3. **Tests** — Updated all callers, added `TestPostReview_EmptyCommitID_OmittedFromPayload` to verify omitempty behavior. ## Why Without explicit commit anchoring, if the PR head advances between analysis and posting, the review targets a different commit than what was evaluated. The stale check prevents most races, but explicit anchoring expresses intent clearly in the API call. Closes #107
rodin added the wip label 2026-05-13 17:29:20 +00:00
rodin added 1 commit 2026-05-13 17:29:21 +00:00
feat(gitea): pass commit_id explicitly in PostReview (#107)
CI / test (pull_request) Successful in 18s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 42s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 57s
Details
e59c2bc831 
Add commitID parameter to gitea.Client.PostReview so the review is
anchored to the specific commit that was evaluated. The caller
(cmd/review-bot) already computes evaluatedSHA from pr.Head.Sha;
this wires it through to the Gitea API payload.

When commitID is empty, omitempty drops it from the JSON and Gitea
defaults to the current PR head (backward-compatible).

Closes #107
rodin removed the wip label 2026-05-13 17:29:25 +00:00
sonnet-review-bot approved these changes 2026-05-13 17:30:05 +00:00
Dismissed
sonnet-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Original review

Superseded — see current review for up-to-date findings.

Previous findings (commit e59c2bc8)

Sonnet Review

Summary

A clean, well-scoped change that adds commitID as an explicit parameter to PostReview, passes evaluatedSHA from the call site, and verifies omitempty behaviour with a targeted test. CI passes, all callers updated consistently.

Findings

# Severity File Line Finding
1 [NIT] gitea/client.go 283 The doc comment on PostReview still says event should be "APPROVED" or "REQUEST_CHANGES", but the integration test and production code also pass "COMMENT". The comment is a pre-existing inaccuracy but the PR touches this function's doc block and is a good opportunity to fix it (e.g. event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT").

Recommendation

APPROVE — The change is correct and minimal. The commitID parameter is properly threaded through, omitempty is tested, and CI is green. Approve as-is; the single nit about the doc comment is cosmetic and doesn't affect correctness.

Review by sonnet

Evaluated against e59c2bc8

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/112#pullrequestreview-3341) for up-to-date findings. <details><summary>Previous findings (commit e59c2bc8)</summary> # Sonnet Review ## Summary A clean, well-scoped change that adds `commitID` as an explicit parameter to `PostReview`, passes `evaluatedSHA` from the call site, and verifies `omitempty` behaviour with a targeted test. CI passes, all callers updated consistently. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `gitea/client.go` | 283 | The doc comment on `PostReview` still says `event should be "APPROVED" or "REQUEST_CHANGES"`, but the integration test and production code also pass `"COMMENT"`. The comment is a pre-existing inaccuracy but the PR touches this function's doc block and is a good opportunity to fix it (e.g. `event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT"`). | ## Recommendation **APPROVE** — The change is correct and minimal. The `commitID` parameter is properly threaded through, `omitempty` is tested, and CI is green. Approve as-is; the single nit about the doc comment is cosmetic and doesn't affect correctness. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against e59c2bc8* </details> <!-- review-bot:sonnet -->
gitea/client.go
@@ -277,3 +280,4 @@
CommitID: commitID,
Comments: comments,
}
sonnet-review-bot commented 2026-05-13 17:30:05 +00:00
First-time contributor
Copy Link
Copy Source

[NIT] The doc comment on PostReview still says event should be "APPROVED" or "REQUEST_CHANGES", but the integration test and production code also pass "COMMENT". The comment is a pre-existing inaccuracy but the PR touches this function's doc block and is a good opportunity to fix it (e.g. event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT").

**[NIT]** The doc comment on `PostReview` still says `event should be "APPROVED" or "REQUEST_CHANGES"`, but the integration test and production code also pass `"COMMENT"`. The comment is a pre-existing inaccuracy but the PR touches this function's doc block and is a good opportunity to fix it (e.g. `event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT"`).
security-review-bot requested review from security-review-bot 2026-05-13 17:30:22 +00:00
security-review-bot approved these changes 2026-05-13 17:30:22 +00:00
Dismissed
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Original review

Superseded — see current review for up-to-date findings.

Previous findings (commit e59c2bc8)

Security Review

Summary

Security-focused review of the changes shows no new vulnerabilities introduced. Adding commit_id to PostReview payload anchors reviews to a specific SHA without increasing attack surface.

Recommendation

APPROVE — CI passed and the changes are limited to passing an explicit commit SHA to Gitea via a JSON-encoded field with omitempty semantics. This does not introduce injection, authorization, or other security risks. Proceed with merge.

Review by security

Evaluated against e59c2bc8

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/112#pullrequestreview-3343) for up-to-date findings. <details><summary>Previous findings (commit e59c2bc8)</summary> # Security Review ## Summary Security-focused review of the changes shows no new vulnerabilities introduced. Adding commit_id to PostReview payload anchors reviews to a specific SHA without increasing attack surface. ## Recommendation **APPROVE** — CI passed and the changes are limited to passing an explicit commit SHA to Gitea via a JSON-encoded field with omitempty semantics. This does not introduce injection, authorization, or other security risks. Proceed with merge. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against e59c2bc8* </details> <!-- review-bot:security -->
gpt-review-bot approved these changes 2026-05-13 17:30:37 +00:00
Dismissed
gpt-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Original review

Superseded — see current review for up-to-date findings.

Previous findings (commit e59c2bc8)

Gpt Review

Summary

Changes cleanly add an explicit commitID to PostReview, update all call sites, and include a test to verify omitempty behavior. The implementation is straightforward, preserves error handling conventions, and CI passed.

Recommendation

APPROVE — The explicit commit anchoring via commit_id is a solid improvement for correctness under concurrent PR updates. Call sites and tests are correctly updated, and the omitempty test guards against sending empty values. No issues found in the changes; proceed with merge. As a general note for future work, ensure the event values used align with the Gitea API semantics across all call sites.

Review by gpt

Evaluated against e59c2bc8

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/112#pullrequestreview-3344) for up-to-date findings. <details><summary>Previous findings (commit e59c2bc8)</summary> # Gpt Review ## Summary Changes cleanly add an explicit commitID to PostReview, update all call sites, and include a test to verify omitempty behavior. The implementation is straightforward, preserves error handling conventions, and CI passed. ## Recommendation **APPROVE** — The explicit commit anchoring via commit_id is a solid improvement for correctness under concurrent PR updates. Call sites and tests are correctly updated, and the omitempty test guards against sending empty values. No issues found in the changes; proceed with merge. As a general note for future work, ensure the event values used align with the Gitea API semantics across all call sites. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against e59c2bc8* </details> <!-- review-bot:gpt -->
rodin added the wip label 2026-05-13 17:35:19 +00:00
rodin commented 2026-05-13 17:36:38 +00:00
Author
Owner
Copy Link
Copy Source

Self-Review: PR #112

Self-review against e59c2bc831

Phase 1: Independent Findings

None — diff looks clean. The commitID parameter is correctly threaded through all call sites, the omitempty tag prevents empty values from being serialized, and a dedicated test verifies that behavior. The integration test and main.go call sites are updated consistently.

Phase 2: Prior Review Verification

Finding Reviewer Status Notes
[NIT] Doc comment lists only APPROVED/REQUEST_CHANGES but COMMENT is also used sonnet-review-bot STILL PRESENT Pre-existing inaccuracy; NIT-severity, reviewer approved regardless

Assessment: ✅ Clean

All three bot reviewers approved. The only outstanding item is a cosmetic NIT about a pre-existing doc comment inaccuracy — not introduced by this PR and not blocking. The change is correct, minimal, and well-tested.

## Self-Review: PR #112 Self-review against e59c2bc83162b38c48abfe8789434f68bbb08524 ### Phase 1: Independent Findings None — diff looks clean. The `commitID` parameter is correctly threaded through all call sites, the `omitempty` tag prevents empty values from being serialized, and a dedicated test verifies that behavior. The integration test and main.go call sites are updated consistently. ### Phase 2: Prior Review Verification | Finding | Reviewer | Status | Notes | |---------|----------|--------|-------| | [NIT] Doc comment lists only APPROVED/REQUEST_CHANGES but COMMENT is also used | sonnet-review-bot | STILL PRESENT | Pre-existing inaccuracy; NIT-severity, reviewer approved regardless | ### Assessment: ✅ Clean All three bot reviewers approved. The only outstanding item is a cosmetic NIT about a pre-existing doc comment inaccuracy — not introduced by this PR and not blocking. The change is correct, minimal, and well-tested.
rodin removed the wip label 2026-05-13 17:36:42 +00:00
rodin added the wip label 2026-05-13 17:40:23 +00:00
rodin commented 2026-05-13 17:41:25 +00:00
Author
Owner
Copy Link
Copy Source

Fix Plan against e59c2bc831:

Finding 1: NIT — PostReview doc comment inaccuracy (comment id 19793)

Source: sonnet-review-bot inline comment on gitea/client.go line ~283
Issue: Doc comment on PostReview says event should be "APPROVED" or "REQUEST_CHANGES" but code also uses "COMMENT".
Fix: Update the doc comment to say event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT".

Implementation

  1. Edit gitea/client.go — update the doc comment on PostReview to list all three valid event values.
  2. Run go test ./... to confirm no regressions.
  3. Commit and push to review-bot-issue-107 branch.
  4. Reply to inline comment id 19793 acknowledging the fix.
## Fix Plan against e59c2bc83162b38c48abfe8789434f68bbb08524: ### Finding 1: NIT — PostReview doc comment inaccuracy (comment id 19793) **Source:** sonnet-review-bot inline comment on `gitea/client.go` line ~283 **Issue:** Doc comment on `PostReview` says `event should be "APPROVED" or "REQUEST_CHANGES"` but code also uses `"COMMENT"`. **Fix:** Update the doc comment to say `event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT"`. ### Implementation 1. Edit `gitea/client.go` — update the doc comment on `PostReview` to list all three valid event values. 2. Run `go test ./...` to confirm no regressions. 3. Commit and push to `review-bot-issue-107` branch. 4. Reply to inline comment id 19793 acknowledging the fix.
rodin added 1 commit 2026-05-13 17:41:57 +00:00
fix(gitea): update PostReview doc comment to include COMMENT event value
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 16s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 53s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m4s
Details
028d46942a
rodin commented 2026-05-13 17:42:04 +00:00
Author
Owner
Copy Link
Copy Source

Fixed in 028d469: updated the doc comment to list all three valid event values: APPROVED, REQUEST_CHANGES, and COMMENT.

Fixed in 028d469: updated the doc comment to list all three valid event values: APPROVED, REQUEST_CHANGES, and COMMENT.
rodin removed the wip label 2026-05-13 17:42:08 +00:00
sonnet-review-bot approved these changes 2026-05-13 17:42:40 +00:00
sonnet-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Sonnet Review

Summary

This PR adds a commitID string parameter to PostReview to anchor reviews to a specific commit SHA, using omitempty so empty values are not sent. The implementation is correct, all callers are updated, the new test for omitempty behavior is well-written, and CI is passing.

Findings

# Severity File Line Finding
1 [NIT] gitea/client.go 284 The anonymous struct payload field alignment uses extra whitespace (CommitID string \json:...`vsBody string `json:...``) that is not gofmt-canonical. This is cosmetic but gofmt would normalize it. Likely a non-issue if gofmt is enforced in CI, but worth keeping consistent.

Recommendation

APPROVE — The change is small, correct, and well-tested. The commitID parameter is placed before comments in the signature, which is a reasonable ordering (metadata before optional extras). The omitempty tag correctly handles the empty-string case. CI passes. The single nit about struct field alignment spacing is cosmetic and not a blocker. Approve.

Review by sonnet

Evaluated against 028d4694

# Sonnet Review ## Summary This PR adds a `commitID string` parameter to `PostReview` to anchor reviews to a specific commit SHA, using `omitempty` so empty values are not sent. The implementation is correct, all callers are updated, the new test for omitempty behavior is well-written, and CI is passing. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `gitea/client.go` | 284 | The anonymous struct payload field alignment uses extra whitespace (`CommitID string \`json:...\`` vs `Body string \`json:...\``) that is not gofmt-canonical. This is cosmetic but gofmt would normalize it. Likely a non-issue if gofmt is enforced in CI, but worth keeping consistent. | ## Recommendation **APPROVE** — The change is small, correct, and well-tested. The `commitID` parameter is placed before `comments` in the signature, which is a reasonable ordering (metadata before optional extras). The `omitempty` tag correctly handles the empty-string case. CI passes. The single nit about struct field alignment spacing is cosmetic and not a blocker. Approve. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 028d4694*
security-review-bot requested review from security-review-bot 2026-05-13 17:43:09 +00:00
security-review-bot approved these changes 2026-05-13 17:43:09 +00:00
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Security Review

Summary

Changes cleanly add an explicit commit_id to review submissions and pass the evaluated SHA from the PR, improving correctness without introducing new security risks. CI has passed and the new tests cover the omitempty behavior and updated call sites.

Recommendation

APPROVE — The update anchors reviews to a specific commit via commit_id and is implemented safely with proper JSON marshaling and omission when empty. Calls now pass the previously fetched pr.Head.Sha, and a stale-head check remains in place. No security vulnerabilities are introduced by these changes. As a defense-in-depth enhancement (optional), you could validate commitID format (e.g., hex SHA) before sending, but this is not required for security. Proceed with merge.

Review by security

Evaluated against 028d4694

# Security Review ## Summary Changes cleanly add an explicit commit_id to review submissions and pass the evaluated SHA from the PR, improving correctness without introducing new security risks. CI has passed and the new tests cover the omitempty behavior and updated call sites. ## Recommendation **APPROVE** — The update anchors reviews to a specific commit via commit_id and is implemented safely with proper JSON marshaling and omission when empty. Calls now pass the previously fetched pr.Head.Sha, and a stale-head check remains in place. No security vulnerabilities are introduced by these changes. As a defense-in-depth enhancement (optional), you could validate commitID format (e.g., hex SHA) before sending, but this is not required for security. Proceed with merge. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 028d4694*
gpt-review-bot approved these changes 2026-05-13 17:43:19 +00:00
gpt-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Gpt Review

Summary

Solid, focused change adding an explicit commitID to PostReview and propagating it through callers. Tests were updated comprehensively, including a new check for omitempty behavior, and CI passed.

Recommendation

APPROVE — The explicit commit anchoring via commit_id is a good improvement and is implemented cleanly with proper JSON omitempty semantics. Call sites are correctly updated, including main and integration tests, and additional tests validate new behavior. Given CI success and no issues detected in the diff, this is ready to merge.

Review by gpt

Evaluated against 028d4694

# Gpt Review ## Summary Solid, focused change adding an explicit commitID to PostReview and propagating it through callers. Tests were updated comprehensively, including a new check for omitempty behavior, and CI passed. ## Recommendation **APPROVE** — The explicit commit anchoring via commit_id is a good improvement and is implemented cleanly with proper JSON omitempty semantics. Call sites are correctly updated, including main and integration tests, and additional tests validate new behavior. Given CI success and no issues detected in the diff, this is ready to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 028d4694*
rodin added the wip label 2026-05-13 17:50:02 +00:00
rodin commented 2026-05-13 17:51:23 +00:00
Author
Owner
Copy Link
Copy Source

Self-Review: PR #112

Self-review against 028d46942a

Phase 1: Independent Findings

None — diff looks clean. The commitID string parameter is correctly threaded through all call sites (main.go passes evaluatedSHA, integration test and existing tests pass ""). The omitempty tag correctly prevents serialization of empty strings. The new TestPostReview_EmptyCommitID_OmittedFromPayload test validates this behavior via raw JSON inspection. The doc comment was updated in this PR to include "COMMENT" as a valid event value. No correctness, error-handling, or coverage gaps found.

Phase 2: Prior Review Verification

Finding Reviewer Status Notes
NIT: doc comment missing "COMMENT" as valid event value sonnet-review-bot (review 3325) ✅ Resolved Fixed in 028d469: doc comment now lists all three valid event values (APPROVED, REQUEST_CHANGES, COMMENT)
NIT: struct field alignment spacing not gofmt-canonical sonnet-review-bot (review 3341) 🚫 Invalid The diff shows standard Go struct literal alignment with tab-aligned field tags — this is idiomatic and gofmt-normalized. The reviewer appears to have been misled by rendered whitespace in the diff view; the actual source uses tabs per gofmt convention

Assessment: ✅ Clean

All prior reviewer findings are resolved or invalid. The change is small, focused, and correct: it adds commitID as an explicit parameter to anchor reviews to a specific commit SHA, uses omitempty to avoid sending empty values, updates all callers consistently, and adds a targeted test for the omitempty behavior. Ready to merge.

## Self-Review: PR #112 Self-review against 028d46942af0c605d1057510455ac4de38af31d2 ### Phase 1: Independent Findings None — diff looks clean. The `commitID string` parameter is correctly threaded through all call sites (`main.go` passes `evaluatedSHA`, integration test and existing tests pass `""`). The `omitempty` tag correctly prevents serialization of empty strings. The new `TestPostReview_EmptyCommitID_OmittedFromPayload` test validates this behavior via raw JSON inspection. The doc comment was updated in this PR to include `"COMMENT"` as a valid event value. No correctness, error-handling, or coverage gaps found. ### Phase 2: Prior Review Verification | Finding | Reviewer | Status | Notes | |---------|----------|--------|-------| | NIT: doc comment missing `"COMMENT"` as valid event value | sonnet-review-bot (review 3325) | ✅ Resolved | Fixed in 028d469: doc comment now lists all three valid event values (APPROVED, REQUEST_CHANGES, COMMENT) | | NIT: struct field alignment spacing not gofmt-canonical | sonnet-review-bot (review 3341) | 🚫 Invalid | The diff shows standard Go struct literal alignment with tab-aligned field tags — this is idiomatic and gofmt-normalized. The reviewer appears to have been misled by rendered whitespace in the diff view; the actual source uses tabs per gofmt convention | ### Assessment: ✅ Clean All prior reviewer findings are resolved or invalid. The change is small, focused, and correct: it adds `commitID` as an explicit parameter to anchor reviews to a specific commit SHA, uses `omitempty` to avoid sending empty values, updates all callers consistently, and adds a targeted test for the omitempty behavior. Ready to merge.
rodin removed the wip label 2026-05-13 17:51:26 +00:00
aweiker was assigned by rodin 2026-05-13 17:55:51 +00:00
rodin added the ready label 2026-05-13 17:55:51 +00:00
aweiker approved these changes 2026-05-13 18:07:36 +00:00
aweiker merged commit b26514714f into main 2026-05-13 18:07:39 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
sonnet-review-bot
security-review-bot
gpt-review-bot
aweiker
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 ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees aweiker
5 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#112
Delete Branch "review-bot-issue-107"

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?