[knechtionscoding]

What type of PR is this?

/kind bug

What this PR does / why we need it:

When Slack rejects a thread reply with cannot_reply_to_message (for example, when the thread is closed or the bot cannot reply to it), the kelos-slack-server reporting cycle currently retries the post forever. Because the failed post prevented the task's kelos.dev/slack-report-phase annotation from being persisted, the reporter re-posted on every 30s tick — observed in production as two self-triggered tasks wedged in the accepted phase with 198+ failed attempts in a single hour.

This PR treats cannot_reply_to_message as a permanent API error:

• Phase transitions (ReportTaskStatus): log the failure once and still persist the report phase, so the reporting cycle stops retrying that reply on every tick. HTTP-level failures (timeouts, 5xx, rate limits) keep the existing retry behavior.
• Progress snapshots (updateProgress): mark the snapshot as posted on a permanent error so it is not re-attempted every cycle.

The permanent-error detection is isolated in a small isPermanentSlackError helper, living alongside the other SlackReporter helpers in internal/reporting/slack.go, so the classification is easy to extend to other permanent Slack API errors later.

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

• The new behavior is covered by unit tests:
  • TestSlackTaskReporter_CannotReplyToMessageNotRetried — permanent error: one attempt, phase annotation persisted, no further attempts on the next cycle.
  • TestSlackTaskReporter_TransientReplyErrorStillRetried — transient error: error surfaced, annotation not set, retry behavior unchanged.
  • TestSlackTaskReporter_PermanentProgressErrorNotReattempted — permanent error on a progress snapshot: single attempt, not re-attempted next cycle.
  • TestIsPermanentSlackError — classification of wrapped errors, other Slack API errors, HTTP status errors, and plain errors. Includes rate_limited, asserting rate limits stay transient and keep their existing retry behavior.
• isPermanentSlackError matches slack.SlackErrorResponse by value only. That is the shape slack-go v0.20.0 returns from SlackResponse.Err() (misc.go:180, misc.go:195), Error() has a value receiver, and no path in the library returns a pointer, so a *SlackErrorResponse branch would be unreachable.
• The permanent-error path uses break rather than an early return, so it falls through to the existing tail of ReportTaskStatus: setActivityTarget is skipped (firstReplyTS is empty), and terminal phases still reach clearProgressCache/clearActivityState before the phase is persisted.
• Verified: go build ./..., go vet, gofmt, and go test ./internal/reporting/ all pass. The new tests fail against the pre-fix code. The repo-wide failures (internal/controller entrypoint tests, test/e2e, test/integration) are pre-existing environment issues (kubebuilder envtest binaries and a git sandbox limitation) that fail identically without this change.
• make test-integration requires envtest binaries not available in the sandbox environment.

Does this PR introduce a user-facing change?

Slack task reports that Slack permanently rejects with cannot_reply_to_message are no longer retried on every reporting cycle, preventing repeated duplicate posts.

---

Summary by cubic

Stop retrying Slack thread replies rejected with cannot_reply_to_message by classifying them as permanent API errors. This prevents 30s retry loops, wedged tasks, and duplicate posts by persisting the report phase and suppressing progress posts.

• Adds isPermanentSlackError that matches slack.SlackErrorResponse.Err == "cannot_reply_to_message"; HTTP failures and rate limits remain retryable.
• Phase transitions: on permanent error, log once with channel and thread timestamp, persist kelos.dev/slack-report-phase, and set a per-task progress block so the next cycle does not attempt progress posts.
• Progress updates: if blocked, return before reading pod logs; on permanent error, set the block and log channel and thread timestamp on failures.
• The progress block is in-memory: a restart/leader failover may trigger one extra rejected post before re-blocking; it clears via clearProgressCache and is swept in SweepProgressCache.
• Unit tests cover permanent vs transient paths, progress suppression, and error classification. No config or migration changes.

^{Written for commit b84f50d. Summary will update on new commits.}

[knechtionscoding]

@gjkim42 small bug fix around failure to post messages back into slack

[cubic-dev-ai]

1 issue found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/reporting/watcher_test.go">

<violation number="1" location="internal/reporting/watcher_test.go:3067">
P3: The task in TestSlackTaskReporter_CannotReplyToMessageNotRetried never sets ObjectMeta.UID, so the second-cycle suppression keys on the empty UID (blockProgress("")/isProgressBlocked("")). That works in-process but diverges from production, where every task has a UID, and from the sibling TestSlackTaskReporter_PermanentProgressErrorNotReattempted, which sets UID "uid-124". Give the task a UID so the test exercises the same keying path as production and as the other suite.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P3: The task in TestSlackTaskReporter_CannotReplyToMessageNotRetried never sets ObjectMeta.UID, so the second-cycle suppression keys on the empty UID (blockProgress("")/isProgressBlocked("")). That works in-process but diverges from production, where every task has a UID, and from the sibling TestSlackTaskReporter_PermanentProgressErrorNotReattempted, which sets UID "uid-124". Give the task a UID so the test exercises the same keying path as production and as the other suite.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/reporting/watcher_test.go, line 3067:

<comment>The task in TestSlackTaskReporter_CannotReplyToMessageNotRetried never sets ObjectMeta.UID, so the second-cycle suppression keys on the empty UID (blockProgress("")/isProgressBlocked("")). That works in-process but diverges from production, where every task has a UID, and from the sibling TestSlackTaskReporter_PermanentProgressErrorNotReattempted, which sets UID "uid-124". Give the task a UID so the test exercises the same keying path as production and as the other suite.</comment>

<file context>
@@ -3063,3 +3063,201 @@ func TestAppendActivityContext_DoesNotMutateBase(t *testing.T) {
 	}
 }
+
+func TestSlackTaskReporter_CannotReplyToMessageNotRetried(t *testing.T) {
+	task := &kelos.Task{
+		ObjectMeta: metav1.ObjectMeta{
</file context>

[gjkim42]

/kelos claude-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-claude-reviewer-issue-comment-33dbe2d29513 has succeeded. ✅

[kelos-bot]

🤖 Kelos Claude Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Classify Slack cannot_reply_to_message as a permanent API error so phase-transition replies persist their report phase instead of retrying every 30s, and suppress further progress posts for the affected task.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	0	—	none
P3	1	internal/reporting/watcher_test.go:3067	Test task has no UID, so suppression keys on the zero-value UID

Findings

Tests

• [P3] internal/reporting/watcher_test.go:3067 — The task in TestSlackTaskReporter_CannotReplyToMessageNotRetried never sets ObjectMeta.UID, so blockProgress/isProgressBlocked operate on the empty UID. The test still exercises the suppression mechanism, but it diverges from production (every real task has a UID) and from the sibling TestSlackTaskReporter_PermanentProgressErrorNotReattempted, which sets UID: "uid-124". Set a UID so the test keys the progressBlocked map the same way production does.

Suggestions (optional)

• [P3] internal/reporting/watcher.go:619 — No test covers a permanent rejection on a terminal transition (succeeded/failed). That path is subtly different: blockProgress is called and then clearProgressCache immediately deletes the entry before the phase is persisted. I traced it and it is harmless (the persisted terminal phase short-circuits all future cycles for the task), but a small test pinning "terminal transition rejected permanently → phase persisted, single attempt" would guard the interaction between the break path and the terminal-phase cleanup.

Key takeaways

• The error classification is verified against slack-go v0.20.0 source: SlackResponse.Err() returns SlackErrorResponse by value (misc.go:180, misc.go:195), so the errors.As value-target match is correct; HTTP 429 surfaces as *RateLimitedError (a distinct type) and API-level rate_limited is excluded by the code check, so rate limits keep the existing retry behavior as claimed.
• Control flow after the permanent-error break was traced end to end: firstReplyTS stays empty so no activity target is set, persistSlackReportingState still runs (and its errors still propagate), and the process-local progressBlocked map is mutex-guarded and swept consistently with the sibling caches. The restart/failover cost (one extra rejected post per task) is documented accurately on the field.
• Test coverage is solid: permanent vs. transient transition errors, progress suppression across cycles including a fresh snapshot text (proving suppression is not mere text deduplication), and wrapped-error classification are all pinned.

[knechtionscoding]

@gjkim42 bump here :)

[knechtionscoding]

@gjkim42 Bump here if you have a chance

[gjkim42]

Lgtm, thanks!

[knechtionscoding]

Unsure if you can rerun E2E @gjkim42 ? Looks like it failed for unrelated reasons

[knechtionscoding]

Unsure what the failure is here or why it is failing to load images into kind. I can rebase, but usnure how you would like to proceed @gjkim42 ?

[gjkim42]

Can you rebase onto origin/main?
Maybe this is related to kelos console renaming.

[knechtionscoding]

@gjkim42 rebased