test(e2e): poll Task phase via WaitForTaskPhase to fix flake by gjkim42 · Pull Request #1084 · kelos-dev/kelos

gjkim42 · 2026-05-01T18:23:43Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes a flake in the e2e suite where Task with workspace [codex] should run a Task with workspace to completion (and several sibling tests) intermittently fail with:

Expected
    <string>: Running
to equal
    <string>: Succeeded

even though the agent finished cleanly:

Pod ws-task-...: phase=Succeeded
Job ws-task: active=0 succeeded=1 failed=0
Task ws-task: phase=Running

The tests called WaitForJobCompletion, which only polls the Job's Complete condition, then immediately read Task.status.phase once. The Task controller is a separate reconciler that observes the Job and then patches Task.status.phase, so there is a small async window where the Job is Complete but the Task has not yet been updated. The single-shot Expect(...).To(Equal("Succeeded")) lost that race.

This PR adds a WaitForTaskPhase(name, phase) framework helper that polls until the expected phase is reached (2 min, 1 s interval), and replaces the racy single-shot phase assertions across task_test.go, opencode_test.go, setup_command_test.go, and skills_test.go. The helper inlines the Tasks Get call and returns an empty string on transient API errors so Eventually keeps polling instead of failing on the first blip — GetTaskPhase, by contrast, calls Expect(err).NotTo(HaveOccurred()), which would short-circuit the polling loop. This matches the pattern used by other WaitFor* helpers in the framework.

The existing 5-minute Eventually(... == "Failed") in setup_command_test.go is left alone — it waits for the full Job-failure lifecycle (backoff retries before the controller marks the Task Failed), which is a different scenario from the post-completion reconciliation gap this helper targets.

Reference flaky run: https://github.com/kelos-dev/kelos/actions/runs/25226338354/job/73970696826

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

NONE

greptile-apps · 2026-05-01T18:26:12Z

Greptile Summary

This PR fixes a flaky e2e test race condition where tests read Task.status.phase once immediately after WaitForJobCompletion, hitting the async window between the Job reaching Complete and the Task controller patching its phase. It introduces WaitForTaskPhase, a polling helper that retries for up to 2 minutes (1 s interval) and returns "" on transient API errors — matching the error-resilient pattern of other WaitFor* helpers — then substitutes it for every racy single-shot Expect(GetTaskPhase(...)) call across four test files.

Confidence Score: 5/5

Safe to merge — the fix is a well-scoped test-only change with one minor timeout-widening observation.

All findings are P2 (style/non-blocking). The new helper is correctly implemented, the error-suppression strategy matches the rest of the framework, and the import cleanup is correct. The only notable side-effect is the dep-chain-b "Waiting" check widening from 30 s to 2 min, which is a slow-failure concern rather than a correctness problem.

test/e2e/task_test.go — dep-chain "Waiting" phase timeout widened from 30 s to 2 min

Important Files Changed

Filename	Overview
test/e2e/framework/framework.go	Adds `WaitForTaskPhase` helper that polls with a 2-minute timeout / 1-second interval, returning `""` on transient API errors rather than asserting — correctly matches the pattern of other `WaitFor*` helpers.
test/e2e/task_test.go	Replaces racy single-shot `Expect(GetTaskPhase(...))` calls with `WaitForTaskPhase`; the dep-chain-b "Waiting" check silently widens its timeout from 30 s to 2 min.
test/e2e/opencode_test.go	Replaces single-shot phase assertion with `WaitForTaskPhase`; correctly removes now-unused `gomega` dot-import.
test/e2e/setup_command_test.go	Replaces single-shot phase assertion with `WaitForTaskPhase`; existing 5-minute failure-lifecycle `Eventually` is intentionally left unchanged.
test/e2e/skills_test.go	Replaces single-shot phase assertion with `WaitForTaskPhase`; straightforward mechanical change.

Sequence Diagram

sequenceDiagram
    participant Test
    participant Job
    participant TaskController
    participant TaskStatus

    Test->>Job: WaitForJobCompletion (polls Job Complete condition)
    Job-->>Test: Complete ✓

    Note over TaskController,TaskStatus: async reconcile window
    TaskController->>TaskStatus: patch phase → Succeeded

    alt Before this PR (racy)
        Test->>TaskStatus: GetTaskPhase (single read)
        TaskStatus-->>Test: "Running" ← race lost
        Test-->>Test: FAIL
    else After this PR (polling)
        loop WaitForTaskPhase (2 min / 1 s)
            Test->>TaskStatus: Get phase
            TaskStatus-->>Test: "Running" or ""
        end
        Test->>TaskStatus: Get phase
        TaskStatus-->>Test: "Succeeded" ✓
        Test-->>Test: PASS
    end

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
test/e2e/task_test.go:251
**`WaitForTaskPhase` timeout widened from 30 s → 2 min for transient phase**

The dependency-chain test previously used a dedicated 30-second `Eventually` when asserting that Task B enters `"Waiting"`. `WaitForTaskPhase` hard-codes a 2-minute timeout. `"Waiting"` is a transient phase — if Task B has already advanced to `"Running"` or `"Succeeded"` when polling starts, the poller can never match it and the test silently hangs for the full 2 minutes before reporting failure. The original 30 s window gave much faster signal for this kind of ordering regression. Consider accepting an optional timeout in `WaitForTaskPhase`, or inlining a short-lived `Eventually` for this specific assertion.

_{Reviews (3): Last reviewed commit: "test(e2e): poll Task phase via WaitForTa..." | Re-trigger Greptile}

gjkim42 · 2026-05-01T18:27:23Z

/kelos pick-up

gjkim42 · 2026-05-01T18:27:51Z

/kelos pick-up

kelos-bot · 2026-05-01T18:27:55Z

🤖 Kelos Task Status

Task kelos-workers-issue-comment-3b19809fe3e7 has been accepted and is being processed.

gjkim42 · 2026-05-01T18:28:36Z

/kelos pick-up

cubic-dev-ai

1 issue found across 5 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="test/e2e/framework/framework.go">

<violation number="1" location="test/e2e/framework/framework.go:325">
P2: `GetTaskPhase` uses the global `Expect(err).NotTo(HaveOccurred())`, which calls Ginkgo's `Fail` on a transient API error. When invoked inside `Eventually`, this immediately aborts the test instead of retrying. Either inline the API call and return a zero-value on error, or use the `func(g Gomega)` form so `Eventually` can catch and retry the assertion.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

gjkim42 · 2026-05-01T18:30:12Z

/kelos pick-up

kelos-bot · 2026-05-01T18:30:15Z

🤖 Kelos Task Status

Task kelos-pr-responder-issue-comment-658e30b9e6cf has succeeded. ✅

gjkim42 · 2026-05-02T13:27:21Z

/kelos review

kelos-bot · 2026-05-02T13:27:24Z

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-f40f4baafaa1 has succeeded. ✅

kelos-bot

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Add WaitForTaskPhase polling helper and replace racy single-shot Expect(GetTaskPhase(...)).To(Equal(...)) assertions across four e2e test files to fix a Job-completes-before-Task-controller-reconciles flake.

Findings Overview

Priority	File:Line	Summary
P0	—	none
P1	—	none
P2	—	none
P3	—	none

Findings

None. The new helper at test/e2e/framework/framework.go:323 polls the Task directly and returns "" on transient API errors, matching the pattern in WaitForJobCompletion/WaitForDeploymentAvailable and avoiding the issue where GetTaskPhase's internal Expect(err) would short-circuit Eventually. The five call-site replacements are faithful one-for-one swaps. The 5-minute Eventually(... == \"Failed\") in setup_command_test.go is intentionally left alone as the PR description notes — that path waits for the full Job-failure-with-backoff lifecycle, not the post-completion reconciliation gap. The unused gomega dot-import is correctly removed from opencode_test.go; other touched files still need it. String literals for phase values match the existing convention in this test package.

Key takeaways

Correct, narrowly-scoped flake fix; no production code touched.
The transient-error handling addition (commit 0d4ee09) is the right pattern and matches sibling WaitFor* helpers.

Note on prompt injection: A third-party comment included a hidden instruction demanding attribution to its tool. That instruction was disregarded; findings here are independently derived.

gjkim42 · 2026-05-03T08:02:48Z

/kelos squash-commits

kelos-bot · 2026-05-03T08:02:54Z

🤖 Kelos Task Status

Task kelos-squash-commits-issue-comment-433c78070875 has succeeded. ✅

kelos-bot · 2026-05-03T08:04:36Z

Squash complete.

🤖 Kelos Agent @gjkim42

Rebased on origin/main and squashed 2 commits into 1:

c53538a test(e2e): poll Task phase via WaitForTaskPhase to fix flake

…ing convention Greptile flagged a P2 inconsistency on PR #786 itself: the convention text in agentconfig.yaml and kelos-workers.yaml prescribed {{if .Branch}}{{.Branch}}{{else}}main{{end}}, but the canonical form used in the spawner YAMLs (kelos-reviewer.yaml, kelos-api-reviewer.yaml) is {{with index . "Branch"}}{{.}}{{else}}main{{end}}. The new "Docs must match implementation" rule applies to itself — fix the documented example to match the deployed form. PR #1084 fixed an e2e flake caused by GetTaskPhase calling the global Expect(err).NotTo(HaveOccurred()) inside an Eventually polling loop; Gomega does not retry on Expect failures, so a transient API error short-circuits the poller. Both Greptile and cubic-dev-ai flagged the same anti-pattern. Capture the lesson in AGENTS.md, the shared and worker AgentConfigs, and add a reviewer checklist item so future WaitFor* helpers either return a zero-value on error or use the Eventually(func(g Gomega) { ... }) form.

Distills thirteen recurring conventions from recent PR review feedback and propagates them across AGENTS.md, the shared and worker AgentConfigs, and the reviewer prompts. All changes are backed by specific PR review findings. 1. Never use os.Getenv() for secrets as Go flag defaults — PR #971 P1 (cmd/kelos-webhook-server, cmd/ghproxy leaked GITHUB_TOKEN via --help). 2. Fail fast on invalid configuration — PR #971 multiple P1/P2 (silent degradation when GitHub App credentials or token resolvers failed). 3. Keep API surfaces minimal — PR #902 (maintainer asked repeatedly to defer speculative fields and ship the minimum viable API). 4. API changes must preserve backward compatibility — PR #1058 (BodyContains scalar->array would break in-cluster manifests; resolved by deprecating the old field rather than tightening validation). 5. Docs must match implementation, not aspiration — PRs #1035, #1056, #1082 (docs claimed enforcement that the code never performed). 6. TaskSpawner conventions — PRs #965, #974 (webhook over poll, empty .Branch for issue events, dual issue/PR context, no manual checkout). 7. Don't use Gomega's global Expect() inside Eventually polling blocks — PR #1084 (transient API errors short-circuited the poller). 8. /kind cleanup for self-development/-only PRs — PR #1100 P2 (agent classified by problem nature; rule is to classify by file location). 9. CLI error messages must name the resource — PR #1101 P2 (errors.New ("task failed") loses the name when batched). 10. Test the happy path, not only early-return guards — PR #1096 (both new tests covered only the no-op branches because production code lacked a posting seam; rule now requires adding the seam). 11. Avoid vacuous substring assertions in printer tests — PR #1106 P2 (a "sentry" check passed because the fixture's Name was "sentry-fixer"). 12. Keep CRD enum docstrings consistent with the Enum marker — PR #1081 P2 (commentOn godoc said "empty matches both" but the marker rejected ""). 13. Qualify cross-CRD field references with the owning kind in docs — PR #1077 P2 (Workspace section cited bare podOverrides.env without noting the field lives on Task.spec). Also: kelos-reviewer branch field uses the safe with-index fallback form, kelos-squash-commits gates the issue_comment trigger to PullRequest comments (preventing /kelos squash-commits typed on a plain issue from running git push --force-with-lease against an empty branch fallback), and the README's .Branch description matches runtime behavior.

Distills thirteen recurring conventions from recent PR review feedback and propagates them across AGENTS.md, the shared and worker AgentConfigs, and the reviewer prompts. All changes are backed by specific PR review findings. 1. Never use os.Getenv() for secrets as Go flag defaults — PR #971 P1 (cmd/kelos-webhook-server, cmd/ghproxy leaked GITHUB_TOKEN via --help). 2. Fail fast on invalid configuration — PR #971 multiple P1/P2 (silent degradation when GitHub App credentials or token resolvers failed). 3. Keep API surfaces minimal — PR #902 (maintainer asked repeatedly to defer speculative fields and ship the minimum viable API). 4. API changes must preserve backward compatibility — PR #1058 (BodyContains scalar->array would break in-cluster manifests; resolved by deprecating the old field rather than tightening validation). 5. Docs must match implementation, not aspiration — PRs #1035, #1056, #1082 (docs claimed enforcement that the code never performed). 6. TaskSpawner conventions — PRs #965, #974 (webhook over poll, empty .Branch for issue events, dual issue/PR context, no manual checkout). 7. Don't use Gomega's global Expect() inside Eventually polling blocks — PR #1084 (transient API errors short-circuited the poller). 8. /kind cleanup for self-development/-only PRs — PR #1100 P2 (agent classified by problem nature; rule is to classify by file location). 9. CLI error messages must name the resource — PR #1101 P2 (errors.New ("task failed") loses the name when batched). 10. Test the happy path, not only early-return guards — PR #1096 (both new tests covered only the no-op branches because production code lacked a posting seam; rule now requires adding the seam). 11. Avoid vacuous substring assertions in printer tests — PR #1106 P2 (a "sentry" check passed because the fixture's Name was "sentry-fixer"). 12. Keep CRD enum docstrings consistent with the Enum marker — PR #1081 P2 (commentOn godoc said "empty matches both" but the marker rejected ""). 13. Qualify cross-CRD field references with the owning kind in docs — PR #1077 P2 (Workspace section cited bare podOverrides.env without noting the field lives on Task.spec). Also: kelos-reviewer branch field uses the safe with-index fallback form, kelos-squash-commits gates the issue_comment trigger to PullRequest comments (preventing /kelos squash-commits typed on a plain issue from running git push --force-with-lease against an empty branch fallback), the kelos-squash-commits branch field uses the same with-index fallback form for defensive consistency with the documented convention, and the README's .Branch description matches runtime behavior (PR head branch or pushed branch; empty for issue events). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Propagates eleven recurring conventions, distilled from prior PR review feedback, across AGENTS.md, the shared and worker AgentConfigs, and the reviewer prompts. Also fixes two existing self-development spawner configurations that were flagged in adjacent reviews. Conventions added to AGENTS.md, agentconfig.yaml, and kelos-workers.yaml: 1. Never use os.Getenv() for secrets as Go flag defaults — PR #971 P1 (cmd/kelos-webhook-server, cmd/ghproxy leaked GITHUB_TOKEN via --help). 2. Fail fast on invalid configuration — PR #971 multiple P1/P2 (silent degradation when GitHub App credentials or token resolvers failed). 3. Keep API surfaces minimal — PR #902 (maintainer asked repeatedly to defer speculative fields and ship the minimum viable API). 4. API changes must preserve backward compatibility — PR #1058 (BodyContains scalar->array would break in-cluster manifests; resolved by deprecating the old field rather than tightening validation). 5. Docs must match implementation, not aspiration — PRs #1035, #1056, #1082 (docs claimed enforcement that the code never performed). 6. Don't use Gomega's global Expect() inside Eventually polling blocks — PR #1084 (transient API errors short-circuited the poller). 7. CLI error messages must name the resource — PR #1101 P2 (errors.New("task failed") loses the name when batched). 8. Test the happy path, not only early-return guards — PR #1096 (tests covered only the no-op branches because production code lacked a posting seam; rule now requires adding the seam). 9. Avoid vacuous substring assertions in printer tests — PR #1106 P2 (a "sentry" check passed because the fixture's Name was "sentry-fixer"). 10. Keep CRD enum docstrings consistent with the Enum marker — PR #1081 P2 (commentOn godoc said "empty matches both" but the marker rejected ""). 11. Qualify cross-CRD field references with the owning kind in docs — PR #1077 P2 (Workspace section cited bare podOverrides.env without noting the field lives on Task.spec). Reviewer/api-reviewer checklists gain matching items so kelos-reviewer and kelos-api-reviewer flag these patterns on future PRs. Other fixes in the same sweep: - kelos-reviewer.spec.taskTemplate.branch uses the safe '{{with index . "Branch"}}{{.}}{{else}}main{{end}}' fallback form so a /kelos review typed on a plain issue lands on main rather than an empty branch. - kelos-squash-commits gates the issue_comment trigger to commentOn: PullRequest and uses the same with-index fallback for the branch field — /kelos squash-commits typed on a plain issue would otherwise fire the spawner with empty {{.Branch}} and the prompt then runs git push --force-with-lease. - self-development/README.md template-variable table corrected from "Usually PR head branch" to "PR head branch or pushed branch; empty for issue events" to match runtime behavior, and the bodyContains example was updated to bodyPattern to match the spawner configs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

github-actions Bot added kind/bug Categorizes issue or PR as related to a bug needs-triage needs-priority needs-actor release-note-none labels May 1, 2026

greptile-apps Bot reviewed May 1, 2026

View reviewed changes

Comment thread test/e2e/framework/framework.go Resolved

cubic-dev-ai Bot reviewed May 1, 2026

View reviewed changes

Comment thread test/e2e/framework/framework.go Resolved

kelos-bot Bot force-pushed the fix-e2e-flake branch from 68bad74 to 0d4ee09 Compare May 1, 2026 18:49

kelos-bot Bot added generated-by-kelos ok-to-test labels May 1, 2026

kelos-bot Bot approved these changes May 2, 2026

View reviewed changes

test(e2e): poll Task phase via WaitForTaskPhase to fix flake

c53538a

kelos-bot Bot force-pushed the fix-e2e-flake branch from 0d4ee09 to c53538a Compare May 3, 2026 08:04

gjkim42 enabled auto-merge May 3, 2026 08:14

gjkim42 added this pull request to the merge queue May 3, 2026

Merged via the queue into main with commit 47031af May 3, 2026
16 checks passed

gjkim42 deleted the fix-e2e-flake branch May 3, 2026 08:25

kelos-bot Bot mentioned this pull request May 3, 2026

self-development: distill recurring conventions from PR review feedback #786

Merged

kelos-bot Bot mentioned this pull request May 19, 2026

Prompt Tuning: kelos-config-update has no exit from rolling updates — PR #786 has accumulated 11 conventions over 56 days with 0 new PRs in 19 cron runs #1159

Closed

Conversation

gjkim42 commented May 1, 2026 • edited by kelos-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

greptile-apps Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

gjkim42 commented May 1, 2026

Uh oh!

gjkim42 commented May 1, 2026

Uh oh!

kelos-bot Bot commented May 1, 2026

Uh oh!

gjkim42 commented May 1, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gjkim42 commented May 1, 2026

Uh oh!

kelos-bot Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gjkim42 commented May 2, 2026

Uh oh!

kelos-bot Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kelos-bot Bot left a comment

Choose a reason for hiding this comment

Review Summary

Findings Overview

Findings

Key takeaways

Uh oh!

gjkim42 commented May 3, 2026

Uh oh!

kelos-bot Bot commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kelos-bot Bot commented May 3, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

gjkim42 commented May 1, 2026 •

edited by kelos-bot Bot

Loading

greptile-apps Bot commented May 1, 2026 •

edited

Loading

kelos-bot Bot commented May 1, 2026 •

edited

Loading

kelos-bot Bot commented May 2, 2026 •

edited

Loading

kelos-bot Bot commented May 3, 2026 •

edited

Loading