fix(#6425): make isTimeoutError self-contained against context errors

[fullsend-ai-coder]

Summary

Make isTimeoutError in internal/forge/github/github.go self-contained against context errors by adding a context.Context parameter. The function now checks ctx.Err() internally instead of relying on callers to guard against context errors before calling it.

Related Issue

Changes

• Added context.Context parameter to isTimeoutError so the function checks ctx.Err() internally
• Updated doc comment to explain why the context parameter is necessary (Go's net/http wraps context.DeadlineExceeded in *url.Error for client timeouts, making error-only introspection insufficient)
• Updated the single call site in do() to pass ctx
• Expanded test table with context-aware test cases: active context vs cancelled context scenarios

Testing

• All existing tests for isTimeoutError and its callers pass
• New test cases verify that isTimeoutError returns false when the caller's context is cancelled, even if the error implements Timeout() bool
• go vet and race detector report no issues
• 100% coverage on isTimeoutError

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• No secrets or sensitive data in the diff

---

Closes #6425

Post-script verification

• Branch is not main/master (agent/6425-harden-istimeouterror)
• Secret scan passed (gitleaks — b294e1eaab74f8174dda7c556cad7e0508f6d9eb..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:10 PM UTC · Completed 6:25 PM UTC

Commit: 06ca7e2 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [test adequacy] internal/forge/github/github_test.go:2996 — Duplicate test case: "timeout error with cancelled context returns false" is identical to "context.DeadlineExceeded with cancelled context" (both pass cancelledCtx with context.DeadlineExceeded and expect false). This provides no additional coverage.
  Remediation: Either remove the duplicate or repurpose it to test a distinct scenario — for example, a custom error type implementing Timeout() (not context.DeadlineExceeded) with a cancelled context, which would exercise the Timeout() branch being short-circuited by the ctx.Err() guard.

• [test adequacy] internal/forge/github/github_test.go — Missing test for the primary real-world scenario motivating this change: a *url.Error wrapping context.DeadlineExceeded (as produced by net/http when Client.Timeout fires) with an active context. Adding such a test would directly validate the documented motivation for the ctx.Err() approach.
  Remediation: Add a test case using &url.Error{Op: "Get", Err: context.DeadlineExceeded} with context.Background() expecting true, and optionally one with a cancelled context expecting false.

• [scope divergence from issue] internal/forge/github/github.go:298 — The issue (Harden existing isTimeoutError against context.DeadlineExceeded without caller obligation #6425) proposed error-only guards (errors.Is checks), while the PR uses a context.Context parameter with ctx.Err(). The PR's approach is well-motivated — Go's net/http wraps context.DeadlineExceeded in *url.Error, making error-only introspection insufficient to distinguish caller deadlines from transport timeouts. This creates a minor stylistic inconsistency with forge.IsTransient (which uses the error-only guard), but that is expected since IsTransient lacks context access.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:16 PM UTC · Completed 12:28 PM UTC

Commit: 06ca7e2 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.83

[fullsend-ai-retro]

Retro: PR #6428 — make isTimeoutError self-contained against context errors

Workflow performance

The end-to-end pipeline performed well. Issue #6425 was surfaced by a retro agent analyzing PR #6217, triaged in ~5 minutes, coded in ~13 minutes, and reviewed in ~15 minutes — issue-to-PR in roughly 20 minutes. The code agent made a better technical decision than the issue proposed: it added a context.Context parameter instead of errors.Is guards, correctly reasoning that Go's net/http wraps context.DeadlineExceeded in *url.Error, making error-only introspection insufficient. The review agent validated this deviation and identified three legitimate findings (all low severity). Codecov confirmed 100% coverage on modified lines.

Timeline

1. Issue created (Aug 20 17:48) — retro agent filed Harden existing isTimeoutError against context.DeadlineExceeded without caller obligation #6425 from PR ci(#6216): retry transient errors in after-scenario cleanup hooks #6217
2. Triage (Aug 20 17:48–17:53) — added context about related Document Timeout() interface / context.DeadlineExceeded pitfall in Go coding guidelines #6424, suggested test cases, labeled ready-to-code
3. Code (Aug 20 17:53–18:07) — produced PR fix(#6425): make isTimeoutError self-contained against context errors #6428 with context-parameter approach
4. Review (Aug 20 18:10–18:25) — APPROVED with 3 low findings: duplicate test case, missing *url.Error wrapping test, scope divergence note
5. Human review (Aug 27 08:18) — rh-hemartin APPROVED
6. Merge queue attempts — 4 attempts (Aug 27, Sep 8 x3), 3 failures due to GCP WIF rate limiting (HTTP 429)
7. Merged (Sep 8 12:14)

Findings

Merge queue flakiness (existing issue #6179). Three of four merge queue attempts failed due to WIF provisioning rate limits. The third failure's logs confirmed: rate limited (HTTP 429) after 7 attempts across 11 of 26 e2e behaviour test scenarios. Issue #6179 (reuse WIF providers across merge-queue attempts) directly addresses this root cause. Recently closed #6129 added GCP quota protection, which may reduce but not eliminate the problem while providers are still recreated per attempt.

Approved-with-findings pattern (existing issue #1286). The review agent approved with two actionable test-quality findings that were never addressed: (1) a duplicate test case at lines 2966–2970 and 2990–2994 of github_test.go (identical cancelledCtx + context.DeadlineExceeded inputs), and (2) a missing test for the primary real-world scenario (*url.Error wrapping context.DeadlineExceeded from net/http client timeouts). Both findings are confirmed present in the merged code. Issue #1286 proposes auto-filing tracking issues for unresolved low-severity findings, which would have caught these.

No new proposals

The identified improvement areas are already tracked by existing open issues. No new proposals are warranted.