What happened
On PR #3194, ParseForgeURL was changed to accept gitlab.com URLs via a new isRecognizedForge function (previously IsSupportedForge rejected them). The human reviewer found 4 separate callers needing forge guards across 3 rounds:
- Round 1 (Jul 9, waynesun09):
harness.go already guarded, but resolve.go and fetchsvc/service.go did not — a GitLab URL would proceed to git-fetch and fail unhelpfully.
- Round 2 (Jul 9, waynesun09): Verified resolve.go/fetchsvc.go fixes but found 2 more callers still unguarded.
- Round 3 (Jul 9, waynesun09): Found
parseAgentSourceURL in internal/cli/agent.go was also missing the guard.
- Round 4 (Jul 10, waynesun09): Found
parseAgentSourceURL in url.go was also missing it.
The review agent caught the resolve.go/fetchsvc gap only AFTER the human pointed out the pattern on harness.go — and only in a subsequent review run, not the initial pass.
What could go better
The correctness sub-agent's consumer completeness guidance says to check callers when 'the diff adds new values to an enum, dispatch table, JSON schema enum, or case/switch structure.' But the gitlab.com change isn't an enum addition — it's a validation/gating function becoming more permissive. This is a common and well-understood class of bug: when you widen a gate, everything behind it must handle the new inputs. The human reviewer needed 3 rounds to find all 4 affected sites, suggesting this requires systematic caller analysis (grep) rather than just reading the diff. Confidence: high — the existing guidance is narrowly scoped to enums, and the missed pattern (validation permissiveness) is clearly distinct and well-defined.
Proposed change
Broaden the 'Consumer completeness' section in the correctness sub-agent (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/correctness.md) to cover validation/gating function changes:
'When the diff modifies a validation, gating, or filtering function to accept inputs it previously rejected (e.g., a support-check function now returns true for a new domain, a URL parser now accepts a new scheme, an allow-list gains a new entry), grep for all callers of that function in the codebase and verify each caller handles the newly-accepted input correctly. A gating function that becomes more permissive without all callers being updated is a medium-severity consumer-completeness finding. Check both direct callers and transitive callers (functions that call functions that call the changed gating function). This pattern is distinct from enum additions — it applies whenever a boolean/validation function's acceptance set widens.'
Validation criteria
On a PR where a validation function is relaxed to accept new inputs (like IsSupportedForge accepting gitlab.com, or a URL parser accepting a new domain), the correctness sub-agent greps for all callers and verifies each handles the new case. Can be validated on PR #3194's original diff or a synthetic PR that widens a Go validation function without updating 2+ callers.
Generated by retro agent from #3194
What happened
On PR #3194,
ParseForgeURLwas changed to acceptgitlab.comURLs via a newisRecognizedForgefunction (previouslyIsSupportedForgerejected them). The human reviewer found 4 separate callers needing forge guards across 3 rounds:harness.goalready guarded, butresolve.goandfetchsvc/service.godid not — a GitLab URL would proceed to git-fetch and fail unhelpfully.parseAgentSourceURLininternal/cli/agent.gowas also missing the guard.parseAgentSourceURLinurl.gowas also missing it.The review agent caught the resolve.go/fetchsvc gap only AFTER the human pointed out the pattern on harness.go — and only in a subsequent review run, not the initial pass.
What could go better
The correctness sub-agent's consumer completeness guidance says to check callers when 'the diff adds new values to an enum, dispatch table, JSON schema enum, or case/switch structure.' But the gitlab.com change isn't an enum addition — it's a validation/gating function becoming more permissive. This is a common and well-understood class of bug: when you widen a gate, everything behind it must handle the new inputs. The human reviewer needed 3 rounds to find all 4 affected sites, suggesting this requires systematic caller analysis (grep) rather than just reading the diff. Confidence: high — the existing guidance is narrowly scoped to enums, and the missed pattern (validation permissiveness) is clearly distinct and well-defined.
Proposed change
Broaden the 'Consumer completeness' section in the correctness sub-agent (
internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/correctness.md) to cover validation/gating function changes:'When the diff modifies a validation, gating, or filtering function to accept inputs it previously rejected (e.g., a support-check function now returns true for a new domain, a URL parser now accepts a new scheme, an allow-list gains a new entry), grep for all callers of that function in the codebase and verify each caller handles the newly-accepted input correctly. A gating function that becomes more permissive without all callers being updated is a medium-severity consumer-completeness finding. Check both direct callers and transitive callers (functions that call functions that call the changed gating function). This pattern is distinct from enum additions — it applies whenever a boolean/validation function's acceptance set widens.'
Validation criteria
On a PR where a validation function is relaxed to accept new inputs (like IsSupportedForge accepting gitlab.com, or a URL parser accepting a new domain), the correctness sub-agent greps for all callers and verifies each handles the new case. Can be validated on PR #3194's original diff or a synthetic PR that widens a Go validation function without updating 2+ callers.
Generated by retro agent from #3194