ci(#6815): migrate behaviour tests to single-org ephemeral repos - #6820
ci(#6815): migrate behaviour tests to single-org ephemeral repos#6820fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
Site previewPreview: https://b45b4b73-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ✅ Success · Started 7:39 PM UTC · Completed 8:00 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.17 |
|
Risk Assessment: elevated (3/5) DetailsTier 1 signals unchanged from prior assessment: large blast radius (22 files, 722 lines, score 5), CI workflow edit on protected path (score 4), bot author; Tier 2 confirms high fix density and churn across e2e infrastructure files; Tier 3 mitigates — well-scoped type/chore with no production code impact, resolving known token-exhaustion and permission-race issues (#6702, #6701). Previous runRisk Assessment: elevated (3/5) DetailsTier 1 signals unchanged from prior assessment: large blast radius (score 5) across 22 files, CI workflow change (score 4), protected path edit (score 3). Tier 2 confirms high fix/revert density and active churn. Tier 3 remains low — well-scoped test-infrastructure chore with no production impact. Previous run (2)Risk Assessment: elevated (3/5) DetailsScore holds at 3 (elevated), unchanged from prior assessment. Tier 1 signals are identical: large blast radius (score 5) across 21 files in the install driver package, CI workflow change (score 4), and protected path edit (score 3). Tier 2 reinforces elevation with high fix/revert density (avg 7.9 commits/file in 90d, score 5) and active churn (avg 7.67 commits/file in 30d). Tier 3 remains low (1.5) — the change is well-scoped to its issue, test-infrastructure-only, and safely revertible. Previous run (3)Risk Assessment: elevated (3/5) DetailsScore holds at 3 (elevated), unchanged from prior assessment. PR is now 20 files / ~700 lines. Core risk drivers persist: large blast radius (score 5) across the install driver package, CI workflow change (score 4) to e2e.yml, and high Tier 2 churn in key files (ensure.go, e2e.yml, suite_test.go). Tier 3 remains low because the change is well-scoped to its issue, test-infrastructure-only, and safely revertible. Previous run (4)Risk Assessment: elevated (3/5) DetailsScore holds at 3 (elevated) after re-review. PR narrowed modestly from 21 to 19 files and 758 to 688 lines, but the core risk drivers persist: large blast radius (score 5) across the install driver package, CI workflow change (score 4) to e2e.yml, and exceptionally high Tier 2 churn — key files like ensure.go (10 fix commits), e2e.yml (24 fix commits), and suite_test.go (9 fix commits) indicate an actively unstable area. Tier 3 is low (1.67) because the change is well-scoped to its issue, test-infrastructure-only, and safely revertible. Previous run (5)Risk Assessment: elevated (3/5) DetailsScore remains 3 (elevated) despite significant PR narrowing (51 to 21 files, 2155 to 758 lines, 4 to 1 protected paths). Large blast radius (score 5), CI workflow change (score 4), and high Tier 2 churn (fix/revert counts of 10-26 across key files) keep the composite at elevated. Test infrastructure scope and good issue alignment provide Tier 3 mitigation. Previous run (6)Risk Assessment: elevated (3/5) DetailsRe-review anchoring: only 2 doc files changed since prior review and all Tier 1 signals remain unchanged (51 files, 2155 lines, large blast radius, 4 protected paths, CI workflows changed). Prior score of 3 (elevated) preserved per anchoring policy. Underlying risk driven by size exceeding 50-file and 2000-line thresholds, four protected path changes, and high recent churn/regression density in the touched files. Mitigated by bot authorship, no security-sensitive files, net code deletion, and strong alignment with issue acceptance criteria. Score unchanged from prior assessment. Previous run (7)Risk Assessment: elevated (3/5) DetailsLarge refactoring PR (51 files, 2141 lines, net -705) migrating behaviour tests from 12-org pool to single-org ephemeral repos. Elevated risk driven by size (exceeds 50-file and 2000-line thresholds), four protected path changes (CI workflows, scaffold templates), and high recent churn/regression density in the touched files. Mitigated by bot authorship, no security-sensitive files, net code deletion, and strong alignment with issue acceptance criteria. Score unchanged from prior assessment. Previous run (8)Risk Assessment: elevated (3/5) DetailsScore increased from 2 to 3: PR grew from 19 files/609 lines to 50 files/2134 lines with production code changes (FetchURL retry removal, dispatch routing, pipeline schedule removal). Four protected paths and CI workflows changed. Previous run (9)Risk Assessment: moderate (2/5) DetailsMedium-sized PR (19 files, 609 lines) confined to behaviour test infrastructure, documentation, and e2e helpers with no production code impact. Git history shows moderate churn in the behaviourtest/drivers/install package. Issue scope aligns well with the PR and rollback is straightforward. Consistent with prior assessment. Previous run (10)Risk Assessment: moderate (2/5) DetailsMedium-sized PR (19 files, 576 lines) confined to behaviour test infrastructure, documentation, and e2e helpers with no production code impact. Git history shows moderate churn in the behaviourtest/drivers/install package. Issue scope aligns well with the PR and rollback is straightforward. Consistent with prior assessment. Previous run (11)Risk Assessment: moderate (2/5) DetailsMedium-sized PR (17 files, 518 lines) confined to test infrastructure and documentation with no production code impact. High recent fix/revert churn in the change area elevates the git-history regression dimension, but the Tier 2 composite remains moderate due to low code-age, revert-frequency, and sentiment scores. Issue scope aligns well with the PR and rollback is straightforward, keeping the overall risk at moderate. Previous run (12)Risk Assessment: moderate (2/5) DetailsMedium-sized PR (14 files, 483 lines) confined to test infrastructure and documentation with no production code impact. High recent churn in the change area elevates git-history risk, but issue scope aligns well with the PR and rollback is straightforward. Previous run (13)Risk Assessment: moderate (2/5) DetailsMedium-sized PR (11 files, 304 lines) confined to test infrastructure and documentation with no production code impact. High recent churn in the change area elevates git-history risk, but issue scope aligns well with the PR and rollback is straightforward. |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (5)ReviewFindingsMedium
Low
Next steps:
Previous run (6)ReviewFindingsMedium
Low
Next steps:
Previous run (7)ReviewFindingsMedium
Low
Next steps:
Previous run (8)ReviewFindingsCritical
Medium
Low
Next steps:
Previous run (9)ReviewFindingsMedium
Low
Next steps:
Previous run (10)ReviewFindingsLow
Previous run (11)ReviewFindingsMedium
Low
Next steps:
Previous run (12)ReviewFindingsMedium
Low
Next steps:
Previous run (13)ReviewAll prior findings from the previous review are resolved: the race condition in The remaining findings are documentation files outside this PR that still reference the old pool model. FindingsHigh
Medium
Low
Next steps:
Previous run (14)ReviewFindingsHigh
Low
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 8:02 PM UTC · Completed 8:14 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.82 |
- Fix race condition in DeallocateRepo: delete ephemeral repo BEFORE
returning the slot to the pool channel, preventing another goroutine
from re-allocating a repo that is still being deleted.
- Fix stale buildRepoList: rename to buildEphemeralRepoList and thread
the UUID prefix from NewRepoPoolCFMintPreviews so mint deploy uses
the same bt-{uuid}-{slot} names as the composed driver.
- Fix ensurer cache leak: add InvalidateCache to ensurer interface,
called by DeallocateRepo after deletion so re-allocated slots
trigger a fresh create+install cycle.
- Fix stale doc: update Pool-org prerequisites to reference
bt-{uuid}-{slot} instead of test-repo-NN.
- Add tests for DeallocateRepo deletion path with fake forge.Client
(delete, keepRepos, notFound, cache invalidation).
- Fix unchecked type assertion in ValidatePerRepoPostInstallRefPinned.
- Update DefaultPoolSize comment to reflect ephemeral slot count.
- Fix RunReposInstall to pass --inference-project/--inference-wif-provider
flags, matching the sibling RunGitHubSetup pattern.
Addresses #6820
🔧 Fix agent — iteration 1 (bot-triggered)Addressed all 8 review findings: fixed race condition in DeallocateRepo (delete before pool return), fixed stale buildRepoList naming for CF mint deploy, added ensurer cache invalidation after repo deletion, updated stale doc reference, added 4 deletion-path tests with fake forge.Client, used checked type assertion in validation, updated stale comment, and threaded inference flags through RunReposInstall. Fixed (8):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:16 PM UTC · Completed 8:34 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.65 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 8:36 PM UTC · Completed 8:47 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.80 |
🔧 Fix agent — iteration 2 (bot-triggered)Updated three documentation files (e2e-testing.md, behaviour-drivers.md, ADR 0066) to replace stale halfsend pool-model references with the new fullsend-ai-test ephemeral repo model. Fixed unchecked type assertion in ValidatePerRepoPostInstall. Disagreed with the error-handling finding on DeallocateRepo — the current log-and-continue behaviour is the correct trade-off. Fixed (4):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
Update e2e-testing.md, behaviour-drivers.md, and ADR 0066 to reflect
the migration from halfsend org pool with test-repo-NN naming to the
single fullsend-ai-test org with ephemeral bt-{uuid}-{slot} repos.
Also fix unchecked type assertion in ValidatePerRepoPostInstall to
match the checked pattern used by ValidatePerRepoPostInstallRefPinned.
Note: pre-commit hooks could not run (network-restricted sandbox).
gofmt, go vet, trailing-whitespace, and end-of-file checks passed
via direct execution.
Addresses #6820
|
🤖 Finished Review · ✅ Success · Started 8:49 PM UTC · Completed 9:10 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.85 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 9:12 PM UTC · Completed 9:22 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.04 |
- Update mint-administration.md to separate admin e2e enrollment from behaviour test enrollment, noting ephemeral repos in fullsend-ai-test - Add keepRepos bool parameter to newComposedDriver, moving os.Getenv read to factory functions for consistency with other env vars - Add unit tests for ValidatePerRepoPostInstallRefPinned (OK and missing-shim paths) - Surface non-NotFound DeleteRepo errors from DeallocateRepo to callers instead of silently swallowing them; slot is still returned to pool Pre-commit hooks could not run (network-restricted sandbox). gofmt, go vet, and package tests passed via direct execution. Addresses #6820
|
🤖 Finished Review · ✅ Success · Started 3:42 AM UTC · Completed 4:02 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.71 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
978d35d to
b0a96a3
Compare
|
🤖 Finished Review · ✅ Success · Started 4:06 AM UTC · Completed 4:26 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.22 |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
b0a96a3 to
3c3e968
Compare
|
🤖 Finished Review · ✅ Success · Started 4:33 AM UTC · Completed 4:54 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.49 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3c3e968 to
22a30d8
Compare
|
🤖 Finished Review · ✅ Success · Started 5:07 AM UTC · Completed 5:28 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.78 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace 12-org halfsend pool with single fullsend-ai-test org using
ephemeral bt-{uuid}-{slot} repos and repos install --fullsend-ref.
- Add composedDriver with per-scenario repo lifecycle (create/delete)
- Add RunReposInstall for ref-pinned installs with version-1 manifest
- Add prHeadSHAFromEvent fallback for pull_request_target workflows
- Add TokenForBehaviourOrg and BehaviourTestOrg to pkg/e2etest
- Update ADR 0066 and behaviour-testing docs for new architecture
- Set BEHAVIOUR_FULLSEND_REF in e2e workflow
Signed-off-by: Greg Allen <greg@fullsend.ai>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:43 AM UTC · Completed 6:04 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.99 |
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
I think this should be implemented as a new install driver alongside the existing ones, rather then a change in place to the current drivers which makes their names make less sense.
The use of ephemeral repos was probably made possible by the fact we not have per-test mints, but I'm not quite sure why we also need to abandon the org pool, leaving the org pool in place while going to ephemeral repos would have probably solved the undelying issues as well while also making the code change less radical.
I had already planned to go for deleting and recreating the repos before each scenario, would that have solved the issue as well?
2 participants
Replace the 12-org halfsend pool with a single fullsend-ai-test org using per-scenario ephemeral repos (bt-{uuid}-{slot}). This eliminates token exhaustion (#6702) and permission propagation races (#6701) that were artifacts of the pool model.
Key changes:
deletes repos on deallocation (unless E2E_KEEP_REPOS=true)
--fullsend-ref instead of github setup --vendor
GITHUB_REF_NAME for ref-pinned install path
Note: pre-commit hooks could not run (network-restricted sandbox). gofmt, go vet, trailing-whitespace, and lychee link checks passed via direct execution.
Closes #6815
Post-script verification
agent/6815-single-org-ephemeral-repos)1ac1750a661c6ccc170267c8b3919d2597cf7810..HEAD)