Correctness sub-agent should check Go resource lifecycle patterns (defer Close, http.Response.Body)

[fullsend-ai-retro]

What happened

On PR #3194, the review agent's correctness sub-agent analyzed IsProtectedBranch across 12 review runs (Jul 7-10). Every run praised its URL escaping and error handling. But the success path (HTTP 200 response) discarded the *http.Response without closing its body — a textbook Go resource leak. Every other c.get() call site in the same file passes the response to decodeJSON (which defers Body.Close) or closes it explicitly.

The human reviewer flagged this as high severity, noting it was 'flagged independently by 4/8 review-squad agents.' Qodo's initial review also found 4 bugs (unescaped URL, nil maps, state mismatch, wrong error type) that the review agent missed in its first pass. The resource leak was fixed by the author after round 1 of human review.

Related but distinct from #3966 (resource cleanup symmetry in multi-phase operations). That issue addresses cross-phase rollback in orchestration flows; this addresses single-function resource lifecycle in Go HTTP client code.

What could go better

The correctness sub-agent's runtime mechanism checklist covers guard mechanisms and failure paths but has no guidance on resource lifecycle patterns. In Go, every *http.Response must have Body.Close() called on all paths — this is one of the most common Go code review items. The sub-agent demonstrably analyzed the function in detail (commenting on PathEscape, EqualFold) but didn't apply the resource lifecycle lens. Confidence: high — 12 runs all missing the same well-known pattern, while 4/8 human review-squad agents caught it independently, indicates this is a prompting gap rather than a model capability gap.

Proposed change

Add a 'Resource lifecycle checklist' section to the correctness sub-agent (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/correctness.md) alongside the existing 'Runtime mechanism checklist.' The new section should instruct:

1. Every acquired resource (http.Response, file handle, database connection, mutex lock) must have a corresponding release on all code paths — success, error, and early-return.
2. For Go HTTP calls specifically: any call that returns *http.Response must have resp.Body.Close() on all paths, including the success path when the body is not decoded. Pattern to flag: _, err := httpCall(...) discarding the response without verifying the callee closes it internally.
3. When reviewing a function that manipulates resources, trace the lifecycle: acquire → use → release. If any path skips release, flag it as a medium+ finding.

This fits the existing checklist structure and should trigger on the same class of bug.

Validation criteria

Run the correctness sub-agent on PR #3194's original diff (or a synthetic PR introducing an HTTP call that discards a successful response body). The sub-agent should flag the unclosed resp.Body as a medium+ finding. Success metric: the sub-agent catches Go resource lifecycle bugs that Qodo and the human review-squad caught on this PR.

---

Generated by retro agent from #3194

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 2:40 PM UTC · Completed 2:44 PM UTC
Commit: de2f978 · View workflow run →

[fullsend-ai-triage]

Triage Summary

Severity: Medium · Category: Feature (prompt enhancement)

Problem

The correctness sub-agent's prompt covers logic errors, runtime mechanisms, consumer completeness, and cross-file verification — but has no guidance on resource lifecycle patterns. This gap caused the sub-agent to miss an unclosed http.Response.Body across 12 consecutive review runs on PR #3194, while 4 of 8 human review-squad agents independently caught the same issue.

Root Cause

The existing "Runtime mechanism checklist" in correctness.md focuses on guard mechanisms and failure paths. There is no instruction to trace the acquire → use → release lifecycle of resources (HTTP responses, file handles, database connections, mutex locks) across all code paths. Without this prompting, the model analyzes the logic it's told to check and misses a well-known Go review pattern.

Relationship to #3966

Distinct from #3966, which covers cross-phase rollback in multi-step orchestration flows. This issue addresses single-function resource lifecycle — a narrower, more common pattern.

Recommended Fix

Add a "Resource lifecycle checklist" section to internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/correctness.md alongside the existing checklist. Key items:

1. Every acquired resource must have a corresponding release on all paths (success, error, early-return)
2. Go-specific: flag *http.Response without Body.Close() on all paths, especially _, err := httpCall(...) discarding the response
3. Trace acquire → use → release; flag any path skipping release as medium+ severity

Proposed Test Case

Synthetic Go diff with an HTTP client function that:
- Calls an HTTP endpoint, returns (*SomeType, error)
- Correctly handles the error path
- On success path: reads status code, returns WITHOUT closing resp.Body

Expected: Sub-agent flags unclosed resp.Body as medium+ finding
Before fix: Sub-agent praises URL escaping/error handling, misses the leak

Edge cases:
- Response passed to helper that closes internally → should NOT flag
- defer resp.Body.Close() immediately after nil-error check → should NOT flag  
- Body closed on error path but not success path → SHOULD flag
- Non-HTTP: os.Open without defer f.Close() → SHOULD flag

This issue is well-scoped and ready for implementation.

---

Labels: This is a feature request for the review agent's correctness sub-agent. Matches type/feature and agent/review labels. Medium priority based on demonstrated impact (missed findings on real PRs) but not blocking any workflow.

[fullsend-ai-prioritize]

RICE Priority Score: 2.7

Score breakdown

Dimension	Score	Reasoning
Reach	1.5	The review agent's correctness sub-agent runs on all Go PRs across the platform. Resource lifecycle bugs (unclosed http.Response.Body, file handles, etc.) are among the most common Go review items, so this gap affects multiple teams and customers who submit Go code for review. Not platform-wide since it's Go-specific, but broadly relevant across Go-using orgs.
Impact	1	Medium — this is a noticeable improvement to the review workflow. The sub-agent demonstrably missed a well-known bug class across 12 runs on PR #3194, but other review mechanisms (human reviewers, Qodo, review-squad) still caught it. The fix improves automated review quality but doesn't unblock a critical workflow since human fallback exists.
Confidence	0.9	Very high — the issue is exceptionally well-documented with concrete evidence: 12 failed runs, a specific PR reference, 4/8 human reviewers catching what the agent missed, and a clear proposed solution with validation criteria. The triage agent confirmed it is well-scoped and ready for implementation. Almost no ambiguity remains.
Effort	0.5	Simple — the change is a prompt addition to a single markdown file (correctness.md), following the existing checklist structure already in place. The triage summary provides the exact content to add. Testing requires running the sub-agent on a synthetic or real diff, which is straightforward given the validation criteria provided.

Formula: (1.5 x 1 x 0.9) / 0.5 = 2.7