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 90.00000% with 9 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/jira/client.go	0.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Looks good to me

Previous run

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/)

[ralphbean]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 8:17 PM UTC · Completed 8:26 PM UTC

Commit: 50bd49f · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed both review findings: (1) added context.DeadlineExceeded/context.Canceled guard in IsTransient before the Timeout() check with corresponding test cases, (2) removed t.Parallel() from cleanup retry tests that mutate package-level state. All tests pass with -race detector.

Fixed (2):

1. IsTransient classifies context.DeadlineExceeded as transient via Timeout() interface (internal/forge/forge.go): Added guard to exclude context.DeadlineExceeded and context.Canceled before the Timeout() interface check, since these reflect caller intent rather than server-side failure. Added 4 test cases (bare and wrapped variants of both context errors) to transient_test.go.
2. speedUpCleanupRetries modifies package-level cleanupBaseDelay without synchronization while tests use t.Parallel() (pkg/behaviourtest/steps/cleanup_test.go): Removed t.Parallel() from all 6 test functions that call speedUpCleanupRetries (TestCleanupRetry_SucceedsImmediately, TestCleanupRetry_TransientThenSuccess, TestCleanupRetry_TransientExhausted, TestCleanupRetry_NonTransientNoRetry, TestCleanupScenario_RetriesTransientCloseIssue, TestCleanupScenario_RetriesTransientCommitFile). These tests mutate a package-level variable and cannot safely run in parallel.

Tests: passed

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 8:28 PM UTC · Completed 8:41 PM UTC

Commit: 0bb1e9b · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 5:34 PM UTC · Completed 5:48 PM UTC

Commit: 0bb1e9b · View workflow run →

[fullsend-ai-retro]

Retro: PR #6217 — retry transient errors in after-scenario cleanup hooks

Timeline

Time	Event
Aug 13, 22:35	Retro agent filed #6216 after PR #6199 was ejected from merge queue due to transient GCP 503
Aug 13, 22:36	Triage agent (run 31750610896) applied labels, marked ready-to-code
Aug 13, 22:41	Code agent (run 31750900057) started
Aug 13, 23:00	Code agent completed — PR #6217 created (+574/−19, 8 files)
Aug 13, 23:18	Review agent posted findings: medium-severity context.DeadlineExceeded misclassification, low-severity test race condition
6-day gap	No human action
Aug 19, 20:15	ralphbean triggered /fs-fix
Aug 19, 20:26	Fix agent addressed both findings (11 min)
Aug 19, 20:41	Review agent re-approved (run 32298552946)
Aug 20, 17:22	ralphbean approved
Aug 20, 17:32	Merged

Total agent time: ~44 minutes. Total elapsed: ~7 days. Human latency dominated.

What went well

• Review quality was excellent. The review agent found two real issues — a medium-severity logic error (context.DeadlineExceeded implements Timeout() bool, so the transient-error classifier misclassified context deadlines as retryable timeouts) and a low-severity test race condition (t.Parallel() with shared package-level variable mutation). Both were genuine bugs. Zero false positives.
• Fix agent was precise. Addressed both findings in 11 minutes with clean code.
• End-to-end pipeline worked. Issue→triage→code→review→fix→re-review→merge all completed successfully.
• Human reviewer found nothing additional. The review agent's coverage was sufficient for this change type.

Evidence for existing issues (not proposing new issues)

• Auto-fix triggering (#2596, #5937, #5350): This PR provides another data point for the auto-fix gap. The 6-day wait between review findings and /fs-fix was the dominant bottleneck. Both review findings were mechanical and clearly auto-fixable. Auto-dispatching the fix agent for bot-authored PRs with actionable findings would have reduced this PR's wall-clock time from 7 days to under 2 hours.
• 422 inline comment posting (#6039, agents#430, agents#760): The review agent hit a 422 posting an inline comment on forge.go:124 and fell back to the review body. The graceful fallback (fullsend#5131) worked correctly — no findings were lost. This appears to be the stale-diff re-review pattern tracked in Investigate 100% inline comment posting failure (422) on re-review rounds #6039.
• Code agent pattern replication (#3307): The code agent replicated the existing isTimeoutError pattern (which uses Timeout() bool without context-error guards) into the new public IsTransient function. The existing function documents the caller-must-check-ctx requirement, but the code agent didn't carry over that defensive context into the new public API.

Proposals filed

See linked proposals below.

Proposals filed

• Document Timeout() interface / context.DeadlineExceeded pitfall in Go coding guidelines (in fullsend-ai/fullsend)
• Harden existing isTimeoutError against context.DeadlineExceeded without caller obligation (in fullsend-ai/fullsend)