What happened
In PR #6217, the code agent wrote a new forge.IsTransient(err) function that detects transient errors. It included a check for the interface{ Timeout() bool } interface to identify HTTP client timeouts. However, context.DeadlineExceeded also implements this interface (returning true), so the function incorrectly classified context deadline errors as transient/retryable.
The code agent likely replicated this pattern from the existing isTimeoutError function in internal/forge/github/github.go:295, which uses the same Timeout() bool check but documents that callers must check ctx.Err() first. The new public IsTransient function had no such caller contract.
The review agent (run 32298552946) correctly caught this as a medium-severity logic error. The fix agent added a guard checking errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) before the Timeout() interface check.
What could go better
The code agent could have avoided this bug on the first attempt if the Go coding guidelines documented this well-known Go pitfall. The repo already has a pattern of documenting domain-specific pitfalls — docs/contributing/cel-triggers.md has a "highest-value pitfall" section about synchronize vs synchronized, and docs/contributing/shell-scripting.md has a yq/jq pitfalls section. But docs/contributing/go-code.md has no pitfalls section despite Go having several well-known interface traps.
Confidence: High. The code agent demonstrably replicated the existing isTimeoutError pattern. Adding this pitfall to the guidelines would put it in the code agent's context window when writing Go error-handling code. The repo already has the pattern and infrastructure for pitfall documentation.
Uncertainty: The code agent might still miss documented pitfalls if it doesn't read the relevant guide during code generation. However, the existing pitfall sections in other guides suggest the harness does surface these docs.
Proposed change
Add a "Go pitfalls" section to docs/contributing/go-code.md with the Timeout() bool interface trap as its first entry. The section should explain:
context.DeadlineExceeded implements interface{ Timeout() bool } returning true- Any timeout detection via interface assertion (e.g.,
if te, ok := err.(interface{ Timeout() bool }); ok && te.Timeout()) must guard against context errors first: if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { return false }
- Reference the
forge.IsTransient function as the canonical example of the correct pattern
- Explain that context deadline/cancellation errors represent intentional cancellation by the caller, not transient infrastructure failures, so they should never be retried
This follows the existing pattern in cel-triggers.md ("highest-value pitfall") and shell-scripting.md ("yq/jq pitfalls").
Validation criteria
The next code agent run that writes Go timeout/transient error detection should include a context.DeadlineExceeded guard without requiring review agent intervention. Verify by monitoring the next 3-5 code agent PRs in this repo that touch error handling — none should repeat this specific bug pattern.
Generated by retro agent from #6217
What happened
In PR #6217, the code agent wrote a new
forge.IsTransient(err)function that detects transient errors. It included a check for theinterface{ Timeout() bool }interface to identify HTTP client timeouts. However,context.DeadlineExceededalso implements this interface (returningtrue), so the function incorrectly classified context deadline errors as transient/retryable.The code agent likely replicated this pattern from the existing
isTimeoutErrorfunction ininternal/forge/github/github.go:295, which uses the sameTimeout() boolcheck but documents that callers must checkctx.Err()first. The new publicIsTransientfunction had no such caller contract.The review agent (run 32298552946) correctly caught this as a medium-severity logic error. The fix agent added a guard checking
errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)before theTimeout()interface check.What could go better
The code agent could have avoided this bug on the first attempt if the Go coding guidelines documented this well-known Go pitfall. The repo already has a pattern of documenting domain-specific pitfalls —
docs/contributing/cel-triggers.mdhas a "highest-value pitfall" section aboutsynchronizevssynchronized, anddocs/contributing/shell-scripting.mdhas ayq/jq pitfallssection. Butdocs/contributing/go-code.mdhas no pitfalls section despite Go having several well-known interface traps.Confidence: High. The code agent demonstrably replicated the existing
isTimeoutErrorpattern. Adding this pitfall to the guidelines would put it in the code agent's context window when writing Go error-handling code. The repo already has the pattern and infrastructure for pitfall documentation.Uncertainty: The code agent might still miss documented pitfalls if it doesn't read the relevant guide during code generation. However, the existing pitfall sections in other guides suggest the harness does surface these docs.
Proposed change
Add a "Go pitfalls" section to
docs/contributing/go-code.mdwith theTimeout() boolinterface trap as its first entry. The section should explain:context.DeadlineExceededimplementsinterface{ Timeout() bool }returningtrueif te, ok := err.(interface{ Timeout() bool }); ok && te.Timeout()) must guard against context errors first:if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { return false }forge.IsTransientfunction as the canonical example of the correct patternThis follows the existing pattern in
cel-triggers.md("highest-value pitfall") andshell-scripting.md("yq/jq pitfalls").Validation criteria
The next code agent run that writes Go timeout/transient error detection should include a
context.DeadlineExceededguard without requiring review agent intervention. Verify by monitoring the next 3-5 code agent PRs in this repo that touch error handling — none should repeat this specific bug pattern.Generated by retro agent from #6217