test(#6120): delete leased pool repos after each scenario

[fullsend-ai-coder]

Summary

Leased behaviour-test pool repos (test-repo-NN) are now ephemeral around a lease: DeallocateRepo deletes the base (and any leftover fork) after scenario cleanup, and the next AllocateRepo recreates from a clean auto_init commit so leftover labels, PRs, branches, workflow runs, and config cannot leak across scenarios.

Related Issue

Implements the pool-repo lifecycle described in the issue (delete+recreate on acquire was already in doEnsure; this PR closes the re-lease cache gap and adds post-scenario deletion).

Changes

• Add ensurer.DeleteRepo, which invalidates the per-lease ensure cache and reuses resetRepo (source + leftover fork, deletion wait).
• composedDriver.DeallocateRepo deletes the leased repo after CleanupScenario (After hook already defers deallocation until cleanup finishes; in-scenario debug collection has already written workflow logs and agent artifacts under BEHAVIOUR_ARTIFACT_DIR).
• Delete failures are logged and do not leak the pool slot; the next ensure still reset+recreates.
• Docs: behaviour-testing.md and behaviour-drivers.md describe the ephemeral lease lifecycle and the extra create+install cost on re-lease.

Testing

• Unit tests cover delete-on-deallocate, cache invalidation, re-ensure after delete, missing-repo idempotency, leftover-fork delete, and delete-error still returning the name to the pool.
• go test -race ./pkg/behaviourtest/drivers/install/ ./pkg/behaviourtest/suite/ passes.
• go vet passes; DeleteRepo and DeallocateRepo are at 100% function coverage.
• pre-commit run could not fetch hook repos (HTTP 403); local equivalents were run (gofmt, go vet, gitleaks, lychee, lint-docs-links, lint-interface-doc-sync).

Checklist

• PR title follows Conventional Commits (test(#6120): ...)
• Autonomous agent commit (DCO not required)
• Changes are limited to the issue scope

---

Closes #6120

Post-script verification

• Branch is not main/master (agent/6120-ephemeral-pool-repos)
• Secret scan passed (gitleaks — 99894b146f1d86c39243334fdc8fdc438719e8ee..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:36 AM UTC · Completed 10:57 AM UTC

Commit: 92b58d1 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.39

[github-actions]

Site preview

Preview: https://a7d6dac5-site.fullsend-ai.workers.dev

Commit: 281aaed8329c926f95bbb0ff5b914cdeda00a309

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Re-review: Tier 1 signals unchanged from the prior assessment (bot author, no protected/security paths, no dependency or CI changes); the only delta is a small incremental fix (3 files) addressing prior review feedback on the same well-scoped, well-tested behaviour-test infrastructure change, so the moderate score of 2 is preserved.

Previous run

Risk Assessment: moderate (2/5)

Details

Bot-authored, well-tested, moderate-sized change confined to behaviour-test infrastructure with no protected/security-sensitive paths, no CI or dependency changes; issue scope and acceptance criteria are well matched, yielding a moderate composite score.

[fullsend-ai-review]

Looks good to me

Previous run

Review

Low

• [logic-error] pkg/behaviourtest/drivers/install/composed.go:143 — Finalize reclaims outstanding leases by returning names to the pool without calling ensurer.DeleteRepo, so a leaked lease (After-hook skip, a panic before the deferred DeallocateRepo, or a cancelled After-hook context) leaves the forge repo in place until the next acquire, bypassing the ephemeral-lease contract this PR introduces for the normal deallocate path.
  Remediation: Best-effort DeleteRepo each leaked name in Finalize (same log-and-continue policy as DeallocateRepo) before returning names to the pool / tearing down mint.

• [nil-handling] pkg/behaviourtest/drivers/install/composed.go:82 — DeallocateRepo guards d.ensurer == nil before calling DeleteRepo, but AllocateRepo calls d.ensurer.EnsureRepo unconditionally, an inconsistency between the two methods (not reachable in production today since all factories pass a real ensurer).
  Remediation: Either reject a nil ensurer in newComposedDriver, or apply the same nil check on the AllocateRepo EnsureRepo call.

• [correctness/scope] docs/guides/dev/behaviour-testing.md:395 — This PR moves the ensure cache from run-scoped to lease-scoped, so every scenario that leases a repo now pays the full create+install+settle cost, instead of only the first lease of each pool slot. The 45-minute CI budget text is left unchanged with only a caveat to "verify" it still holds. Recommend measuring actual wall-clock time on this PR's CI run rather than assuming the existing budget holds; do not pre-emptively raise the timeout without confirming the need (see docs/contributing/go-code.md suite-timeout policy).

• [stale-documentation] docs/guides/dev/behaviour-testing.md:440 — States the settle step is skipped on CLI re-vendors "since the workflow file already existed." doEnsure in ensure.go always resets, recreates, installs, and settles — there is no skip-on-revendor code path. This inaccuracy pre-dates this PR (the sentence and the underlying always-settle behavior are unmodified by this diff), but it directly contradicts the lifecycle description this PR is otherwise updating in the same file.
  Remediation: Update or remove the re-vendor/settle-skip claim to reflect that doEnsure always performs a fresh install and settle wait.

• [stale-documentation] docs/guides/dev/behaviour-testing.md:278 — Describes halfsend STAGE test-repo-NN repos as "durable repos reused across runs." NewRepoPoolCFMintStage (unmodified by this PR) wires the same composedDriver/repoEnsurer used by the DEV pool, so this PR's new DeallocateRepo → DeleteRepo path now deletes STAGE pool repos after every lease as well — the numbered repos are no longer durable across runs, even though the halfsend org/mint itself is.
  Remediation: Update line 278 to say the halfsend STAGE organization/mint is durable, but its test-repo-NN repos now follow the same ephemeral per-lease lifecycle as DEV.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 10:59 AM UTC · Completed 11:09 AM UTC

Commit: 92b58d1 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.32

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (bot-triggered)

Fixed the two composed.go findings (Finalize now deletes leaked leases; newComposedDriver rejects a nil ensurer) with new test coverage, and corrected the two stale-documentation findings in behaviour-testing.md. The CI-budget finding was verified against this PR's actual CI run (13m2s, within the 45-minute budget) rather than changed, consistent with the review's own caution against pre-emptive timeout changes.

Fixed (4):

1. logic-error: Finalize reclaims leaked leases without calling ensurer.DeleteRepo, so a missed After hook / panic / cancelled context leaves the forge repo in place until next acquire (pkg/behaviourtest/drivers/install/composed.go): Finalize now best-effort deletes each reclaimed (leaked) repo via ensurer.DeleteRepo before returning it to the pool, using the same log-and-continue policy as DeallocateRepo. Added TestComposedDriver_FinalizeWithOutstanding assertion and a new TestComposedDriver_FinalizeDeleteErrorStillReturnsLeakErr test.
2. nil-handling: DeallocateRepo guards d.ensurer == nil but AllocateRepo calls d.ensurer.EnsureRepo unconditionally, an inconsistency between the two methods (pkg/behaviourtest/drivers/install/composed.go): Chose the constructor-rejection remediation option: newComposedDriver now returns an error if ensurer is nil, so the invariant holds for all callers (both AllocateRepo's unconditional call and DeallocateRepo's existing guard are now consistent with an always-non-nil ensurer). Added TestNewComposedDriver_NilEnsurer.
3. stale-documentation: behaviour-testing.md:440 claims the settle step is skipped on CLI re-vendors since the workflow file already existed, but doEnsure always resets, recreates, installs, and settles (docs/guides/dev/behaviour-testing.md): Rewrote the sentence to state that doEnsure always resets, installs, and settles on every ensure, since every lease starts from a freshly created repo (verified in pkg/behaviourtest/drivers/install/ensure.go doEnsure).
4. stale-documentation: behaviour-testing.md:278 describes halfsend STAGE test-repo-NN repos as durable repos reused across runs, but this PR's DeallocateRepo -> DeleteRepo path now deletes them after every lease too (docs/guides/dev/behaviour-testing.md): Updated the blast-radius containment sentence to say the STAGE organisation/mint itself is durable, but its test-repo-NN repos now follow the same ephemeral per-lease lifecycle as DEV (deleted and recreated on each lease). Verified NewRepoPoolCFMintStage wires the same composedDriver/repoEnsurer as DEV.

Disagreed (1):

1. correctness/scope: 45-minute CI budget text unverified; measure actual wall-clock, don't pre-emptively raise it: Checked this PR's own CI run via gh pr checks 7398: the behaviour job completed in 13m2s, well within the existing 45-minute budget, even with the lease-scoped re-ensure cost this PR introduces. This confirms the current budget still holds, so per the review's own guidance not to pre-emptively raise the timeout, no doc or timeout change was made.

Tests: passed

Decision points

• For the nil-ensurer inconsistency, reject nil in the constructor rather than adding a matching nil guard to AllocateRepo (alternatives: Add the same if d.ensurer != nil guard to AllocateRepo's EnsureRepo call, Reject a nil ensurer in newComposedDriver; rationale: Rejecting nil at construction establishes a real invariant (ensurer is always usable) instead of teaching both methods to silently no-op on a nil ensurer, which would mask a misconfigured driver instead of failing fast.)

---

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:11 AM UTC · Completed 11:24 AM UTC

Commit: 281aaed · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.65

Superseded by updated review