test(#5707): fix missing harness agent fixtures and timeout diagnostics - #5714
test(#5707): fix missing harness agent fixtures and timeout diagnostics#5714ifireball wants to merge 2 commits into
Conversation
…t diagnostics Behaviour custom harnesses still reference agents/triage.md after scaffold agents were removed (fullsend-ai#5552). Clean pool repos fail with a silent artifact timeout. Commit a minimal agent fixture during harness registration, and fail-fast WaitForHarnessAgent with recent workflow-run diagnostics when a completed non-success run has no artifact. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…-fast Concurrency cancel-in-progress leaves skipped conclusions while a newer fullsend run is still in progress. Treat only failure/timed_out/ startup_failure as terminal so WaitForHarnessAgent keeps waiting. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 1:19 PM UTC · Completed 1:35 PM UTC |
Behaviour CI result
Supports the missing-agent hypothesis from #5707 forensics: committing |
ReviewFindingsMedium
Low
Labels: PR modifies behaviour test infrastructure (pkg/behaviourtest/) and is linked to issue #5707 which carries component/e2e |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| } | ||
| if failed := selectFailedWorkflowRunAfter(d.listRecentRuns(ctx, owner, repo), after); failed != nil { | ||
| return nil, fmt.Errorf("harness agent %q did not complete successfully: workflow run %d concluded with %q%s%s", | ||
| agent, failed.ID, failed.Conclusion, formatWorkflowRunRef(failed), formatRecentRunsDiag(d.listRecentRuns(ctx, owner, repo), after)) |
There was a problem hiding this comment.
[medium] redundant-api-call
d.listRecentRuns(ctx, owner, repo) is called twice in the fail-fast error path: once as input to selectFailedWorkflowRunAfter, and again inside formatRecentRunsDiag within the same fmt.Errorf. This doubles API load and introduces a TOCTOU window where the two calls may return different results.
Suggested fix: Store the result in a local variable and pass the same slice to both selectFailedWorkflowRunAfter and formatRecentRunsDiag.
| } | ||
| if failed := selectFailedWorkflowRunAfter(d.listRecentRuns(ctx, owner, repo), after); failed != nil { | ||
| return nil, fmt.Errorf("harness agent %q did not complete successfully: workflow run %d concluded with %q%s%s", | ||
| agent, failed.ID, failed.Conclusion, formatWorkflowRunRef(failed), formatRecentRunsDiag(d.listRecentRuns(ctx, owner, repo), after)) |
There was a problem hiding this comment.
[low] edge-case
selectFailedWorkflowRunAfter operates on all runs returned by ListRecentWorkflowRuns without filtering by workflow name. An unrelated workflow failure could cause a false fail-fast.
| assert.Equal(t, 99, run.ID) | ||
| } | ||
|
|
||
| func TestWaitForHarnessAgent_FailFastOnFailedRun(t *testing.T) { |
There was a problem hiding this comment.
[low] test-fragility
New tests mutate package-level dispatchWait/dispatchPoll vars. No race with existing t.Parallel() tests (Go serializes sequential tests before releasing parallel tests), but the pattern is fragile if someone later adds t.Parallel().
fullsend-ai-review
Bot
added
requires-manual-review
|
PR mixes too much stuff together |
|
🤖 Finished Retro · ✅ Success · Started 3:37 PM UTC · Completed 3:57 PM UTC |
Retro: PR #5714 — missing harness agent fixtures and timeout diagnosticsTimeline
What went well
Evidence for existing issue
No new proposalsThe primary improvement opportunity (surfacing mixed-scope PR detection as an actionable finding) is already tracked by fullsend#5135. The remaining timeout-diagnostics improvement from the closed PR is a product-level concern for the repo maintainers, not an agent workflow gap. |
1 participant
Summary
agent: agents/triage.md, but scaffold agents were removed in Step 7: Remove scaffold agent fallback infrastructure #5552. Clean pool repos fail at runtime withstat .../.fullsend/agents/triage.md: no such file, upload no artifact, thenWaitForHarnessAgenttimes out opaquely after ~12 minutes..fullsend/when registering a custom harness viagivenCustomHarness(same helper already used for URL-sourced harnesses).WaitForHarnessAgent: fail fast when a completed non-success workflow run exists afterScenarioStartwithout a success artifact, and include recent run id/conclusion/URL diagnostics on timeout.Hypothesis (from main run forensics)
Run https://github.com/fullsend-ai/fullsend/actions/runs/30408887762 leased
halfsend-01/test-repo-09. Harness did run (https://github.com/halfsend-01/test-repo-09/actions/runs/30408975596) and failed on missingtriage.md; no artifact → opaque behaviour timeout. Most pool repos still have stale leftover agents, so failures are intermittent.What behaviour CI should confirm
agents/triage.md.did not complete successfully.Fixes #5707
Test plan
go test ./pkg/behaviourtest/steps/ ./pkg/behaviourtest/drivers/ci/githubactions/make lintok-to-testfor fork PRs)Made with Cursor