From 87d14791d8ae7cf2e0b9e098aa2abb88d1efb0c9 Mon Sep 17 00:00:00 2001 From: Andy Dalton Date: Wed, 8 Jul 2026 13:57:13 -0400 Subject: [PATCH 1/2] fix: prevent feedback loop from silent reply failures and stale session data The bot ran 14 feedback sessions ($34) on a single PR because: 1. Stale comment-responses.json from prior sessions caused false "success" on the AI-responses path, resetting the retry counter. 2. The final-attempt path returned success regardless of whether addressed-reply comments actually posted, enabling infinite loops. 3. handleNoChanges lacked final-attempt handling, leaving review comments dangling without acknowledgement. Changes: - Clean AI output files between feedback sessions (comment-responses, session-output, cli-output, pr.md) to prevent stale data from prior sessions being read as current. session-context.md is preserved. - Reply functions (replyToCommentsOnRepo, replyToComments, replyUnableToAddress) now return the count of successfully posted replies. Critical paths (final-attempt, AI-responses) return an error when zero replies land, preventing false retry-counter resets. Commit-success paths remain best-effort. - handleNoChanges now handles the final-attempt scenario by posting "unable to address" replies instead of returning a bare error. - Cost labels use round-based numbering with retry tracking and outcome suffixes: "Feedback (2) retry 1 (no changes)" instead of sequential "Feedback #3". Infrastructure errors get "(error)" label distinct from "(no changes)". - Cost labels use parenthesized numbers instead of # to prevent GitHub autolinking to unrelated issues/PRs. - Missing cost comment on single-repo final-attempt ErrNoChanges path is now recorded. Assisted-by: Claude claude-opus-4-6 (1M) --- executor/costcomment.go | 44 +++++-- executor/costcomment_test.go | 227 +++++++++++++++++++++++++++++++---- executor/feedback.go | 127 +++++++++++++++++--- executor/feedback_test.go | 64 ++++++++-- executor/merge.go | 4 +- executor/pipeline.go | 4 +- executor/pipeline_test.go | 1 + 7 files changed, 403 insertions(+), 68 deletions(-) diff --git a/executor/costcomment.go b/executor/costcomment.go index 8106246..d2ebc2e 100644 --- a/executor/costcomment.go +++ b/executor/costcomment.go @@ -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 @@ -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) @@ -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}}) diff --git a/executor/costcomment_test.go b/executor/costcomment_test.go index 1038eb3..22baaf7 100644 --- a/executor/costcomment_test.go +++ b/executor/costcomment_test.go @@ -1,6 +1,8 @@ package executor import ( + "errors" + "fmt" "strings" "testing" @@ -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) @@ -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") { @@ -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** | ` @@ -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]) } } @@ -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) @@ -144,47 +172,196 @@ 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: "Feedback #4", + want: 1, }, { - name: "empty entries", + 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: 3, + }, + { + 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: "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) + } + }) + } +} + +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) } }) } diff --git a/executor/feedback.go b/executor/feedback.go index a0cd351..c5d90aa 100644 --- a/executor/feedback.go +++ b/executor/feedback.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "os" "path/filepath" "sort" "strings" @@ -133,6 +134,9 @@ func (p *Pipeline) executeFeedback(ctx context.Context, job *jobmanager.Job) (re return result, err } + // --- Step 9a: Remove stale AI outputs from prior session --- + cleanAIOutputs(logger, wsPath) + // --- Step 10: Determine AI provider --- provider := p.resolveProvider(settings) logger.Info("AI provider selected", zap.String("provider", provider)) @@ -213,7 +217,7 @@ func (p *Pipeline) executeFeedback(ctx context.Context, job *jobmanager.Job) (re return result, fmt.Errorf("check changes: %w", err) } if !hasChanges { - return p.handleNoChanges(logger, settings, prDetails, newComments, ciFailures, wsPath, result, exitCode) + return p.handleNoChanges(logger, settings, prDetails, newComments, ciFailures, wsPath, result, exitCode, job.AttemptNum) } // --- Step 15: Commit via GitHub API --- @@ -226,12 +230,18 @@ func (p *Pipeline) executeFeedback(ctx context.Context, job *jobmanager.Job) (re if errors.Is(err, services.ErrNoChanges) { if p.isFinalAttempt(job.AttemptNum) { logger.Info("Final attempt produced no changes, posting unable-to-address replies") - p.replyUnableToAddress(logger, settings, prDetails, newComments) + posted := p.replyUnableToAddress(logger, settings, prDetails, newComments) + p.postOrUpdateCostComment(logger, + settings.Repos[0].Owner, settings.Repos[0].Repo, + prDetails.Number, result.CostUSD, "Feedback (unable)", job.AttemptNum) + if posted == 0 { + return result, fmt.Errorf("final attempt: failed to post unable-to-address replies") + } return result, nil } p.postOrUpdateCostComment(logger, settings.Repos[0].Owner, settings.Repos[0].Repo, - prDetails.Number, result.CostUSD, "Feedback") + prDetails.Number, result.CostUSD, "Feedback (no changes)", job.AttemptNum) return result, fmt.Errorf("AI produced no committable changes (exit code: %d)", exitCode) } if err != nil { @@ -246,14 +256,14 @@ func (p *Pipeline) executeFeedback(ctx context.Context, job *jobmanager.Job) (re // --- Step 17: Clear failure labels and reply to addressed comments --- p.clearFailureLabels(logger, job.TicketKey, settings.FailureLabels) aiResponses := readCommentResponses(wsPath) - p.replyToComments(logger, settings, prDetails, newComments, sha, aiResponses) + p.replyToComments(logger, settings, prDetails, newComments, sha, aiResponses) // best-effort: commit is the primary outcome // --- Step 17a: Post CI fix attempt marker --- p.postCIFixMarker(logger, owner, repo, prDetails.Number, ciFailures, sha) p.postOrUpdateCostComment(logger, settings.Repos[0].Owner, settings.Repos[0].Repo, - prDetails.Number, result.CostUSD, "Feedback") + prDetails.Number, result.CostUSD, "Feedback", job.AttemptNum) result.PRURL = prDetails.URL result.PRNumber = prDetails.Number @@ -379,6 +389,9 @@ func (p *Pipeline) executeMultiRepoFeedback( return result, err } + // --- Step 8a: Remove stale AI outputs from prior session --- + cleanAIOutputs(logger, wsPath) + // --- Step 9: Provider, command, container --- provider := p.resolveProvider(settings) sp := buildScriptParams(provider, p.cfg.DefaultClaudeModel, p.cfg.DefaultGeminiModel, repoConfigs[0]) @@ -481,10 +494,10 @@ func (p *Pipeline) executeMultiRepoFeedback( ri.pr.Number, ri.ciFailures, sha) } - // Post cost on the first PR regardless of outcome. + costLabel := feedbackCostLabel(err, len(repoSHAs), p.isFinalAttempt(job.AttemptNum)) p.postOrUpdateCostComment(logger, repoInfos[0].repo.Owner, repoInfos[0].repo.Repo, - repoInfos[0].pr.Number, result.CostUSD, "Feedback") + repoInfos[0].pr.Number, result.CostUSD, costLabel, job.AttemptNum) if err != nil { return result, err @@ -656,18 +669,26 @@ func (p *Pipeline) commitMultiRepoFeedback( aiResponses := readCommentResponses(params.wsPath) if aiResponses != nil { logger.Info("AI produced no code changes but provided comment responses") + totalPosted := 0 for _, ri := range params.repoInfos { - p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, + totalPosted += p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, ri.pr, ri.newCmts, "", aiResponses) } + if totalPosted == 0 { + return nil, fmt.Errorf("AI provided comment responses but failed to post any replies") + } return nil, nil } if params.finalAttempt { logger.Info("Final attempt produced no changes, posting unable-to-address replies") + totalPosted := 0 for _, ri := range params.repoInfos { - p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, + totalPosted += p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, ri.pr, ri.newCmts, "unable") } + if totalPosted == 0 { + return nil, fmt.Errorf("final attempt: failed to post unable-to-address replies") + } return nil, nil } return nil, fmt.Errorf("AI produced no changes (exit code: %d)", params.exitCode) @@ -703,10 +724,14 @@ func (p *Pipeline) commitMultiRepoFeedback( if len(repoSHAs) == 0 { if params.finalAttempt { logger.Info("Final attempt produced no committable changes, posting unable-to-address replies") + totalPosted := 0 for _, ri := range params.repoInfos { - p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, + totalPosted += p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, ri.pr, ri.newCmts, "unable") } + if totalPosted == 0 { + return nil, fmt.Errorf("final attempt: failed to post unable-to-address replies") + } return nil, nil } return nil, fmt.Errorf("AI produced no committable changes (exit code: %d)", params.exitCode) @@ -755,7 +780,7 @@ func (p *Pipeline) ensureForkRemoteForRepo( // replyToCommentsOnRepo posts replies to comments on a specific // repo's PR. When sha is "unable", posts unable-to-address replies. // When sha is a commit SHA, posts addressed replies with optional AI -// response summaries. +// response summaries. Returns the number of replies successfully posted. func (p *Pipeline) replyToCommentsOnRepo( logger *zap.Logger, owner, repo string, @@ -763,12 +788,13 @@ func (p *Pipeline) replyToCommentsOnRepo( comments []models.PRComment, sha string, aiResponses ...map[int64]string, -) { +) int { var responses map[int64]string if len(aiResponses) > 0 { responses = aiResponses[0] } + posted := 0 for _, c := range comments { var replyBody string switch { @@ -789,6 +815,8 @@ func (p *Pipeline) replyToCommentsOnRepo( logger.Warn("Failed to reply to review comment", zap.Int64("comment_id", c.ID), zap.Error(err)) + } else { + posted++ } } else { contextual := conversationReplyBody(c, replyBody) @@ -797,9 +825,12 @@ func (p *Pipeline) replyToCommentsOnRepo( logger.Warn("Failed to reply to conversation comment", zap.Int64("comment_id", c.ID), zap.Error(err)) + } else { + posted++ } } } + return posted } // CategorizeComments separates PR comments into new (requiring action) @@ -917,11 +948,12 @@ func (p *Pipeline) replyToComments( comments []models.PRComment, commitSHA string, aiResponses map[int64]string, -) { +) int { shortSHA := commitSHA if len(shortSHA) > 7 { shortSHA = shortSHA[:7] } + posted := 0 for _, c := range comments { var replyBody string if summary, ok := aiResponses[c.ID]; ok { @@ -942,6 +974,8 @@ func (p *Pipeline) replyToComments( logger.Warn("Failed to reply to review comment", zap.Int64("comment_id", c.ID), zap.Error(err)) + } else { + posted++ } } else { contextual := conversationReplyBody(c, replyBody) @@ -951,9 +985,12 @@ func (p *Pipeline) replyToComments( logger.Warn("Failed to reply to conversation comment", zap.Int64("comment_id", c.ID), zap.Error(err)) + } else { + posted++ } } } + return posted } // isFinalAttempt returns true when the current attempt is the last @@ -967,13 +1004,15 @@ func (p *Pipeline) isFinalAttempt(attemptNum int) bool { // replyUnableToAddress posts a reply to each comment indicating that // the bot was unable to make changes after multiple attempts. The // reply includes an addressed marker so the comment is not picked up -// again by future scanner cycles. +// again by future scanner cycles. Returns the number of replies +// successfully posted. func (p *Pipeline) replyUnableToAddress( logger *zap.Logger, settings *models.ProjectSettings, prDetails *models.PRDetails, comments []models.PRComment, -) { +) int { + posted := 0 for _, c := range comments { replyBody := "I was unable to produce code changes to address this comment after multiple attempts." @@ -983,6 +1022,8 @@ func (p *Pipeline) replyUnableToAddress( logger.Warn("Failed to reply to review comment", zap.Int64("comment_id", c.ID), zap.Error(err)) + } else { + posted++ } } else { contextual := conversationReplyBody(c, replyBody) @@ -992,9 +1033,12 @@ func (p *Pipeline) replyUnableToAddress( logger.Warn("Failed to reply to conversation comment", zap.Int64("comment_id", c.ID), zap.Error(err)) + } else { + posted++ } } } + return posted } // conversationReplyBody builds a reply to a conversation comment with @@ -1036,6 +1080,40 @@ func (p *Pipeline) analyzeMultiRepoCIFailures( return all } +func feedbackCostLabel(commitErr error, commitCount int, finalAttempt bool) string { + switch { + case commitErr != nil && (strings.Contains(commitErr.Error(), "no changes") || + strings.Contains(commitErr.Error(), "no committable changes")): + return "Feedback (no changes)" + case commitErr != nil: + return "Feedback (error)" + case commitCount == 0 && finalAttempt: + return "Feedback (unable)" + case commitCount == 0: + return "Feedback (no changes)" + default: + return "Feedback" + } +} + +// cleanAIOutputs removes AI-generated output files from the workspace +// to prevent stale data from a prior session being read as current. +// SessionContextPath is intentionally preserved — it carries design +// context from the original session that helps the AI address feedback. +func cleanAIOutputs(logger *zap.Logger, wsPath string) { + for _, rel := range []string{ + taskfile.CommentResponsesPath, + taskfile.PRDescriptionPath, + sessionOutputPath, + cliOutputPath, + } { + if err := os.Remove(filepath.Join(wsPath, rel)); err != nil && !errors.Is(err, os.ErrNotExist) { + logger.Debug("Failed to clean AI output file", + zap.String("path", rel), zap.Error(err)) + } + } +} + func (p *Pipeline) handleNoChanges( logger *zap.Logger, settings *models.ProjectSettings, @@ -1045,6 +1123,7 @@ func (p *Pipeline) handleNoChanges( wsPath string, result jobmanager.JobResult, exitCode int, + attemptNum int, ) (jobmanager.JobResult, error) { owner := settings.Repos[0].Owner repo := settings.Repos[0].Repo @@ -1056,11 +1135,23 @@ func (p *Pipeline) handleNoChanges( aiResponses := readCommentResponses(wsPath) if aiResponses != nil { logger.Info("AI produced no code changes but provided comment responses") - p.replyToComments(logger, settings, prDetails, newComments, "", aiResponses) - p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback") + posted := p.replyToComments(logger, settings, prDetails, newComments, "", aiResponses) + p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback (no changes)", attemptNum) + if posted == 0 { + return result, fmt.Errorf("AI provided comment responses but failed to post any replies") + } + return result, nil + } + if p.isFinalAttempt(attemptNum) { + logger.Info("Final attempt produced no changes, posting unable-to-address replies") + posted := p.replyUnableToAddress(logger, settings, prDetails, newComments) + p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback (unable)", attemptNum) + if posted == 0 { + return result, fmt.Errorf("final attempt: failed to post unable-to-address replies") + } return result, nil } - p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback") + p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback (no changes)", attemptNum) return result, fmt.Errorf("AI produced no changes (exit code: %d)", exitCode) } diff --git a/executor/feedback_test.go b/executor/feedback_test.go index 8a19997..0807f96 100644 --- a/executor/feedback_test.go +++ b/executor/feedback_test.go @@ -86,13 +86,13 @@ func TestExecuteFeedback_AIGeneratedReplies(t *testing.T) { return "abc1234567890", nil } - // Write comment-responses.json before the reply step runs. - // In the real flow the AI writes this during its session; here we - // simulate it by writing the file to the workspace directory that - // the pipeline will read from. - writeCommentResponses(t, d.wsDir, `[ - {"comment_id": 1, "response": "Switched to Optional pattern as suggested."} - ]`) + // AI writes comment-responses.json during the session. + d.containers.ExecFunc = func(ctx context.Context, ctr *container.Container, cmd []string) (string, int, error) { + writeCommentResponses(t, d.wsDir, `[ + {"comment_id": 1, "response": "Switched to Optional pattern as suggested."} + ]`) + return "", 0, nil + } var replyBodies []string d.git.ReplyToCommentFunc = func(_, _ string, _ int, _ int64, body string) error { @@ -225,9 +225,13 @@ func TestExecuteFeedback_NoChanges_WithCommentResponses(t *testing.T) { return false, nil } - writeCommentResponses(t, d.wsDir, `[ - {"comment_id": 1, "response": "No code changes needed — this is already handled."} - ]`) + // AI writes comment-responses.json during the session (inside the container). + d.containers.ExecFunc = func(ctx context.Context, ctr *container.Container, cmd []string) (string, int, error) { + writeCommentResponses(t, d.wsDir, `[ + {"comment_id": 1, "response": "No code changes needed — this is already handled."} + ]`) + return "", 0, nil + } var repliedTo []int64 d.git.ReplyToCommentFunc = func(_, _ string, _ int, commentID int64, _ string) error { @@ -246,6 +250,46 @@ func TestExecuteFeedback_NoChanges_WithCommentResponses(t *testing.T) { } } +func TestExecuteFeedback_StaleCommentResponsesCleaned(t *testing.T) { + d := newFeedbackDeps(t) + d.git.HasChangesFunc = func(dir, baseBranch string) (bool, error) { + return false, nil + } + + // Stale file from a prior session — written BEFORE Execute, not + // by the container. The cleanup should remove it before the AI runs. + writeCommentResponses(t, d.wsDir, `[ + {"comment_id": 1, "response": "Stale response from prior session"} + ]`) + + p := d.pipeline(t) + _, err := p.Execute(context.Background(), newFeedbackJob("PROJ-1")) + + if err == nil || !strings.Contains(err.Error(), "no changes") { + t.Fatalf("expected no-changes error (stale file should be cleaned), got %v", err) + } +} + +func TestExecuteFeedback_CleanupPreservesSessionContext(t *testing.T) { + d := newFeedbackDeps(t) + + // Write session-context.md (should survive cleanup). + ctxPath := filepath.Join(d.wsDir, taskfile.SessionContextPath) + if err := os.MkdirAll(filepath.Dir(ctxPath), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(ctxPath, []byte("design rationale from initial session"), 0o644); err != nil { + t.Fatal(err) + } + + p := d.pipeline(t) + _, _ = p.Execute(context.Background(), newFeedbackJob("PROJ-1")) + + if _, err := os.Stat(ctxPath); os.IsNotExist(err) { + t.Error("session-context.md should be preserved across feedback sessions") + } +} + // --- AI timeout --- func TestExecuteFeedback_SessionTimeout(t *testing.T) { diff --git a/executor/merge.go b/executor/merge.go index 2a57427..d8f4757 100644 --- a/executor/merge.go +++ b/executor/merge.go @@ -234,7 +234,7 @@ func (p *Pipeline) executeSingleRepoMerge( } p.postOrUpdateCostComment(logger, - repo.Owner, repo.Repo, prDetails.Number, result.CostUSD, "Merge conflict resolution") + repo.Owner, repo.Repo, prDetails.Number, result.CostUSD, "Merge conflict resolution", 0) result.PRURL = prDetails.URL result.PRNumber = prDetails.Number @@ -536,7 +536,7 @@ func (p *Pipeline) runMultiRepoMergeAI( p.postOrUpdateCostComment(logger, repoInfos[0].repo.Owner, repoInfos[0].repo.Repo, - repoInfos[0].pr.Number, result.CostUSD, "Merge conflict resolution") + repoInfos[0].pr.Number, result.CostUSD, "Merge conflict resolution", 0) result.PRURL = repoInfos[0].pr.URL result.PRNumber = repoInfos[0].pr.Number diff --git a/executor/pipeline.go b/executor/pipeline.go index aba1417..0e33cf1 100644 --- a/executor/pipeline.go +++ b/executor/pipeline.go @@ -346,7 +346,7 @@ func (p *Pipeline) executeNewTicket(ctx context.Context, job *jobmanager.Job) (r p.clearFailureLabels(logger, job.TicketKey, settings.FailureLabels) p.postOrUpdateCostComment(logger, settings.Repos[0].Owner, settings.Repos[0].Repo, - pr.Number, result.CostUSD, "New ticket") + pr.Number, result.CostUSD, "New ticket", 0) if !draft { p.setLifecycleLabel(logger, job.TicketKey, settings.LifecycleLabels, settings.LifecycleLabels.Review) @@ -986,7 +986,7 @@ func (p *Pipeline) executeMultiRepoNewTicket( // Post cost on the first PR only to avoid double-counting. p.postOrUpdateCostComment(logger, prs[0].owner, prs[0].repo, - prs[0].number, result.CostUSD, "New ticket") + prs[0].number, result.CostUSD, "New ticket", 0) result.PRURL = prs[0].url result.PRNumber = prs[0].number diff --git a/executor/pipeline_test.go b/executor/pipeline_test.go index 26a4183..d0f21d2 100644 --- a/executor/pipeline_test.go +++ b/executor/pipeline_test.go @@ -3665,6 +3665,7 @@ func (d *testDeps) pipeline(t *testing.T) *executor.Pipeline { BotUsername: "ai-bot", DefaultProvider: "claude", AIAPIKeys: map[string]string{"claude": "test-key"}, + MaxRetries: 3, }) } From 3cc84f461642913f824c508da662c5974bfafdf7 Mon Sep 17 00:00:00 2001 From: Andy Dalton Date: Wed, 8 Jul 2026 14:58:40 -0400 Subject: [PATCH 2/2] fix: address CodeRabbit review feedback - Guard posted==0 checks with len(newComments)>0 so CI-only feedback sessions (no review comments, only CI failures) don't incorrectly fail on the final attempt when there are zero comments to reply to - Add attemptNum=0 edge case to TestFeedbackLabel - Log intentionally-ignored Execute error in session-context preservation test - Extract handleErrNoChanges to keep executeFeedback under the cyclomatic complexity threshold Assisted-by: Claude claude-opus-4-6 (1M) --- executor/costcomment_test.go | 6 ++++ executor/feedback.go | 59 ++++++++++++++++++++++++------------ executor/feedback_test.go | 6 +++- 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/executor/costcomment_test.go b/executor/costcomment_test.go index 22baaf7..73618d9 100644 --- a/executor/costcomment_test.go +++ b/executor/costcomment_test.go @@ -243,6 +243,12 @@ func TestFeedbackLabel(t *testing.T) { 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, diff --git a/executor/feedback.go b/executor/feedback.go index c5d90aa..ca388ca 100644 --- a/executor/feedback.go +++ b/executor/feedback.go @@ -228,21 +228,7 @@ func (p *Pipeline) executeFeedback(ctx context.Context, job *jobmanager.Job) (re commitMsg, wsPath, settings.Repos[0].BaseBranch, workItem.Assignee, importExcludes, ) if errors.Is(err, services.ErrNoChanges) { - if p.isFinalAttempt(job.AttemptNum) { - logger.Info("Final attempt produced no changes, posting unable-to-address replies") - posted := p.replyUnableToAddress(logger, settings, prDetails, newComments) - p.postOrUpdateCostComment(logger, - settings.Repos[0].Owner, settings.Repos[0].Repo, - prDetails.Number, result.CostUSD, "Feedback (unable)", job.AttemptNum) - if posted == 0 { - return result, fmt.Errorf("final attempt: failed to post unable-to-address replies") - } - return result, nil - } - p.postOrUpdateCostComment(logger, - settings.Repos[0].Owner, settings.Repos[0].Repo, - prDetails.Number, result.CostUSD, "Feedback (no changes)", job.AttemptNum) - return result, fmt.Errorf("AI produced no committable changes (exit code: %d)", exitCode) + return p.handleErrNoChanges(logger, settings, prDetails, newComments, result, exitCode, job.AttemptNum) } if err != nil { return result, fmt.Errorf("commit changes: %w", err) @@ -674,7 +660,11 @@ func (p *Pipeline) commitMultiRepoFeedback( totalPosted += p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, ri.pr, ri.newCmts, "", aiResponses) } - if totalPosted == 0 { + totalComments := 0 + for _, ri := range params.repoInfos { + totalComments += len(ri.newCmts) + } + if totalPosted == 0 && totalComments > 0 { return nil, fmt.Errorf("AI provided comment responses but failed to post any replies") } return nil, nil @@ -682,11 +672,13 @@ func (p *Pipeline) commitMultiRepoFeedback( if params.finalAttempt { logger.Info("Final attempt produced no changes, posting unable-to-address replies") totalPosted := 0 + totalComments := 0 for _, ri := range params.repoInfos { totalPosted += p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, ri.pr, ri.newCmts, "unable") + totalComments += len(ri.newCmts) } - if totalPosted == 0 { + if totalPosted == 0 && totalComments > 0 { return nil, fmt.Errorf("final attempt: failed to post unable-to-address replies") } return nil, nil @@ -725,11 +717,13 @@ func (p *Pipeline) commitMultiRepoFeedback( if params.finalAttempt { logger.Info("Final attempt produced no committable changes, posting unable-to-address replies") totalPosted := 0 + totalComments := 0 for _, ri := range params.repoInfos { totalPosted += p.replyToCommentsOnRepo(logger, ri.repo.Owner, ri.repo.Repo, ri.pr, ri.newCmts, "unable") + totalComments += len(ri.newCmts) } - if totalPosted == 0 { + if totalPosted == 0 && totalComments > 0 { return nil, fmt.Errorf("final attempt: failed to post unable-to-address replies") } return nil, nil @@ -1096,6 +1090,31 @@ func feedbackCostLabel(commitErr error, commitCount int, finalAttempt bool) stri } } +func (p *Pipeline) handleErrNoChanges( + logger *zap.Logger, + settings *models.ProjectSettings, + prDetails *models.PRDetails, + newComments []models.PRComment, + result jobmanager.JobResult, + exitCode int, + attemptNum int, +) (jobmanager.JobResult, error) { + owner := settings.Repos[0].Owner + repo := settings.Repos[0].Repo + + if p.isFinalAttempt(attemptNum) { + logger.Info("Final attempt produced no changes, posting unable-to-address replies") + posted := p.replyUnableToAddress(logger, settings, prDetails, newComments) + p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback (unable)", attemptNum) + if posted == 0 && len(newComments) > 0 { + return result, fmt.Errorf("final attempt: failed to post unable-to-address replies") + } + return result, nil + } + p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback (no changes)", attemptNum) + return result, fmt.Errorf("AI produced no committable changes (exit code: %d)", exitCode) +} + // cleanAIOutputs removes AI-generated output files from the workspace // to prevent stale data from a prior session being read as current. // SessionContextPath is intentionally preserved — it carries design @@ -1137,7 +1156,7 @@ func (p *Pipeline) handleNoChanges( logger.Info("AI produced no code changes but provided comment responses") posted := p.replyToComments(logger, settings, prDetails, newComments, "", aiResponses) p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback (no changes)", attemptNum) - if posted == 0 { + if posted == 0 && len(newComments) > 0 { return result, fmt.Errorf("AI provided comment responses but failed to post any replies") } return result, nil @@ -1146,7 +1165,7 @@ func (p *Pipeline) handleNoChanges( logger.Info("Final attempt produced no changes, posting unable-to-address replies") posted := p.replyUnableToAddress(logger, settings, prDetails, newComments) p.postOrUpdateCostComment(logger, owner, repo, prDetails.Number, result.CostUSD, "Feedback (unable)", attemptNum) - if posted == 0 { + if posted == 0 && len(newComments) > 0 { return result, fmt.Errorf("final attempt: failed to post unable-to-address replies") } return result, nil diff --git a/executor/feedback_test.go b/executor/feedback_test.go index 0807f96..839214f 100644 --- a/executor/feedback_test.go +++ b/executor/feedback_test.go @@ -283,7 +283,11 @@ func TestExecuteFeedback_CleanupPreservesSessionContext(t *testing.T) { } p := d.pipeline(t) - _, _ = p.Execute(context.Background(), newFeedbackJob("PROJ-1")) + // Error is expected (no changes produced); we only care that + // cleanup ran and preserved session-context.md. + if _, err := p.Execute(context.Background(), newFeedbackJob("PROJ-1")); err != nil { + t.Logf("Execute returned expected error: %v", err) + } if _, err := os.Stat(ctxPath); os.IsNotExist(err) { t.Error("session-context.md should be preserved across feedback sessions")