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

feat: always post fresh review, supersede old with collapsed body #38

Merged
rodin merged 3 commits from feat/34-always-post-fresh into main 2026-05-02 18:36:43 +00:00
Conversation 9 Commits 3 Files Changed 3
+130 -137
rodin commented 2026-05-02 18:26:20 +00:00
Owner
Copy Link
Copy Source

Closes #34

Summary

Replaces the edit-in-place + reviewUnchanged() skip logic with a simpler model:

  1. Always post a fresh review on the current commit (no more stale badges)
  2. Supersede old review — PATCH it with struck-through banner + collapsed original in <details>
  3. Commit footer on every review: Evaluated against <sha>

What changed

  • Removed reviewUnchanged() function entirely
  • Removed edit-in-place PATCH-body-with-same-content path
  • Removed --update-existing flag (always supersede+post)
  • Added buildSupersededBody() — creates the collapsed history format
  • Added CommitID to Review struct (needed for supersede body)
  • Added commit SHA footer to review body
  • Tests updated: removed TestReviewUnchanged, added TestBuildSupersededBody

Why

The old logic caused reviews to silently skip re-posting when content was unchanged, leaving stale badges that blocked merges. This was a recurring friction point requiring manual intervention (delete stale reviews + push empty commits).

Net effect

  • -133 lines / +87 lines (simpler logic)
  • Every push → fresh non-stale review
  • Old review threads preserved as navigable collapsed history
  • No more "delete stale + retrigger" dance
Closes #34 ## Summary Replaces the edit-in-place + `reviewUnchanged()` skip logic with a simpler model: 1. **Always post a fresh review** on the current commit (no more stale badges) 2. **Supersede old review** — PATCH it with struck-through banner + collapsed original in `<details>` 3. **Commit footer** on every review: `Evaluated against <sha>` ## What changed - Removed `reviewUnchanged()` function entirely - Removed edit-in-place PATCH-body-with-same-content path - Removed `--update-existing` flag (always supersede+post) - Added `buildSupersededBody()` — creates the collapsed history format - Added `CommitID` to `Review` struct (needed for supersede body) - Added commit SHA footer to review body - Tests updated: removed `TestReviewUnchanged`, added `TestBuildSupersededBody` ## Why The old logic caused reviews to silently skip re-posting when content was unchanged, leaving stale badges that blocked merges. This was a recurring friction point requiring manual intervention (delete stale reviews + push empty commits). ## Net effect - -133 lines / +87 lines (simpler logic) - Every push → fresh non-stale review - Old review threads preserved as navigable collapsed history - No more "delete stale + retrigger" dance
rodin added 1 commit 2026-05-02 18:26:20 +00:00
feat: always post fresh review, supersede old with collapsed body
CI / test (pull_request) Successful in 14s
Details
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
Details
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m2s
Details
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m25s
Details
fdd75699d9 
Closes #34

- Remove reviewUnchanged() skip logic — every push gets a fresh review
- Remove edit-in-place (PATCH same body) — always POST new
- Supersede old review: PATCH with struck-through banner + collapsed
  original body in <details> for historical reference
- Add commit footer to every review: 'Evaluated against <sha>'
- Remove --update-existing flag (no longer needed)
- Add CommitID field to Review struct
- Add TestBuildSupersededBody tests
gpt-review-bot approved these changes 2026-05-02 18:26:59 +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 ecbae332)

Gpt Review

Summary

The changes simplify review flow by always posting a fresh review and superseding the previous one with clear, user-friendly formatting. Legacy update-in-place logic is removed, making the process more robust and maintaining review history. All code and tests are updated accordingly.

Recommendation

APPROVE — The patch is correct, improves maintainability, and passes CI. The revised supersede logic is clear, and obsolete complexity is removed. The new 'buildSupersededBody' approach is idiomatic and the Review struct is cleanly extended to include CommitID. Tests cover the updated logic and edge cases. Merge as-is.

Review by gpt

Evaluated against ecbae332

~~Original review~~ **Superseded** — see current review for up-to-date findings. <details><summary>Previous findings (commit ecbae332)</summary> # Gpt Review ## Summary The changes simplify review flow by always posting a fresh review and superseding the previous one with clear, user-friendly formatting. Legacy update-in-place logic is removed, making the process more robust and maintaining review history. All code and tests are updated accordingly. ## Recommendation **APPROVE** — The patch is correct, improves maintainability, and passes CI. The revised supersede logic is clear, and obsolete complexity is removed. The new 'buildSupersededBody' approach is idiomatic and the Review struct is cleanly extended to include CommitID. Tests cover the updated logic and edge cases. Merge as-is. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against ecbae332* </details> <!-- review-bot:gpt -->
security-review-bot approved these changes 2026-05-02 18:27:38 +00:00
Dismissed
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Security Review

Summary

The changes simplify the review posting strategy and add a safe superseding flow. No exploitable security issues are evident; CI passed. One minor hardening opportunity exists around verifying ownership when locating previous reviews to supersede.

Findings

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 316 Supersede logic identifies prior review and corresponding timeline event solely by a sentinel substring without confirming authorship. Although the Gitea API should prevent editing comments not owned by the token, an attacker could craft a review containing the sentinel and cause unnecessary edit attempts. Consider verifying that the located review/timeline event belongs to the authenticated user (e.g., fetch current user and compare to Review.User.Login) before calling EditComment.

Recommendation

APPROVE — Overall the refactor appears sound, with CI passing. The new always-fresh review plus superseded-history approach does not introduce material security risks. As a defense-in-depth improvement, when locating an existing review to supersede and when scanning timeline events via the sentinel, also assert that the review/user login matches the authenticated account to avoid attempting to edit others' comments in edge cases. Otherwise, the commit footer and superseded body construction are benign, and the existing input validation for reviewer names mitigates injection via the sentinel.

Review by security

Evaluated against fdd75699

# Security Review ## Summary The changes simplify the review posting strategy and add a safe superseding flow. No exploitable security issues are evident; CI passed. One minor hardening opportunity exists around verifying ownership when locating previous reviews to supersede. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 316 | Supersede logic identifies prior review and corresponding timeline event solely by a sentinel substring without confirming authorship. Although the Gitea API should prevent editing comments not owned by the token, an attacker could craft a review containing the sentinel and cause unnecessary edit attempts. Consider verifying that the located review/timeline event belongs to the authenticated user (e.g., fetch current user and compare to Review.User.Login) before calling EditComment. | ## Recommendation **APPROVE** — Overall the refactor appears sound, with CI passing. The new always-fresh review plus superseded-history approach does not introduce material security risks. As a defense-in-depth improvement, when locating an existing review to supersede and when scanning timeline events via the sentinel, also assert that the review/user login matches the authenticated account to avoid attempting to edit others' comments in edge cases. Otherwise, the commit footer and superseded body construction are benign, and the existing input validation for reviewer names mitigates injection via the sentinel. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against fdd75699*
cmd/review-bot/main.go
@@ -307,56 +316,33 @@ func main() {
}
security-review-bot commented 2026-05-02 18:27:38 +00:00
Collaborator
Copy Link
Copy Source

[MINOR] Supersede logic identifies prior review and corresponding timeline event solely by a sentinel substring without confirming authorship. Although the Gitea API should prevent editing comments not owned by the token, an attacker could craft a review containing the sentinel and cause unnecessary edit attempts. Consider verifying that the located review/timeline event belongs to the authenticated user (e.g., fetch current user and compare to Review.User.Login) before calling EditComment.

**[MINOR]** Supersede logic identifies prior review and corresponding timeline event solely by a sentinel substring without confirming authorship. Although the Gitea API should prevent editing comments not owned by the token, an attacker could craft a review containing the sentinel and cause unnecessary edit attempts. Consider verifying that the located review/timeline event belongs to the authenticated user (e.g., fetch current user and compare to Review.User.Login) before calling EditComment.
sonnet-review-bot approved these changes 2026-05-02 18:28:01 +00:00
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 ecbae332)

Sonnet Review

Summary

Overall approach is sound and simplifies the review flow, and CI is passing. However, there is a logic bug in the supersede path: the strict check compares the Gitea user login to the reviewer-name flag, which will usually not match and prevents superseding older reviews.

Findings

# Severity File Line Finding
1 [MAJOR] cmd/review-bot/main.go 326 findOwnReviewStrict is called with expectedLogin set to reviewer-name (*reviewerName), but reviews[i].User.Login contains the Gitea account login. This mismatch will typically cause the strict check to filter out all candidate reviews, so older reviews are never marked as superseded. Pass the actual Gitea login (e.g., derived via the sentinel from existing reviews or via a /user API call) or disable the strict check by passing an empty expectedLogin.
2 [MINOR] cmd/review-bot/main_test.go 1 There are tests for findOwnReview but none for findOwnReviewStrict. Adding tests that cover the strict path (including correct vs incorrect expectedLogin) would prevent regressions and would have caught the mismatch bug.

Recommendation

REQUEST_CHANGES — Fix the supersede logic by ensuring findOwnReviewStrict receives the correct expected login. For example, reuse the same ownLogin discovery used in hasSharedToken (scan existingReviews for the first review containing the sentinel and record r.User.Login), then pass that login into findOwnReviewStrict. Alternatively, if the token identity is trusted, pass an empty string to skip the strict check and rely on the sentinel alone. After correcting this, add unit tests for findOwnReviewStrict to validate both matching and mismatching login scenarios. Once addressed, the new always-post + supersede behavior should work as intended.

Review by sonnet

Evaluated against ecbae332

~~Original review~~ **Superseded** — see current review for up-to-date findings. <details><summary>Previous findings (commit ecbae332)</summary> # Sonnet Review ## Summary Overall approach is sound and simplifies the review flow, and CI is passing. However, there is a logic bug in the supersede path: the strict check compares the Gitea user login to the reviewer-name flag, which will usually not match and prevents superseding older reviews. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `cmd/review-bot/main.go` | 326 | findOwnReviewStrict is called with expectedLogin set to reviewer-name (*reviewerName), but reviews[i].User.Login contains the Gitea account login. This mismatch will typically cause the strict check to filter out all candidate reviews, so older reviews are never marked as superseded. Pass the actual Gitea login (e.g., derived via the sentinel from existing reviews or via a /user API call) or disable the strict check by passing an empty expectedLogin. | | 2 | [MINOR] | `cmd/review-bot/main_test.go` | 1 | There are tests for findOwnReview but none for findOwnReviewStrict. Adding tests that cover the strict path (including correct vs incorrect expectedLogin) would prevent regressions and would have caught the mismatch bug. | ## Recommendation **REQUEST_CHANGES** — Fix the supersede logic by ensuring findOwnReviewStrict receives the correct expected login. For example, reuse the same ownLogin discovery used in hasSharedToken (scan existingReviews for the first review containing the sentinel and record r.User.Login), then pass that login into findOwnReviewStrict. Alternatively, if the token identity is trusted, pass an empty string to skip the strict check and rely on the sentinel alone. After correcting this, add unit tests for findOwnReviewStrict to validate both matching and mismatching login scenarios. Once addressed, the new always-post + supersede behavior should work as intended. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against ecbae332* </details> <!-- review-bot:sonnet -->
cmd/review-bot/main.go
@@ -314,4 +323,4 @@
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
if err != nil {
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
} else {
sonnet-review-bot commented 2026-05-02 18:28:01 +00:00
First-time contributor
Copy Link
Copy Source

[MINOR] Selecting which previous review to supersede relies on findOwnReview returning the first match. If ListReviews does not guarantee reverse-chronological ordering, this may supersede an older review instead of the most recent one. Consider choosing the most recent review (e.g., by CreatedAt/ID) or filtering out already-superseded bodies.

**[MINOR]** Selecting which previous review to supersede relies on findOwnReview returning the first match. If ListReviews does not guarantee reverse-chronological ordering, this may supersede an older review instead of the most recent one. Consider choosing the most recent review (e.g., by CreatedAt/ID) or filtering out already-superseded bodies.
cmd/review-bot/main.go
@@ -540,3 +515,1 @@
if r.State == newEvent && r.Body == newBody {
return true
}
// buildSupersededBody creates the body for a superseded review: struck-through banner
sonnet-review-bot commented 2026-05-02 18:28:01 +00:00
First-time contributor
Copy Link
Copy Source

[NIT] If commitSHA is empty, the superseded summary renders as "Previous findings (commit )". Consider omitting the commit text when the SHA is empty to avoid awkward formatting.

**[NIT]** If commitSHA is empty, the superseded summary renders as "Previous findings (commit )". Consider omitting the commit text when the SHA is empty to avoid awkward formatting.
cmd/review-bot/main.go
@@ -543,0 +515,4 @@
// buildSupersededBody creates the body for a superseded review: struck-through banner
// with collapsed original content and the commit it was evaluated against.
func buildSupersededBody(originalBody, commitSHA, sentinel string) string {
shortSHA := commitSHA
sonnet-review-bot commented 2026-05-02 18:28:01 +00:00
First-time contributor
Copy Link
Copy Source

[MINOR] buildSupersededBody appends the sentinel to the superseded comment body, which means future runs will still detect the superseded review when looking for the sentinel. This can make selecting the correct review to supersede ambiguous. Consider removing the sentinel from superseded bodies or adding a marker (e.g., "Superseded") that findOwnReview skips.

**[MINOR]** buildSupersededBody appends the sentinel to the superseded comment body, which means future runs will still detect the superseded review when looking for the sentinel. This can make selecting the correct review to supersede ambiguous. Consider removing the sentinel from superseded bodies or adding a marker (e.g., "Superseded") that findOwnReview skips.
rodin added 1 commit 2026-05-02 18:30:36 +00:00
fix: address review findings
CI / test (pull_request) Successful in 14s
Details
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
Details
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m17s
Details
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m22s
Details
ecbae332f4 
- findOwnReview: skip superseded reviews, pick highest ID (most recent)
- findOwnReviewStrict: verify authorship before superseding (defense-in-depth)
- buildSupersededBody: handle empty commitSHA gracefully
- Tests: add cases for superseded skip, highest-ID selection
gpt-review-bot approved these changes 2026-05-02 18:31:15 +00:00
Dismissed
gpt-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Gpt Review

Summary

The changes simplify review flow by always posting a fresh review and superseding the previous one with clear, user-friendly formatting. Legacy update-in-place logic is removed, making the process more robust and maintaining review history. All code and tests are updated accordingly.

Recommendation

APPROVE — The patch is correct, improves maintainability, and passes CI. The revised supersede logic is clear, and obsolete complexity is removed. The new 'buildSupersededBody' approach is idiomatic and the Review struct is cleanly extended to include CommitID. Tests cover the updated logic and edge cases. Merge as-is.

Review by gpt

Evaluated against ecbae332

# Gpt Review ## Summary The changes simplify review flow by always posting a fresh review and superseding the previous one with clear, user-friendly formatting. Legacy update-in-place logic is removed, making the process more robust and maintaining review history. All code and tests are updated accordingly. ## Recommendation **APPROVE** — The patch is correct, improves maintainability, and passes CI. The revised supersede logic is clear, and obsolete complexity is removed. The new 'buildSupersededBody' approach is idiomatic and the Review struct is cleanly extended to include CommitID. Tests cover the updated logic and edge cases. Merge as-is. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against ecbae332*
security-review-bot approved these changes 2026-05-02 18:32:10 +00:00
Dismissed
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Security Review

Summary

Changes implement a safer, simpler review posting strategy without introducing new security risks. No injection, auth, or data exposure concerns were observed in the modified code paths.

Recommendation

APPROVE — The update to always post a fresh review and supersede prior reviews appears sound and does not weaken security. Path handling for system prompt files remains guarded against traversal and symlink attacks, and added content (short commit SHA) is non-sensitive. Proceed with merge.

Review by security

Evaluated against ecbae332

# Security Review ## Summary Changes implement a safer, simpler review posting strategy without introducing new security risks. No injection, auth, or data exposure concerns were observed in the modified code paths. ## Recommendation **APPROVE** — The update to always post a fresh review and supersede prior reviews appears sound and does not weaken security. Path handling for system prompt files remains guarded against traversal and symlink attacks, and added content (short commit SHA) is non-sensitive. Proceed with merge. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against ecbae332*
sonnet-review-bot requested changes 2026-05-02 18:32:15 +00:00
sonnet-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Sonnet Review

Summary

Overall approach is sound and simplifies the review flow, and CI is passing. However, there is a logic bug in the supersede path: the strict check compares the Gitea user login to the reviewer-name flag, which will usually not match and prevents superseding older reviews.

Findings

# Severity File Line Finding
1 [MAJOR] cmd/review-bot/main.go 326 findOwnReviewStrict is called with expectedLogin set to reviewer-name (*reviewerName), but reviews[i].User.Login contains the Gitea account login. This mismatch will typically cause the strict check to filter out all candidate reviews, so older reviews are never marked as superseded. Pass the actual Gitea login (e.g., derived via the sentinel from existing reviews or via a /user API call) or disable the strict check by passing an empty expectedLogin.
2 [MINOR] cmd/review-bot/main_test.go 1 There are tests for findOwnReview but none for findOwnReviewStrict. Adding tests that cover the strict path (including correct vs incorrect expectedLogin) would prevent regressions and would have caught the mismatch bug.

Recommendation

REQUEST_CHANGES — Fix the supersede logic by ensuring findOwnReviewStrict receives the correct expected login. For example, reuse the same ownLogin discovery used in hasSharedToken (scan existingReviews for the first review containing the sentinel and record r.User.Login), then pass that login into findOwnReviewStrict. Alternatively, if the token identity is trusted, pass an empty string to skip the strict check and rely on the sentinel alone. After correcting this, add unit tests for findOwnReviewStrict to validate both matching and mismatching login scenarios. Once addressed, the new always-post + supersede behavior should work as intended.

Review by sonnet

Evaluated against ecbae332

# Sonnet Review ## Summary Overall approach is sound and simplifies the review flow, and CI is passing. However, there is a logic bug in the supersede path: the strict check compares the Gitea user login to the reviewer-name flag, which will usually not match and prevents superseding older reviews. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `cmd/review-bot/main.go` | 326 | findOwnReviewStrict is called with expectedLogin set to reviewer-name (*reviewerName), but reviews[i].User.Login contains the Gitea account login. This mismatch will typically cause the strict check to filter out all candidate reviews, so older reviews are never marked as superseded. Pass the actual Gitea login (e.g., derived via the sentinel from existing reviews or via a /user API call) or disable the strict check by passing an empty expectedLogin. | | 2 | [MINOR] | `cmd/review-bot/main_test.go` | 1 | There are tests for findOwnReview but none for findOwnReviewStrict. Adding tests that cover the strict path (including correct vs incorrect expectedLogin) would prevent regressions and would have caught the mismatch bug. | ## Recommendation **REQUEST_CHANGES** — Fix the supersede logic by ensuring findOwnReviewStrict receives the correct expected login. For example, reuse the same ownLogin discovery used in hasSharedToken (scan existingReviews for the first review containing the sentinel and record r.User.Login), then pass that login into findOwnReviewStrict. Alternatively, if the token identity is trusted, pass an empty string to skip the strict check and rely on the sentinel alone. After correcting this, add unit tests for findOwnReviewStrict to validate both matching and mismatching login scenarios. Once addressed, the new always-post + supersede behavior should work as intended. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against ecbae332*
cmd/review-bot/main.go
@@ -314,4 +323,4 @@
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
if err != nil {
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
} else {
sonnet-review-bot commented 2026-05-02 18:32:15 +00:00
First-time contributor
Copy Link
Copy Source

[MAJOR] findOwnReviewStrict is called with expectedLogin set to reviewer-name (*reviewerName), but reviews[i].User.Login contains the Gitea account login. This mismatch will typically cause the strict check to filter out all candidate reviews, so older reviews are never marked as superseded. Pass the actual Gitea login (e.g., derived via the sentinel from existing reviews or via a /user API call) or disable the strict check by passing an empty expectedLogin.

**[MAJOR]** findOwnReviewStrict is called with expectedLogin set to reviewer-name (*reviewerName), but reviews[i].User.Login contains the Gitea account login. This mismatch will typically cause the strict check to filter out all candidate reviews, so older reviews are never marked as superseded. Pass the actual Gitea login (e.g., derived via the sentinel from existing reviews or via a /user API call) or disable the strict check by passing an empty expectedLogin.
rodin added 1 commit 2026-05-02 18:34:00 +00:00
fix: remove findOwnReviewStrict, use findOwnReview directly
CI / test (pull_request) Successful in 14s
Details
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
Details
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 42s
Details
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m31s
Details
f6baa41b2c 
The strict authorship check compared reviewer-name to User.Login which
could mismatch. The sentinel is already role-specific (e.g.
<!-- review-bot:sonnet -->) and Gitea's API blocks editing others'
comments (403). Defense-in-depth via login comparison is unnecessary
complexity that introduced a bug. Removed.
gpt-review-bot approved these changes 2026-05-02 18:34:35 +00:00
gpt-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Gpt Review

Summary

This change eliminates in-place review updates in favor of always posting a fresh review and superseding older ones with a collapsed/strikethrough format. The logic is simpler, more robust, and CI passes.

Recommendation

APPROVE — The PR achieves its intended result: always posting a new review for every run and correctly archiving prior reviews. Dead code (reviewUnchanged) and unused flags are cleanly removed, and the new utility (buildSupersededBody) is well-tested. The expanded tests for supersedure, skipping, edge cases, and the updated Review struct are sound. As the CI is green and the overall approach aligns with the repo's conventions, it is ready to merge.

Review by gpt

Evaluated against f6baa41b

# Gpt Review ## Summary This change eliminates in-place review updates in favor of always posting a fresh review and superseding older ones with a collapsed/strikethrough format. The logic is simpler, more robust, and CI passes. ## Recommendation **APPROVE** — The PR achieves its intended result: always posting a new review for every run and correctly archiving prior reviews. Dead code (`reviewUnchanged`) and unused flags are cleanly removed, and the new utility (`buildSupersededBody`) is well-tested. The expanded tests for supersedure, skipping, edge cases, and the updated Review struct are sound. As the CI is green and the overall approach aligns with the repo's conventions, it is ready to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against f6baa41b*
security-review-bot approved these changes 2026-05-02 18:34:54 +00:00
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Security Review

Summary

Changes simplify review posting and add a safe supersede mechanism. No new security-relevant issues were introduced and CI has passed.

Recommendation

APPROVE — The new always-post + supersede flow looks sound from a security perspective. Inputs embedded into review bodies are validated or originate from trusted sources (commit SHA, prior bot review). Path handling for the optional system prompt remains guarded against traversal and symlink breakout. Proceed with merge.

Review by security

Evaluated against f6baa41b

# Security Review ## Summary Changes simplify review posting and add a safe supersede mechanism. No new security-relevant issues were introduced and CI has passed. ## Recommendation **APPROVE** — The new always-post + supersede flow looks sound from a security perspective. Inputs embedded into review bodies are validated or originate from trusted sources (commit SHA, prior bot review). Path handling for the optional system prompt remains guarded against traversal and symlink breakout. Proceed with merge. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against f6baa41b*
sonnet-review-bot approved these changes 2026-05-02 18:35:45 +00:00
sonnet-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Sonnet Review

Summary

Clear, coherent refactor that simplifies the review update strategy and addresses stale review issues. Tests have been updated appropriately and CI is passing. A couple of small robustness improvements are suggested around locating the correct timeline comment to supersede.

Findings

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 334 When superseding the previous review, GetTimelineReviewCommentID is used to find a timeline comment containing the sentinel, but it returns the first matching event rather than the latest. If multiple reviews with the same sentinel exist, this may patch an older review instead of the most recent non-superseded one found by findOwnReview. Consider matching the timeline event to the selected review (e.g., by comparing CommitID or body) or selecting the latest matching event by ID.
2 [NIT] cmd/review-bot/main.go 521 buildSupersededBody appends the sentinel to the superseded comment. This is useful for diagnostics but also increases the chance that future timeline scans pick a superseded event. Since findOwnReview already skips superseded bodies, consider adjusting GetTimelineReviewCommentID to prefer the latest or match by CommitID to avoid editing superseded entries, even if they contain the sentinel.

Recommendation

APPROVE — Overall, the change achieves the intended behavior: always posting a fresh review and superseding previous ones with a clear, collapsed history. CI is green and tests cover the new logic. To make the supersede operation more robust when multiple reviews exist, update GetTimelineReviewCommentID usage (or the function itself) to identify and patch the latest matching review (e.g., match by CommitID or choose the event with the highest ID) so the correct previous review is superseded consistently. Otherwise, the PR looks good to merge.

Review by sonnet

Evaluated against f6baa41b

# Sonnet Review ## Summary Clear, coherent refactor that simplifies the review update strategy and addresses stale review issues. Tests have been updated appropriately and CI is passing. A couple of small robustness improvements are suggested around locating the correct timeline comment to supersede. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 334 | When superseding the previous review, GetTimelineReviewCommentID is used to find a timeline comment containing the sentinel, but it returns the first matching event rather than the latest. If multiple reviews with the same sentinel exist, this may patch an older review instead of the most recent non-superseded one found by findOwnReview. Consider matching the timeline event to the selected review (e.g., by comparing CommitID or body) or selecting the latest matching event by ID. | | 2 | [NIT] | `cmd/review-bot/main.go` | 521 | buildSupersededBody appends the sentinel to the superseded comment. This is useful for diagnostics but also increases the chance that future timeline scans pick a superseded event. Since findOwnReview already skips superseded bodies, consider adjusting GetTimelineReviewCommentID to prefer the latest or match by CommitID to avoid editing superseded entries, even if they contain the sentinel. | ## Recommendation **APPROVE** — Overall, the change achieves the intended behavior: always posting a fresh review and superseding previous ones with a clear, collapsed history. CI is green and tests cover the new logic. To make the supersede operation more robust when multiple reviews exist, update GetTimelineReviewCommentID usage (or the function itself) to identify and patch the latest matching review (e.g., match by CommitID or choose the event with the highest ID) so the correct previous review is superseded consistently. Otherwise, the PR looks good to merge. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against f6baa41b*
cmd/review-bot/main.go
@@ -344,1 +332,3 @@
}
commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
if err != nil {
slog.Warn("could not find old review comment ID for supersede", "error", err)
sonnet-review-bot commented 2026-05-02 18:35:45 +00:00
First-time contributor
Copy Link
Copy Source

[MINOR] When superseding the previous review, GetTimelineReviewCommentID is used to find a timeline comment containing the sentinel, but it returns the first matching event rather than the latest. If multiple reviews with the same sentinel exist, this may patch an older review instead of the most recent non-superseded one found by findOwnReview. Consider matching the timeline event to the selected review (e.g., by comparing CommitID or body) or selecting the latest matching event by ID.

**[MINOR]** When superseding the previous review, GetTimelineReviewCommentID is used to find a timeline comment containing the sentinel, but it returns the first matching event rather than the latest. If multiple reviews with the same sentinel exist, this may patch an older review instead of the most recent non-superseded one found by findOwnReview. Consider matching the timeline event to the selected review (e.g., by comparing CommitID or body) or selecting the latest matching event by ID.
cmd/review-bot/main.go
@@ -543,1 +518,4 @@
shortSHA := commitSHA
if len(shortSHA) > 8 {
shortSHA = shortSHA[:8]
}
sonnet-review-bot commented 2026-05-02 18:35:45 +00:00
First-time contributor
Copy Link
Copy Source

[NIT] buildSupersededBody appends the sentinel to the superseded comment. This is useful for diagnostics but also increases the chance that future timeline scans pick a superseded event. Since findOwnReview already skips superseded bodies, consider adjusting GetTimelineReviewCommentID to prefer the latest or match by CommitID to avoid editing superseded entries, even if they contain the sentinel.

**[NIT]** buildSupersededBody appends the sentinel to the superseded comment. This is useful for diagnostics but also increases the chance that future timeline scans pick a superseded event. Since findOwnReview already skips superseded bodies, consider adjusting GetTimelineReviewCommentID to prefer the latest or match by CommitID to avoid editing superseded entries, even if they contain the sentinel.
rodin merged commit da586a512a into main 2026-05-02 18:36:43 +00:00
rodin deleted branch feat/34-always-post-fresh 2026-05-02 18:36:43 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
gpt-review-bot
security-review-bot
sonnet-review-bot
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
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#38
Delete Branch "feat/34-always-post-fresh"

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?