Skip to content

fix: prevent feedback loop from silent reply failures and stale session data #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adalton merged 2 commits into from
Jul 8, 2026
Merged

fix: prevent feedback loop from silent reply failures and stale session data #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87d1479
fix: prevent feedback loop from silent reply failures and stale sessi…
adalton Jul 8, 2026
3cc84f4
fix: address CodeRabbit review feedback
adalton Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions executor/costcomment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,27 +84,47 @@ func findCostComment(comments []models.IssueComment) *models.IssueComment {
return nil
}

// nextFeedbackLabel derives the sequence label for a feedback entry
// by counting existing feedback entries.
func nextFeedbackLabel(entries []costEntry) string {
// countFeedbackRounds returns the number of distinct feedback rounds
// in the existing entries. A round starts with a "Feedback (N)" entry;
// retries and error entries are excluded from the count.
func countFeedbackRounds(entries []costEntry) int {
count := 0
for _, e := range entries {
if strings.HasPrefix(e.Label, "Feedback") {
if strings.HasPrefix(e.Label, "Feedback") &&
!strings.Contains(e.Label, "retry") &&
!strings.Contains(e.Label, "error") {
count++
}
}
return fmt.Sprintf("Feedback #%d", count+1)
return count
}

// feedbackLabel builds a descriptive label for a feedback cost entry.
// attemptNum distinguishes new rounds (1) from retries (2+). suffix
// describes the outcome (e.g., " (no changes)", " (unable)").
func feedbackLabel(entries []costEntry, attemptNum int, suffix string) string {
if attemptNum <= 1 {
round := countFeedbackRounds(entries) + 1
return fmt.Sprintf("Feedback (%d)%s", round, suffix)
}
round := countFeedbackRounds(entries)
if round == 0 {
round = 1
}
retry := attemptNum - 1
return fmt.Sprintf("Feedback (%d) retry %d%s", round, retry, suffix)
}

// postOrUpdateCostComment posts or updates a cost comment on a PR.
// If label is "Feedback", the sequence number is auto-derived.
// Errors are logged but not propagated — cost comments are non-critical.
// Labels starting with "Feedback" are auto-sequenced into rounds and
// retries based on attemptNum. Errors are logged but not propagated.
func (p *Pipeline) postOrUpdateCostComment(
logger *zap.Logger,
owner, repo string,
prNumber int,
cost float64,
label string,
attemptNum int,
) {
if cost <= 0 {
return
Expand All @@ -121,8 +141,9 @@ func (p *Pipeline) postOrUpdateCostComment(
existing := findCostComment(comments)
if existing != nil {
entries := parseCostComment(existing.Body)
if label == "Feedback" {
label = nextFeedbackLabel(entries)
if strings.HasPrefix(label, "Feedback") {
suffix := strings.TrimPrefix(label, "Feedback")
label = feedbackLabel(entries, attemptNum, suffix)
}
entries = append(entries, costEntry{Label: label, Cost: cost})
body := formatCostComment(entries)
Expand All @@ -135,8 +156,9 @@ func (p *Pipeline) postOrUpdateCostComment(
return
}

if label == "Feedback" {
label = "Feedback #1"
if strings.HasPrefix(label, "Feedback") {
suffix := strings.TrimPrefix(label, "Feedback")
label = feedbackLabel(nil, attemptNum, suffix)
}
body := formatCostComment([]costEntry{{Label: label, Cost: cost}})

Expand Down
233 changes: 208 additions & 25 deletions executor/costcomment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package executor

import (
"errors"
"fmt"
"strings"
"testing"

Expand All @@ -10,7 +12,7 @@ import (
func TestFormatCostComment(t *testing.T) {
entries := []costEntry{
{Label: "New ticket", Cost: 4.32},
{Label: "Feedback #1", Cost: 1.15},
{Label: "Feedback (1)", Cost: 1.15},
}

got := formatCostComment(entries)
Expand All @@ -24,7 +26,7 @@ func TestFormatCostComment(t *testing.T) {
if !strings.Contains(got, "$4.32") {
t.Error("should contain first entry cost")
}
if !strings.Contains(got, "Feedback #1") {
if !strings.Contains(got, "Feedback (1)") {
t.Error("should contain second entry label")
}
if !strings.Contains(got, "$1.15") {
Expand Down Expand Up @@ -54,7 +56,7 @@ func TestParseCostComment(t *testing.T) {
| Session | Cost |
|---------|------|
| New ticket | $4.32 |
| Feedback #1 | $1.15 |
| Feedback (1) | $1.15 |
| **Total** | **$5.47** |
`

Expand All @@ -66,8 +68,8 @@ func TestParseCostComment(t *testing.T) {
if entries[0].Label != "New ticket" || entries[0].Cost != 4.32 {
t.Errorf("first entry = %+v, want {New ticket, 4.32}", entries[0])
}
if entries[1].Label != "Feedback #1" || entries[1].Cost != 1.15 {
t.Errorf("second entry = %+v, want {Feedback #1, 1.15}", entries[1])
if entries[1].Label != "Feedback (1)" || entries[1].Cost != 1.15 {
t.Errorf("second entry = %+v, want {Feedback (1), 1.15}", entries[1])
}
}

Expand All @@ -91,8 +93,34 @@ func TestParseCostComment_EmptyTable(t *testing.T) {
func TestFormatThenParse_Roundtrip(t *testing.T) {
original := []costEntry{
{Label: "New ticket", Cost: 4.32},
{Label: "Feedback #1", Cost: 1.15},
{Label: "Feedback #2", Cost: 0.89},
{Label: "Feedback (1)", Cost: 1.15},
{Label: "Feedback (2)", Cost: 0.89},
}

body := formatCostComment(original)
parsed := parseCostComment(body)

if len(parsed) != len(original) {
t.Fatalf("roundtrip: got %d entries, want %d", len(parsed), len(original))
}
for i, e := range parsed {
if e.Label != original[i].Label {
t.Errorf("entry %d label = %q, want %q", i, e.Label, original[i].Label)
}
if e.Cost != original[i].Cost {
t.Errorf("entry %d cost = %v, want %v", i, e.Cost, original[i].Cost)
}
}
}

func TestFormatThenParse_Roundtrip_WithRetriesAndSuffixes(t *testing.T) {
original := []costEntry{
{Label: "New ticket", Cost: 3.99},
{Label: "Feedback (1)", Cost: 0.56},
{Label: "Feedback (2) (no changes)", Cost: 0.48},
{Label: "Feedback (2) retry 1 (no changes)", Cost: 4.14},
{Label: "Feedback (2) retry 2 (unable)", Cost: 1.83},
{Label: "Feedback (3) (no changes)", Cost: 2.34},
}

body := formatCostComment(original)
Expand Down Expand Up @@ -144,47 +172,202 @@ func TestFindCostComment_EmptyList(t *testing.T) {
}
}

func TestNextFeedbackLabel(t *testing.T) {
func TestCountFeedbackRounds(t *testing.T) {
tests := []struct {
name string
entries []costEntry
want string
want int
}{
{
name: "no existing feedback",
name: "no feedback entries",
entries: []costEntry{{Label: "New ticket", Cost: 1}},
want: "Feedback #1",
want: 0,
},
{
name: "one existing feedback",
name: "one round",
entries: []costEntry{
{Label: "New ticket", Cost: 1},
{Label: "Feedback #1", Cost: 1},
{Label: "Feedback (1)", Cost: 1},
},
want: "Feedback #2",
want: 1,
},
{
name: "three existing feedbacks",
name: "retries do not count as rounds",
entries: []costEntry{
{Label: "New ticket", Cost: 1},
{Label: "Feedback #1", Cost: 1},
{Label: "Feedback #2", Cost: 1},
{Label: "Feedback #3", Cost: 1},
{Label: "Feedback (1) (no changes)", Cost: 1},
{Label: "Feedback (1) retry 1 (no changes)", Cost: 1},
{Label: "Feedback (1) retry 2 (unable)", Cost: 1},
},
want: 1,
},
{
name: "multiple rounds with retries",
entries: []costEntry{
{Label: "Feedback (1)", Cost: 1},
{Label: "Feedback (2) (no changes)", Cost: 1},
{Label: "Feedback (2) retry 1 (no changes)", Cost: 1},
{Label: "Feedback (3)", Cost: 1},
},
want: "Feedback #4",
want: 3,
},
{
name: "empty entries",
name: "nil entries",
entries: nil,
want: "Feedback #1",
want: 0,
},
{
name: "error entries do not count as rounds",
entries: []costEntry{
{Label: "Feedback (1)", Cost: 1},
{Label: "Feedback (error)", Cost: 1},
{Label: "Feedback (2)", Cost: 1},
},
want: 2,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := countFeedbackRounds(tt.entries)
if got != tt.want {
t.Errorf("countFeedbackRounds() = %d, want %d", got, tt.want)
}
})
}
}

func TestFeedbackLabel(t *testing.T) {
tests := []struct {
name string
entries []costEntry
attemptNum int
suffix string
want string
}{
{
name: "zero attemptNum treated as new round",
entries: nil,
attemptNum: 0,
want: "Feedback (1)",
},
{
name: "first attempt, no existing entries",
entries: nil,
attemptNum: 1,
want: "Feedback (1)",
},
{
name: "first attempt, one existing round",
entries: []costEntry{
{Label: "New ticket", Cost: 1},
{Label: "Feedback (1)", Cost: 1},
},
attemptNum: 1,
want: "Feedback (2)",
},
{
name: "first attempt with suffix",
entries: []costEntry{
{Label: "Feedback (1)", Cost: 1},
},
attemptNum: 1,
suffix: " (no changes)",
want: "Feedback (2) (no changes)",
},
{
name: "retry of current round",
entries: []costEntry{
{Label: "Feedback (1)", Cost: 1},
{Label: "Feedback (2) (no changes)", Cost: 1},
},
attemptNum: 2,
suffix: " (no changes)",
want: "Feedback (2) retry 1 (no changes)",
},
{
name: "third retry",
entries: []costEntry{
{Label: "Feedback (1)", Cost: 1},
{Label: "Feedback (2) (no changes)", Cost: 1},
{Label: "Feedback (2) retry 1 (no changes)", Cost: 1},
{Label: "Feedback (2) retry 2 (no changes)", Cost: 1},
},
attemptNum: 4,
suffix: " (unable)",
want: "Feedback (2) retry 3 (unable)",
},
{
name: "retry with no existing entries defaults to round 1",
entries: nil,
attemptNum: 2,
suffix: " (no changes)",
want: "Feedback (1) retry 1 (no changes)",
},
{
name: "retry with no suffix",
entries: []costEntry{
{Label: "Feedback (1) (no changes)", Cost: 1},
},
attemptNum: 2,
want: "Feedback (1) retry 1",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := feedbackLabel(tt.entries, tt.attemptNum, tt.suffix)
if got != tt.want {
t.Errorf("feedbackLabel() = %q, want %q", got, tt.want)
}
})
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func TestFeedbackCostLabel(t *testing.T) {
tests := []struct {
name string
commitErr error
commitCount int
finalAttempt bool
want string
}{
{
name: "no-changes error",
commitErr: fmt.Errorf("AI produced no changes (exit code: 0)"),
want: "Feedback (no changes)",
},
{
name: "no-committable-changes error",
commitErr: fmt.Errorf("AI produced no committable changes (exit code: 0)"),
want: "Feedback (no changes)",
},
{
name: "infrastructure error",
commitErr: errors.New("commit changes for svc-a: API rate limit"),
want: "Feedback (error)",
},
{
name: "final attempt with no commits",
commitCount: 0,
finalAttempt: true,
want: "Feedback (unable)",
},
{
name: "no changes, not final",
commitCount: 0,
want: "Feedback (no changes)",
},
{
name: "success with commits",
commitCount: 2,
want: "Feedback",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := nextFeedbackLabel(tt.entries)
got := feedbackCostLabel(tt.commitErr, tt.commitCount, tt.finalAttempt)
if got != tt.want {
t.Errorf("nextFeedbackLabel() = %q, want %q", got, tt.want)
t.Errorf("feedbackCostLabel() = %q, want %q", got, tt.want)
}
})
}
Expand Down
Loading
Loading