test(#6169): suite uses unified Driver for repo allocate/deallocate/finalize - #6172
test(#6169): suite uses unified Driver for repo allocate/deallocate/finalize#6172fullsend-ai-coder[bot] wants to merge 5 commits into
Conversation
…inalize Refactor the behaviour test suite lifecycle so that suite and scenario code speaks only to the unified install.Driver interface (AllocateRepo/DeallocateRepo/Finalize/Capacity) instead of manually composing RepoEnsurer + RepoPool + MintDriver. Key changes: - Rename install.Driver → install.MintDriver to free the Driver name for the unified repo-allocation interface defined in #6135. - Add install.Factory type and unified install.Driver interface with AllocateRepo, DeallocateRepo, Finalize, and Capacity methods. - Add composedDriver (install.NewComposedDriver) that wraps a MintDriver with an internal slot pool and RepoEnsurer. The constructor calls mint.Install, creates internal pool and ensurer, and returns a unified Driver. Finalize handles both outstanding lease reclamation and mint teardown. - Replace World.Ensurer with World.RepoDriver (unified Driver). - Simplify InitScenario(sc, template) — remove the *RepoPool param. AllocateRepo is now called by the "Given the enrolled test repository" step (not the Before hook), so scenarios that never allocate a repo do not consume a pool slot. - afterScenario calls DeallocateRepo (not pool.Release) for cleanup. - suite_test.go creates a MintDriver, wraps it with NewComposedDriver, and passes only the unified Driver to the template. Concurrency > Capacity is now an advisory warning (not fatal) per the #6135 contract. - cfmint.NewDriver and legacy.NewDriver return install.MintDriver. - Update behaviour-drivers.md and behaviour-testing.md API docs. Note: pre-commit could not run (network restriction in sandbox). The post-script runs it authoritatively on the runner. Closes #6169
|
🤖 Finished Review · ✅ Success · Started 10:12 AM UTC · Completed 10:28 AM UTC Commit: |
Site previewPreview: https://06367c3b-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsMedium
Previous run (4)ReviewFindingsMedium
Low
Labels: PR modifies e2e behaviour test infrastructure and install driver packages Next steps:
|
![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
component/e2e
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 10:39 AM UTC · Completed 10:50 AM UTC Commit: |
- Add double-call guard in givenEnrolledTestRepository to prevent slot leaks when the step is called twice in the same scenario - Use errors.Join in Finalize to propagate both leak and teardown errors - Remove unused Factory type from install package - Consolidate advisory warning comment in suite_test.go - Update stale RepoPool/RepoEnsurer references in e2e-testing.md and mint-administration.md to reflect unified install.Driver - Add tests for double-call guard and combined Finalize errors Addresses review feedback on #6172
🔧 Fix agent — iteration 1 (human-triggered)Addressed 6 of 7 review findings with code/doc fixes. Disagreed with 1 (intentional breaking API changes — informational finding requiring no action). Key fix: added double-call guard in givenEnrolledTestRepository to prevent slot leaks. Also improved Finalize error propagation with errors.Join, removed dead Factory type, and updated stale doc references. Fixed (6):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:51 AM UTC · Completed 11:05 AM UTC Commit: |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
|
/fs-fix |
|
🤖 Finished Fix · ❌ Failure · Started 11:07 AM UTC · Completed 11:15 AM UTC Commit: |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/31694023090 Details: Please check the workflow logs for full details and retry with |
|
/fs-fix Pre-commit failed on the previous attempt: |
|
🤖 Finished Fix · ✅ Success · Started 11:17 AM UTC · Completed 11:25 AM UTC Commit: |
Rename the "Lazy create+install (RepoEnsurer)" heading in behaviour-testing.md to "Lazy create+install (AllocateRepo)" to reflect the unified Driver API, and update the fragment links in e2e-testing.md and mint-administration.md to match the new slug. Fixes lint-md-links pre-commit failure from the previous iteration. Addresses review feedback on #6172
🔧 Fix agent — iteration 2 (human-triggered)Updated the RepoEnsurer heading in behaviour-testing.md to AllocateRepo (matching the unified Driver API) and fixed both stale fragment links in e2e-testing.md and mint-administration.md. This resolves the lint-md-links pre-commit failure from iteration 1. Fixed (3):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 11:27 AM UTC · Completed 11:40 AM UTC Commit: |
|
🤖 Finished Fix · ✅ Success · Started 11:42 AM UTC · Completed 11:51 AM UTC Commit: |
Type-assert the returned Driver to *composedDriver and the internal ensurer to *repoEnsurer, then verify that re.e2eCfg.MintURL equals the mint URL from the MintDriver install state. This replaces the previous placeholder comment that acknowledged the by-value limitation but did not actually verify the URL reached the RepoEnsurer. Addresses review feedback on #6172
🔧 Fix agent — iteration 3 (human-triggered)Strengthened TestNewComposedDriver_ThreadsMintURL to type-assert the returned Driver to *composedDriver and inspect the internal *repoEnsurer, verifying that the mint URL from MintDriver install state is correctly threaded to the ensurer's e2eCfg.MintURL field. Left GODOG_CONCURRENCY advisory as t.Logf per #6135 and world.RepoPool removal for a follow-up, as directed. Fixed (1):
Disagreed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 11:52 AM UTC · Completed 12:09 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix Restore and use GoalSuite construction should go through a single Required changes
Do not expand into the full #6170 concrete-driver fold/rename; just land the Factory seam so follow-ups consolidate behind it. |
|
🤖 Finished Fix · ✅ Success · Started 10:01 AM UTC · Completed 10:16 AM UTC Commit: |
Add install.Factory type with the #6135 consolidation shape so suite code selects a Factory and does not compose MintDriver + ensurer + pool itself. Provide cfmint.NewFactory as the first implementation. Update suite_test.go to use the Factory instead of direct cfmint.NewDriver + install.NewComposedDriver composition. Add install.StateProvider interface so the composed Driver can expose its install state without threading State through the Factory return value. Update docs to reflect Factory-based construction. Addresses review feedback on #6172
🔧 Fix agent — iteration 4 (human-triggered)Restored install.Factory type with the #6135 consolidation shape. Added cfmint.NewFactory as the first implementation. Updated suite_test.go to use Factory-based construction instead of direct MintDriver + NewComposedDriver composition. Added StateProvider interface so the Driver exposes install state without threading it through the Factory return. Updated docs to reflect the new pattern. NewComposedDriver is retained as an internal helper used by Factory implementations. Fixed (6):
Tests: passed
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:17 AM UTC · Completed 10:31 AM UTC Commit: |
| t.Fatalf("installing fullsend on %s: %v", org, err) | ||
| // Advisory: excess workers block in AllocateRepo (#6135). | ||
| if concurrency > driver.Capacity() { | ||
| t.Logf("WARNING: GODOG_CONCURRENCY=%d exceeds driver capacity %d; excess workers may block in AllocateRepo", concurrency, driver.Capacity()) |
There was a problem hiding this comment.
[low] error-handling
The guard for GODOG_CONCURRENCY exceeding driver capacity was downgraded from t.Fatalf (hard failure) to t.Logf (advisory warning). While this is intentional per the #6135 contract (AllocateRepo blocks until a slot frees), excess goroutines will block until the go test timeout fires, producing a confusing timeout failure. The log warning appears early in output and may be missed.
Suggested fix: Consider using t.Errorf instead of t.Logf so the misconfiguration is visible in test results as a failure, even though the run continues.
| allocated []string | ||
| deallocated []string | ||
| allocateErr error | ||
| deallocateRv error |
There was a problem hiding this comment.
[low] naming-consistency
The field deallocateRv on fakeDriver uses the Rv (return value) suffix, which is inconsistent with the codebase's established convention for error fields in test fakes. Every other test fake uses Err (e.g., installErr, teardownErr, allocateErr on the same struct).
Suggested fix: Rename deallocateRv to deallocateErr for consistency.
| // | ||
| // Renamed from Driver so that the Driver name can refer to the unified | ||
| // repo-allocation interface (#6135). | ||
| type MintDriver interface { |
There was a problem hiding this comment.
[low] naming-consistency
The interface rename from Driver to MintDriver inverts the typical package convention where Driver is the primary interface name (e.g., scm.Driver, ci.Driver). The current naming gives the simpler name to the new unified interface, which is the primary abstraction callers use — a reasonable design choice.
|
/fs-fix Drop Context
Required changes
Keep the |
|
🤖 Finished Fix · ❌ Failure · Started 11:25 AM UTC · Completed 11:37 AM UTC Commit: |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/31944245704 Details: |
|
/fs-fix Retry the previous fix, but rebase onto latest WhyThe last attempt did the right code work (drop Required
Do not expand into |
|
/fs-retro Investigate why this PR ( Please look at:
Keep findings actionable for fullsend agent/dispatch behavior, not just this one PR’s content. |
|
🤖 Finished Fix · ❌ Failure · Started 11:46 AM UTC · Completed 11:57 AM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 11:47 AM UTC · Completed 12:07 PM UTC Commit: |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/31945204416 Details: |
Retro: PR #6172 — Concurrent sibling code agents and stale-base rework chainWhat happenedOn Aug 13, ifireball created two sibling issues (#6168, #6169) as children of epic #6135, 2 seconds apart. Both were triaged and dispatched to concurrent code agents within 5 seconds (runs 31687582369 and 31687588304), both forking from the same Because PR #6172 was built on a pre-#6171 base, it lacked the step config cleanup from #6168, leading to a chain of rework:
Additionally, the review agent re-raised the same Root causes
Evidence for existing open issues
AssessmentNo novel systemic gaps were identified beyond what existing open issues already capture. The workflow's rework chain was caused by a known set of gaps: concurrent dispatch without coordination (fullsend#5795), fix agent not rebasing (agents#565), inflated post-fix scan range (agents#318), and review agent re-raising dismissed findings (agents#106, fullsend#4682). Resolving agents#565 alone would have prevented the most expensive part of the rework (iterations 4–6). |
|
Closing: fix-agent pushes keep failing post-script gitleaks over a large commit window after rebase/merge onto main (file-scoped scans clean; nothing lands). Restarting from a fresh |
|
🤖 Finished Retro · ✅ Success · Started 1:29 PM UTC · Completed 2:17 PM UTC Commit: |
Retro: PR #6172 (closed without merging)What happenedPR #6172 implemented issue #6169 (unified Four human-triggered By Aug 16, the branch was 108 commits behind Total cost: 1 code run, 4 review runs, ~8 fix runs, 3 days of human interaction — all discarded. Root causes and existing issue coverage1. Gitleaks SCAN_RANGE breaks after rebase (direct cause of closure) —
2. Stale base from concurrent sibling dispatch — Issues #6168 and #6169 were dispatched to concurrent code agents within 5 seconds from the same SHA. #6168 merged first, leaving #6172 behind with overlapping changes. The fix agent later added an unnecessary
3. Review agent repeated dismissed findings — The GODOG_CONCURRENCY advisory finding was posted 3 times across reviews 2-4 despite the human explicitly dismissing it each time with the same rationale. The MintDriver naming finding appeared 2-3 times similarly. The review agent regenerated each finding with slightly evolving wording, suggesting stateless regeneration rather than copy-paste.
Review qualityThe review agent's finding quality was strong. The double-call guard regression was a genuine functional bug that could cause pool slot leaks. Dead-code, error-handling, and test-adequacy findings were all valid and addressed. The agent approved appropriately after each fix iteration. Human interventions were limited to architectural direction (restoring No new proposalsAll identified root causes are tracked by existing open issues. This retro provides additional supporting evidence — particularly for fullsend#6259 and agents#318, where PR #6172 represents the second documented instance of the SCAN_RANGE bug causing complete PR abandonment. |
1 participant
Summary
Refactor the behaviour test suite lifecycle so that suite and scenario code speaks only to the unified
install.Driverinterface (AllocateRepo/DeallocateRepo/Finalize/Capacity) instead of manually composingRepoEnsurer+RepoPool+MintDriver. This is the child of #6135 that consolidates the suite/scenario lifecycle surface.Changes
install.Driver→install.MintDriverto free theDrivername for the unified repo-allocation interfaceinstall.Driverinterface withAllocateRepo,DeallocateRepo,Finalize,Capacity(matching the test(behaviour): consolidate install driver, RepoEnsurer, and RepoPool behind one driver interface #6135 contract)install.Factorytype for future driver constructioncomposedDriver(install.NewComposedDriver) that wraps aMintDriverwith an internal slot pool andRepoEnsurer. The constructor callsmint.Install, creates internal pool and ensurer, and returns a unifiedDriverWorld.EnsurerwithWorld.RepoDriver(unifiedDriver)InitScenario(sc, template)— remove the*RepoPoolparam.AllocateRepois now called by theGiven the enrolled test repositorystep (not the Before hook), so scenarios that never allocate a repo don't consume a pool slotafterScenariocallsDeallocateRepoinstead ofpool.Releasesuite_test.gocreates aMintDriver, wraps it withNewComposedDriver, and passes only the unifiedDriverto the templatecfmint.NewDriverandlegacy.NewDriverreturninstall.MintDriverTesting
pkg/behaviourtest/...tests pass with-racecomposed.gofunctionse2e/behaviour/suite_test.gocompiles with-tags behaviourgo vet ./pkg/behaviourtest/...passesgo build ./...passesCloses #6169
Post-script verification
agent/6169-unified-driver-lifecycle)fafb2df30a66eb2dfa0ae9e4dff6641bfebe41a4..HEAD)