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

[adalton]

Summary

• Fix feedback loop that caused 14 AI sessions ($34) on a single PR due to silent reply failures resetting the retry counter and stale comment-responses.json from prior sessions causing false success
• Clean AI output files between feedback sessions to prevent stale data from being read as current
• Reply functions now return posted count; critical paths (final-attempt, AI-responses) fail when zero replies land
• handleNoChanges now handles final-attempt scenario with "unable to address" replies
• Cost labels use round-based numbering with retries and outcome suffixes (e.g., "Feedback (2) retry 1 (no changes)") instead of sequential "Feedback Add Basic CI Tests #3", and parenthesized numbers to avoid GitHub autolinks

Test plan

• TestExecuteFeedback_StaleCommentResponsesCleaned — stale file from prior session doesn't cause false success
• TestExecuteFeedback_NoChanges_WithCommentResponses — AI writing responses during session (not pre-written) works correctly
• TestExecuteFeedback_CleanupPreservesSessionContext — session-context.md survives cleanup
• TestFeedbackCostLabel — all four branches (no-changes, infrastructure error, final attempt, success)
• TestCountFeedbackRounds — error and retry entries excluded from round count
• TestFeedbackLabel — round-based labeling with retries and suffixes
• All 37 feedback tests pass with -race
• Full test suite passes
• Lint clean (no new issues)

Assisted-by: Claude noreply@anthropic.com

Affects executor/ only.

• Feedback handling now cleans stale AI output artifacts between runs (preventing old comment-responses.json from being reused) while preserving session-context.md, addressing the repeated AI-session feedback loop.
• Reply posting now returns the number of successfully posted replies; “no changes”/failure paths fail when zero replies are posted (including stricter final-attempt “unable to address” handling).
• Cost-comment labeling is updated to round-based, parenthesized formats with retry/outcome suffixes (e.g., Feedback (2) retry 1 (no changes) / Feedback (unable)), avoiding GitHub autolink behavior.
• Ticket/PR-adjacent cost-comment call sites were updated to pass the new attempt parameter (merge flow and new-ticket pipelines).
• Tests were expanded to cover stale-output cleanup, session-context preservation, updated feedback-label formatting/round counting, and the new failure semantics when no replies are posted.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: d7a35054-1389-4387-ad16-68957457ebd1

📥 Commits

Reviewing files that changed from the base of the PR and between 87d1479 and 3cc84f4.

📒 Files selected for processing (3)

• executor/costcomment_test.go
• executor/feedback.go
• executor/feedback_test.go

---

Walkthrough

Cost-comment labels now use parenthesized feedback rounds and attempt-aware retries. Feedback execution clears stale AI output files, counts posted replies, and fails when AI responses produce no successful posts. Call sites and tests were updated for the new behavior.

Changes

Feedback Labeling and Reply Tracking

Layer / File(s)	Summary
Feedback round/retry label generation executor/costcomment.go, executor/costcomment_test.go	Replaces the old Feedback #N`` sequencing with round/retry label generation based on attemptNum; tests now cover parsing, formatting, round counting, and feedback cost labels.
postOrUpdateCostComment call site updates executor/merge.go, executor/pipeline.go, executor/pipeline_test.go	Updates merge and new-ticket cost-comment calls to pass the new attemptNum argument, and sets MaxRetries: 3 in shared pipeline test config.
Workspace AI output cleanup and reply counting executor/feedback.go	Adds stale AI output cleanup, attempt-aware cost labels, posted-reply counting, and zero-success validation across single-repo and multi-repo feedback flows.
Feedback pipeline test updates for cleanup and reply flow executor/feedback_test.go	Moves comment-responses.json creation into simulated container exec and adds cleanup-related regression tests.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• flightctl/jira-ai-issue-solver#42: Also changes executor/feedback.go multi-repo feedback flow and related posting behavior.

Suggested labels: executor

Suggested reviewers: eranco74, amir-yogev-gh

🚥 Pre-merge checks | ✅ 13

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main fix: preventing feedback loops caused by silent reply failures and stale session data.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets, credential URLs, or long base64 blobs were found in the touched files; changes are test fixtures and feedback logic only.
No-Weak-Crypto	✅ Passed	No weak-crypto usage found; the PR only changes feedback/cost-labeling and cleanup logic, with no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or secret comparisons.
No-Injection-Vectors	✅ Passed	No SQL, shell, eval/exec, YAML, pickle, or HTML injection sinks were added; changes only format labels, delete fixed workspace files, and call Git APIs.
Container-Privileges	✅ Passed	PASS: PR only changes Go code/tests; no container/K8s manifests were modified and no privileged/securityContext fields appear in the diff.
No-Sensitive-Data-In-Logs	✅ Passed	PASS: New logs only include fixed filenames/comment IDs and generic errors; no passwords, tokens, PII, session IDs, hostnames, or customer data are logged.
Resource-Leaks	✅ Passed	PASS: no new unclosed files/HTTP bodies/connections or stray goroutines in touched code; the temp file in tests is explicitly closed.
Unchecked-Errors	✅ Passed	No new unchecked error returns found; production paths log/return errors, and underscore bindings ignore non-error tuple values in tests.
Ai-Attribution	✅ Passed	PR and commit bodies include acceptable Assisted-by: Claude trailers; no Co-Authored-By AI attribution was found.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

executor/costcomment.go (1)

144-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication in suffix/label derivation.

The strings.HasPrefix(label, "Feedback") → TrimPrefix → feedbackLabel(...) sequence is repeated verbatim in both branches (existing comment vs. first comment), differing only in whether entries or nil is passed. Consider extracting a tiny helper (e.g. deriveFeedbackLabel(entries []costEntry, attemptNum int, label string) string) to avoid the duplicate 3-line block.

♻️ Proposed helper extraction

+func deriveFeedbackLabel(entries []costEntry, attemptNum int, label string) string {
+	if !strings.HasPrefix(label, "Feedback") {
+		return label
+	}
+	suffix := strings.TrimPrefix(label, "Feedback")
+	return feedbackLabel(entries, attemptNum, suffix)
+}
+
 	existing := findCostComment(comments)
 	if existing != nil {
 		entries := parseCostComment(existing.Body)
-		if strings.HasPrefix(label, "Feedback") {
-			suffix := strings.TrimPrefix(label, "Feedback")
-			label = feedbackLabel(entries, attemptNum, suffix)
-		}
+		label = deriveFeedbackLabel(entries, attemptNum, label)
 		entries = append(entries, costEntry{Label: label, Cost: cost})
 		body := formatCostComment(entries)
 		...
 	}
 
-	if strings.HasPrefix(label, "Feedback") {
-		suffix := strings.TrimPrefix(label, "Feedback")
-		label = feedbackLabel(nil, attemptNum, suffix)
-	}
+	label = deriveFeedbackLabel(nil, attemptNum, label)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@executor/costcomment.go` around lines 144 - 162, The Feedback label
derivation logic is duplicated in both branches of cost comment handling.
Extract the repeated strings.HasPrefix/TrimPrefix/feedbackLabel sequence into a
small helper near cost comment generation (for example, in the code path around
formatCostComment and UpdateIssueComment) and use it in both the
existing-comment and first-comment cases, passing either entries or nil as
needed.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@executor/costcomment_test.go`:
- Around line 238-317: `TestFeedbackLabel` is missing coverage for the
`attemptNum == 0` boundary that falls into `feedbackLabel`’s `attemptNum <= 1`
branch. Add a dedicated test case in `TestFeedbackLabel` using the existing
`feedbackLabel` helper and `costEntry` fixtures to assert the expected label
when `attemptNum` is zero, so the zero-value/edge-case behavior is explicitly
verified alongside the current `attemptNum` 1 and retry cases.

In `@executor/feedback_test.go`:
- Around line 285-291: The test in feedback_test.go is discarding the error from
p.Execute, which makes the preservation assertion unreliable. Update the test
around d.pipeline(t), p.Execute, and the ctxPath os.Stat check to either assert
that Execute succeeds before checking session-context.md or add a brief
justification comment if the error is intentionally ignored. Keep the assertion
meaningful by ensuring the test only passes when Execute reaches the cleanup
path in feedback.go.

In `@executor/feedback.go`:
- Around line 1136-1153: The no-op reply paths in feedback.go currently treat
posted == 0 as an error even when there are no comments to answer. Update the
reply handling around p.replyToComments and p.replyUnableToAddress in the
feedback flow so the zero-post failure is only checked when newComments actually
contains comments to reply to, allowing CI-only cases to exit successfully
without an error.

---

Outside diff comments:
In `@executor/costcomment.go`:
- Around line 144-162: The Feedback label derivation logic is duplicated in both
branches of cost comment handling. Extract the repeated
strings.HasPrefix/TrimPrefix/feedbackLabel sequence into a small helper near
cost comment generation (for example, in the code path around formatCostComment
and UpdateIssueComment) and use it in both the existing-comment and
first-comment cases, passing either entries or nil as needed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: c34ca4c3-8acd-4c15-a176-d4d3cdc6a514

📥 Commits

Reviewing files that changed from the base of the PR and between ecc42e3 and 87d1479.

📒 Files selected for processing (7)

• executor/costcomment.go
• executor/costcomment_test.go
• executor/feedback.go
• executor/feedback_test.go
• executor/merge.go
• executor/pipeline.go
• executor/pipeline_test.go

[adalton]

@coderabbitai Re: the suggestion to extract deriveFeedbackLabel to deduplicate the HasPrefix/TrimPrefix/feedbackLabel sequence in postOrUpdateCostComment — declining this one. It's a 3-line block duplicated across two branches that differ only in whether entries is nil. Extracting a helper adds a function call and indirection for negligible readability gain; the two sites are adjacent and easy to keep in sync.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@adalton Fair enough — that's a reasonable call. A 3-line block that's already adjacent and easy to eyeball for consistency doesn't really justify a helper function; the indirection cost can outweigh the DRY benefit at that size. I won't push on this further.

(=ↀωↀ=)

---

✏️ Learnings added

Learnt from: adalton
URL: https://github.com/flightctl/jira-ai-issue-solver/pull/44

Timestamp: 2026-07-08T19:00:24.484Z
Learning: In executor/costcomment.go, within postOrUpdateCostComment, the HasPrefix/TrimPrefix/feedbackLabel sequence is intentionally duplicated across the two branches (existing entries vs. nil entries) rather than extracted into a helper like deriveFeedbackLabel. Maintainer adalton prefers keeping this 3-line duplicated block as-is since the two call sites are adjacent and easy to keep in sync; extracting a helper was deemed to add indirection for negligible readability gain.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[eranco74]

/lgtm