ci(#6216): retry transient errors in after-scenario cleanup hooks

[fullsend-ai-coder]

Summary

• Add forge.IsTransient() transient error detection for forge API errors, timeouts, EOF, and race conditions
• Add IsTransient() method to GitHub, GitLab, and Jira APIError types (HTTP 429 and 500–504)
• Wrap all API operations in CleanupScenario with a cleanupRetry helper: retries transient errors up to 3 times with exponential backoff, then logs and continues
• Non-transient errors (401, 404, 422) are logged immediately without retry, preserving existing behavior

Context

After-scenario cleanup hooks in the behaviour test suite perform API operations (closing issues/PRs, deleting branches/repos, clearing dummy scripts) that are not part of the test assertion itself. When these cleanup calls hit transient infrastructure errors (GCP IAM 503, GitHub 5xx), the cleanup fails and can leave orphaned resources. This change adds retry resilience to prevent transient errors from disrupting cleanup, while keeping non-transient errors visible through logging.

Also addresses the broader pattern described in #5774 (422 Tree SHA race) by detecting ErrNonFastForward as a transient error worthy of retry at the cleanup level.

Testing

• Unit tests for forge.IsTransient() covering all error types (nil, sentinels, transient reporter, timeout, EOF, non-transient)
• Unit tests for APIError.IsTransient() across all HTTP status codes
• Unit tests for cleanupRetry helper: immediate success, transient-then-success, exhausted retries, non-transient no-retry
• Integration tests verifying CleanupScenario retries transient CloseIssue and CommitFile errors
• All existing cleanup tests continue to pass

---

Closes #6216

Post-script verification

• Branch is not main/master (agent/6216-cleanup-transient-retry)
• Secret scan passed (gitleaks — fcd4702368c74d517911b177f7ee7eca1634865b..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:02 PM UTC · Completed 11:18 PM UTC

Commit: 50bd49f · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/behaviourtest/steps/cleanup.go	90.76%	3 Missing and 3 partials ⚠️
internal/forge/gitlab/gitlab.go	0.00%	3 Missing ⚠️
internal/forge/jira/client.go	0.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Medium

• [logic error] internal/forge/forge.go:124 — IsTransient classifies context.DeadlineExceeded as transient because it implements Timeout() bool returning true. The comment says "HTTP client timeout (distinct from context cancellation)" but the code does not actually distinguish them. In the cleanup path this is harmless (context.Background() is used), and the existing isTimeoutError in the same codebase follows the same pattern, but IsTransient is a public function and could surprise future callers with deadline-bound contexts.
  Remediation: Add a guard before the Timeout() check: if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { return false }

Low

• [race condition] pkg/behaviourtest/steps/cleanup_test.go — speedUpCleanupRetries modifies the package-level cleanupBaseDelay variable without synchronization while tests run with t.Parallel(). In practice all tests write the same value (1ms) so the race is benign, but the -race detector would flag it.
  Remediation: Remove t.Parallel() from tests that modify package-level variables, or pass delay/attempts as parameters to cleanupRetry.

• [test adequacy] internal/forge/transient_test.go — Missing test cases for context.DeadlineExceeded and context.Canceled errors, which would document the Timeout() interface overlap described above.

• [test adequacy] pkg/behaviourtest/steps/cleanup_test.go — Integration tests cover retry-then-succeed but not retry-then-exhaust at the CleanupScenario level. The unit-level TestCleanupRetry_TransientExhausted covers the helper, but end-to-end behavior (log warning and continue) is untested.

• [missing-api-documentation] docs/ADRs/0005-forge-abstraction-layer.md — ADR-0005 documents sentinel errors and their errors.Is() helpers but does not mention the new forge.IsTransient classifier. A brief note alongside the existing sentinel error documentation would help discoverability.

---

Labels: PR modifies forge error handling infrastructure (internal/forge/) and behaviour test cleanup hooks (pkg/behaviourtest/)

[fullsend-ai-review]

See the review comment for full details.

---

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

• internal/forge/forge.go:124: [medium] logic error

IsTransient classifies context.DeadlineExceeded as transient because it implements Timeout() bool returning true. The comment says 'HTTP client timeout (distinct from context cancellation)' but the code does not actually distinguish them. In the cleanup path this is harmless (context.Background() is used), and the existing isTimeoutError in the same codebase follows the same pattern, but IsTransient is a public function and could surprise future callers with deadline-bound contexts.

Suggested fix: Add a guard before the Timeout() check: if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { return false }

• pkg/behaviourtest/steps/cleanup_test.go (file-level): Line 619 · [low] race condition

speedUpCleanupRetries modifies the package-level cleanupBaseDelay variable without synchronization while tests run with t.Parallel(). In practice all tests write the same value (1ms) so the race is benign, but the -race detector would flag it.

Suggested fix: Remove t.Parallel() from tests that modify package-level variables, or pass delay/attempts as parameters to cleanupRetry.