feat: always post fresh review, supersede old with collapsed body
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m2s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m25s

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
This commit is contained in:
Rodin
2026-05-02 11:26:06 -07:00
parent dc450f7771
commit fdd75699d9
3 changed files with 88 additions and 134 deletions
+37 -50
View File
@@ -67,7 +67,6 @@ func main() {
patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)") patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)")
patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo") patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo")
dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting") dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
updateExisting := flag.Bool("update-existing", envOrDefaultBool("UPDATE_EXISTING", true), "Delete previous review from same bot before posting (default true)")
llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)") llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)")
llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)") llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)")
llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic") llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic")
@@ -279,6 +278,16 @@ func main() {
// Step 10: Format and post review // Step 10: Format and post review
reviewBody := review.FormatMarkdown(result, *reviewerName) reviewBody := review.FormatMarkdown(result, *reviewerName)
// Add commit footer so readers know which commit was evaluated
if pr.Head.Sha != "" {
shortSHA := pr.Head.Sha
if len(shortSHA) > 8 {
shortSHA = shortSHA[:8]
}
reviewBody += fmt.Sprintf("\n\n---\n*Evaluated against %s*", shortSHA)
}
event := review.GiteaEvent(result.Verdict) event := review.GiteaEvent(result.Verdict)
if *dryRun { if *dryRun {
@@ -307,56 +316,33 @@ func main() {
} }
// --- Review update strategy --- // --- Review update strategy ---
// 1. No existing review → POST new // Always post a fresh review on the current commit. If a previous review
// 2. Existing review, same state → PATCH body in place (preserves threads) // exists, mark it as superseded (preserves old threads as navigable history).
// 3. Existing review, state change → PATCH old to "Superseded", POST new // Never skip posting — the fresh review must land on HEAD to clear stale badges.
if *updateExisting && *reviewerName != "" { if *reviewerName != "" {
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
if err != nil { if err != nil {
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err) slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
} else { } else {
// Detect shared-token misconfiguration: if detected, skip all // In shared-token mode, skip superseding to avoid clobbering sibling reviews.
// update logic (PATCH/supersede) to avoid clobbering a sibling's review.
sharedToken := hasSharedToken(existingReviews, sentinel) sharedToken := hasSharedToken(existingReviews, sentinel)
if sharedToken { if !sharedToken {
slog.Warn("shared token mode: skipping update-in-place logic to avoid clobbering sibling review")
} else {
existing := findOwnReview(existingReviews, sentinel) existing := findOwnReview(existingReviews, sentinel)
if existing != nil { if existing != nil {
if reviewUnchanged(existingReviews, reviewBody, event, sentinel) {
slog.Info("review unchanged from previous run; skipping to preserve threads", "pr", prNumber)
return
}
// Same state → PATCH in place
if existing.State == event {
commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
if err != nil { if err != nil {
slog.Warn("could not find review comment ID, falling back to new post", "error", err) slog.Warn("could not find old review comment ID for supersede", "error", err)
} else { } else {
if err := giteaClient.EditComment(ctx, owner, repoName, commentID, reviewBody); err != nil { supersededBody := buildSupersededBody(existing.Body, existing.CommitID, sentinel)
slog.Warn("could not edit review, falling back to new post", "comment_id", commentID, "error", err)
} else {
slog.Info("review updated in place", "comment_id", commentID, "pr", prNumber)
return
}
}
} else {
// State change → mark old as superseded, post new below
commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
if err != nil {
slog.Warn("could not find old review comment ID", "error", err)
} else {
supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel)
if err := giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody); err != nil { if err := giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody); err != nil {
slog.Warn("could not mark old review as superseded", "comment_id", commentID, "error", err) slog.Warn("could not mark old review as superseded", "comment_id", commentID, "error", err)
} else { } else {
slog.Info("marked old review as superseded", "old_state", existing.State, "new_state", event, "pr", prNumber) slog.Info("marked old review as superseded", "old_state", existing.State, "pr", prNumber)
}
} }
} }
} }
} else {
slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review")
} }
} }
} }
@@ -526,22 +512,23 @@ func validateReviewerName(name string) error {
return nil return nil
} }
// reviewUnchanged checks if an existing review with the same sentinel // buildSupersededBody creates the body for a superseded review: struck-through banner
// already has identical body and state. Returns true if a re-post would // with collapsed original content and the commit it was evaluated against.
// produce the same result (skip to preserve conversation threads). func buildSupersededBody(originalBody, commitSHA, sentinel string) string {
func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string) bool { shortSHA := commitSHA
for _, r := range reviews { if len(shortSHA) > 8 {
if r.Stale { shortSHA = shortSHA[:8]
continue
} }
if !strings.Contains(r.Body, sentinel) { var sb strings.Builder
continue sb.WriteString("~~Original review~~\n\n")
} sb.WriteString("**Superseded** \u2014 see current review for up-to-date findings.\n\n")
if r.State == newEvent && r.Body == newBody { sb.WriteString("<details><summary>Previous findings (commit ")
return true sb.WriteString(shortSHA)
} sb.WriteString(")</summary>\n\n")
} sb.WriteString(originalBody)
return false sb.WriteString("\n\n</details>\n\n")
sb.WriteString(sentinel)
return sb.String()
} }
// hasSharedToken detects if another review-bot role posted under the same // hasSharedToken detects if another review-bot role posted under the same
+40 -74
View File
@@ -56,82 +56,48 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
return r return r
} }
func TestReviewUnchanged(t *testing.T) {
tests := []struct { func TestBuildSupersededBody(t *testing.T) {
name string original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
existing []gitea.Review sentinel := "<!-- review-bot:sonnet -->"
newBody string
newEvent string result := buildSupersededBody(original, "abcdef1234567890", sentinel)
sentinel string
want bool // Should contain the struck-through banner
}{ if !strings.Contains(result, "~~Original review~~") {
{ t.Error("missing struck-through banner")
name: "no existing review", }
existing: nil, // Should contain superseded notice
newBody: "new review", if !strings.Contains(result, "**Superseded**") {
newEvent: "APPROVED", t.Error("missing superseded notice")
sentinel: "<!-- review-bot:sonnet -->", }
want: false, // Should contain collapsed original
}, if !strings.Contains(result, "<details>") {
{ t.Error("missing details/collapse")
name: "identical body and state", }
existing: []gitea.Review{ // Should contain short commit SHA
makeReview(100, "bot", "APPROVED", false, "same body\n<!-- review-bot:sonnet -->"), if !strings.Contains(result, "abcdef12") {
}, t.Error("missing short SHA")
newBody: "same body\n<!-- review-bot:sonnet -->", }
newEvent: "APPROVED", // Should NOT contain full SHA
sentinel: "<!-- review-bot:sonnet -->", if strings.Contains(result, "abcdef1234567890") {
want: true, t.Error("should truncate SHA to 8 chars")
}, }
{ // Should contain the original body inside details
name: "same body but different state", if !strings.Contains(result, original) {
existing: []gitea.Review{ t.Error("original body not preserved in collapsed section")
makeReview(100, "bot", "APPROVED", false, "body\n<!-- review-bot:sonnet -->"), }
}, // Should end with sentinel
newBody: "body\n<!-- review-bot:sonnet -->", if !strings.Contains(result, sentinel) {
newEvent: "REQUEST_CHANGES", t.Error("missing sentinel")
sentinel: "<!-- review-bot:sonnet -->", }
want: false,
},
{
name: "different body same state",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", false, "old body\n<!-- review-bot:sonnet -->"),
},
newBody: "new body\n<!-- review-bot:sonnet -->",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "stale review with same body (should still post)",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", true, "same\n<!-- review-bot:sonnet -->"),
},
newBody: "same\n<!-- review-bot:sonnet -->",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "different sentinel (not our review)",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
},
newBody: "body\n<!-- review-bot:sonnet -->",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
} }
for _, tc := range tests { func TestBuildSupersededBodyShortSHA(t *testing.T) {
t.Run(tc.name, func(t *testing.T) { // Short SHA should pass through without panic
got := reviewUnchanged(tc.existing, tc.newBody, tc.newEvent, tc.sentinel) result := buildSupersededBody("body", "abc", "<!-- review-bot:x -->")
if got != tc.want { if !strings.Contains(result, "abc") {
t.Errorf("reviewUnchanged() = %v, want %v", got, tc.want) t.Error("short SHA not preserved")
}
})
} }
} }
+1
View File
@@ -323,6 +323,7 @@ type Review struct {
} `json:"user"` } `json:"user"`
State string `json:"state"` State string `json:"state"`
Stale bool `json:"stale"` Stale bool `json:"stale"`
CommitID string `json:"commit_id"`
} }
// ListReviews returns all reviews on a pull request. // ListReviews returns all reviews on a pull request.