What happened
On PR #4081, the review agent reviewed internal/repos/uninstall.go which uses parallel goroutines to delete variables and secrets simultaneously (lines 186-207). Each goroutine writes to a separate error variable (varErr, secretErr). When both fail, only varErr was returned — secretErr was silently discarded. The review agent did not detect this pattern despite reading the file and posting findings on it. @waynesun09's squad caught it as a MEDIUM-severity finding on Jul 17, and the fix used errors.Join to combine both errors. Separately, fullsend#3512 tracks adding AGENTS.md guidance for code agents to use errors.Join in concurrent fan-out — this proposal is the review-side counterpart to catch the pattern when it slips through.
What could go better
The correctness sub-agent checks for error handling gaps generally but does not have a specific pattern for parallel execution with multiple error variables. The dangerous code shape is: two or more goroutines (or sequential branches) each write to separate err variables, then a conditional block checks them individually with if errA != nil { return errA }; if errB != nil { return errB } — silently dropping errB when both are non-nil. This is a well-known Go antipattern that errors.Join (added in Go 1.20) was designed to solve. Confidence: HIGH — the sub-agent read the file, the pattern is syntactically identifiable, and the fix is mechanical.
Proposed change
Add a checklist item to skills/pr-review/sub-agents/correctness.md in the error handling section. The item should instruct the sub-agent to check for parallel or concurrent code paths that write to separate error variables and verify that all error values are consolidated before returning (e.g., via errors.Join). Flag as medium severity when multiple error variables from independent operations are checked sequentially with early returns, since only the first non-nil error is propagated. Include the canonical Go pattern to detect: var errA, errB error / go func() { errA = ... }() / go func() { errB = ... }() / if errA != nil { return errA } / if errB != nil { return errB } — where the fix is return errors.Join(errA, errB).
Validation criteria
On the next 3 PRs to any fullsend-managed Go repo that introduce parallel goroutines or concurrent operations with separate error variables, the correctness sub-agent should flag cases where errors are not consolidated. Validate by checking whether findings mention errors.Join, error consolidation, or silent error dropping for parallel operations. Cross-reference with fullsend#3512 — once that AGENTS.md guidance lands, the code agent should also avoid introducing this pattern in the first place.
Generated by retro agent from fullsend-ai/fullsend#4081
What happened
On PR #4081, the review agent reviewed
internal/repos/uninstall.gowhich uses parallel goroutines to delete variables and secrets simultaneously (lines 186-207). Each goroutine writes to a separate error variable (varErr,secretErr). When both fail, onlyvarErrwas returned —secretErrwas silently discarded. The review agent did not detect this pattern despite reading the file and posting findings on it. @waynesun09's squad caught it as a MEDIUM-severity finding on Jul 17, and the fix usederrors.Jointo combine both errors. Separately, fullsend#3512 tracks adding AGENTS.md guidance for code agents to useerrors.Joinin concurrent fan-out — this proposal is the review-side counterpart to catch the pattern when it slips through.What could go better
The correctness sub-agent checks for error handling gaps generally but does not have a specific pattern for parallel execution with multiple error variables. The dangerous code shape is: two or more goroutines (or sequential branches) each write to separate
errvariables, then a conditional block checks them individually withif errA != nil { return errA }; if errB != nil { return errB }— silently dropping errB when both are non-nil. This is a well-known Go antipattern thaterrors.Join(added in Go 1.20) was designed to solve. Confidence: HIGH — the sub-agent read the file, the pattern is syntactically identifiable, and the fix is mechanical.Proposed change
Add a checklist item to
skills/pr-review/sub-agents/correctness.mdin the error handling section. The item should instruct the sub-agent to check for parallel or concurrent code paths that write to separate error variables and verify that all error values are consolidated before returning (e.g., viaerrors.Join). Flag as medium severity when multiple error variables from independent operations are checked sequentially with early returns, since only the first non-nil error is propagated. Include the canonical Go pattern to detect:var errA, errB error / go func() { errA = ... }() / go func() { errB = ... }() / if errA != nil { return errA } / if errB != nil { return errB }— where the fix isreturn errors.Join(errA, errB).Validation criteria
On the next 3 PRs to any fullsend-managed Go repo that introduce parallel goroutines or concurrent operations with separate error variables, the correctness sub-agent should flag cases where errors are not consolidated. Validate by checking whether findings mention
errors.Join, error consolidation, or silent error dropping for parallel operations. Cross-reference with fullsend#3512 — once that AGENTS.md guidance lands, the code agent should also avoid introducing this pattern in the first place.Generated by retro agent from fullsend-ai/fullsend#4081