Skip to content

fix: remove error-entry exclusion from countFeedbackRounds - #50

Merged
adalton merged 1 commit into
flightctl:mainfrom
adalton:andalton/fix-cost-label-oscillation
Jul 10, 2026
Merged

adalton merged 1 commit into
flightctl:mainfrom
adalton:andalton/fix-cost-label-oscillation

Conversation

@adalton

@adalton adalton commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix cost label oscillation observed in production on osac-operator PR #338: labels cycled between "Feedback (15)" and "Feedback (14) retry 1" because countFeedbackRounds excluded error entries from the count
  • Remove !strings.Contains(e.Label, "error") filter — error entries with round numbers (e.g., "Feedback (15) (error)") should count as rounds since feedbackLabel always includes the round number
  • Follow-up to fix: prevent feedback loop from silent reply failures and stale session data #44 which introduced the error exclusion based on a code review concern about bare "Feedback (error)" labels, which can't occur in practice

Test plan

  • Updated TestCountFeedbackRounds — "error entries count as rounds" replaces "error entries do not count as rounds"
  • All executor tests pass with -race
  • Lint clean

Assisted-by: Claude noreply@anthropic.com

  • Affected package: executor/
  • Execution pipeline: Updates feedback handling so error feedback entries count toward distinct feedback rounds, preventing oscillating cost labels across retries.
  • Tests: Adjusts round-counting expectations for error feedback entries.
  • Infrastructure/configuration: No changes to container management, workspace lifecycle, crash recovery, configuration, or deployment.

Error entries like "Feedback (15) (error)" were excluded from the
round count by the "!strings.Contains(error)" filter. This caused
the first attempt of each round to compute round N+1 while retries
computed round N (since the error entry wasn't counted), producing
oscillating labels: Feedback (15) → Feedback (14) retry 1 →
Feedback (14) retry 2 → Feedback (15) again.

Since feedbackLabel always includes the round number in error entries,
a bare "Feedback (error)" can never reach the cost table. The
exclusion is unnecessary and harmful — remove it.

Assisted-by: Claude claude-opus-4-6 (1M) <noreply@anthropic.com>
@adalton adalton self-assigned this Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

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: 33fb4e9f-cfa6-4f50-8fd7-26125f9ab329

📥 Commits

Reviewing files that changed from the base of the PR and between aba05f6 and 327d347.

📒 Files selected for processing (2)
  • executor/costcomment.go
  • executor/costcomment_test.go

Walkthrough

countFeedbackRounds now counts every Feedback label that is not a retry, including error-labelled entries. The associated test updates the expected count from 2 to 3.

Changes

Feedback round counting

Layer / File(s) Summary
Count non-retry feedback rounds
executor/costcomment.go, executor/costcomment_test.go
countFeedbackRounds no longer excludes labels containing error, and TestCountFeedbackRounds verifies that error-labelled feedback counts as an additional round.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: executor

🚥 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 change: removing error-entry exclusion from countFeedbackRounds.
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, credentialed URLs, or long base64 literals were introduced in the changed files.
No-Weak-Crypto ✅ Passed No weak crypto, custom crypto, or secret comparisons were introduced; the PR only changes feedback-label counting logic in costcomment.go/tests.
No-Injection-Vectors ✅ Passed The patch only changes feedback-round counting and tests; no SQL/shell/eval/yaml/exec injection sinks or user-controlled execution paths were added.
Container-Privileges ✅ Passed PASS: Only executor Go logic/tests changed; no container/K8s manifests or privileged settings (privileged, hostPID/Network/IPC, allowPrivilegeEscalation, root) were added.
No-Sensitive-Data-In-Logs ✅ Passed PASS: no logging changes; the patch only adjusts feedback-round counting and tests, so it introduces no new sensitive-data exposure risk.
Resource-Leaks ✅ Passed No new file, HTTP, DB, or network opens/goroutines were added; the patch only changes label-counting logic and tests.
Unchecked-Errors ✅ Passed Touched files only change round counting/tests; no _ = err or silent error discard was introduced, and existing error paths are logged.
Ai-Attribution ✅ Passed HEAD commit includes an Assisted-by trailer for Claude, and no Co-Authored-By AI trailer was found.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@adalton
adalton merged commit 2a17ed3 into flightctl:main Jul 10, 2026
3 checks passed
@adalton
adalton deleted the andalton/fix-cost-label-oscillation branch July 10, 2026 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant