What happened
On PR #4081, the review agent (run 29126582718) reviewed runReposRemove in internal/cli/repos.go and flagged 4 medium findings but did not detect that the function had an all-or-nothing error gate: if any single repo's uninstall failed, the function returned an error before reaching the manifest update, meaning successfully-uninstalled repos would remain in the manifest. This was caught 3 days later by @waynesun09's multi-model review squad as a HIGH-severity finding. The same pattern appeared in reverse for repos add --install (manifest written before install, no rollback on failure). The fix required tracking per-repo success and narrowing the manifest update to only successful repos.
What could go better
The correctness sub-agent's definition covers 'error handling gaps' and 'failure paths' generically but has no specific guidance for batch operations where individual items can independently succeed or fail. The dangerous pattern is: a function iterates over N items, accumulates a failure count, then gates a state-mutating operation (manifest write, config update, resource cleanup) on if failures > 0 { return error } — discarding the results of successful items and leaving the system in an inconsistent state where infrastructure and manifest disagree. This pattern is common in CLI tools that operate on multiple repos, resources, or entities. Confidence: HIGH — the correctness sub-agent read the relevant code (it posted a finding on the same file) but did not trace the interaction between the error return and the manifest update that follows it.
Proposed change
Add a checklist item to skills/pr-review/sub-agents/correctness.md under the runtime mechanism verification section. The new item should instruct the sub-agent to check batch/loop operations for partial-failure consistency: when a function iterates over multiple items and some can fail independently, verify that (a) successful items' side effects are preserved or rolled back consistently, (b) the function does not gate downstream state mutations on an all-or-nothing error check that discards successful results, and (c) the error message distinguishes which items failed vs succeeded. Include an example pattern to watch for: for item := range items { if err := process(item); err != nil { failed++ } } / if failed > 0 { return error } / updateState(items) — where updateState should receive only the successful subset.
Validation criteria
On the next 5 PRs to fullsend-ai/fullsend that add or modify batch CLI operations (repos install, repos remove, repos uninstall, or similar iterate-and-act patterns), the correctness sub-agent should flag any all-or-nothing error gate that discards successful results, at medium severity or above. Validate by checking whether the sub-agent's findings mention partial failure, per-item tracking, or state consistency for batch operations.
Generated by retro agent from fullsend-ai/fullsend#4081
What happened
On PR #4081, the review agent (run 29126582718) reviewed
runReposRemoveininternal/cli/repos.goand flagged 4 medium findings but did not detect that the function had an all-or-nothing error gate: if any single repo's uninstall failed, the function returned an error before reaching the manifest update, meaning successfully-uninstalled repos would remain in the manifest. This was caught 3 days later by @waynesun09's multi-model review squad as a HIGH-severity finding. The same pattern appeared in reverse forrepos add --install(manifest written before install, no rollback on failure). The fix required tracking per-repo success and narrowing the manifest update to only successful repos.What could go better
The correctness sub-agent's definition covers 'error handling gaps' and 'failure paths' generically but has no specific guidance for batch operations where individual items can independently succeed or fail. The dangerous pattern is: a function iterates over N items, accumulates a failure count, then gates a state-mutating operation (manifest write, config update, resource cleanup) on
if failures > 0 { return error }— discarding the results of successful items and leaving the system in an inconsistent state where infrastructure and manifest disagree. This pattern is common in CLI tools that operate on multiple repos, resources, or entities. Confidence: HIGH — the correctness sub-agent read the relevant code (it posted a finding on the same file) but did not trace the interaction between the error return and the manifest update that follows it.Proposed change
Add a checklist item to
skills/pr-review/sub-agents/correctness.mdunder the runtime mechanism verification section. The new item should instruct the sub-agent to check batch/loop operations for partial-failure consistency: when a function iterates over multiple items and some can fail independently, verify that (a) successful items' side effects are preserved or rolled back consistently, (b) the function does not gate downstream state mutations on an all-or-nothing error check that discards successful results, and (c) the error message distinguishes which items failed vs succeeded. Include an example pattern to watch for:for item := range items { if err := process(item); err != nil { failed++ } } / if failed > 0 { return error } / updateState(items)— whereupdateStateshould receive only the successful subset.Validation criteria
On the next 5 PRs to fullsend-ai/fullsend that add or modify batch CLI operations (repos install, repos remove, repos uninstall, or similar iterate-and-act patterns), the correctness sub-agent should flag any all-or-nothing error gate that discards successful results, at medium severity or above. Validate by checking whether the sub-agent's findings mention partial failure, per-item tracking, or state consistency for batch operations.
Generated by retro agent from fullsend-ai/fullsend#4081