What happened
Across 4 fix iterations on PR #145, the agent tried four different approaches: (1) --privileged + stat /proc/1 chown, (2) unified GetUser() + removed :U, (3) restored :U + --security-opt, (4) STORAGE_DRIVER=vfs. Because the agent cannot force-push, each iteration was additive. The final state contained unused methods (SetPrivileged, WithPrivileged, WithSecurityOpt) from abandoned strategies. The review agent flagged these as low-severity 'dead-code' findings in its final review but still approved. Human reviewer chmeliik noted the PR had become 'a mess of commits that undo previous solutions and try a new one.'
What could go better
When the fix agent changes strategy between iterations, it should clean up code added by previous approaches that is no longer used. Existing issue #2329 covers the review agent detecting dead code, but the fix agent should proactively remove it before pushing. This would keep PRs reviewable and reduce the 'wall of abandoned code' problem that contributed to this PR's rejection. Existing issue #819 (squash-merge) would help post-merge but doesn't solve the review-time problem. Confidence: MEDIUM — the fix agent may not always have full context about which code came from prior iterations, but in this case the methods were clearly unused and the agent itself introduced them.
Proposed change
Add guidance to the fix agent definition (agents/fix.md) instructing it to: (1) review all code added by previous fix iterations on the same PR before pushing a new commit, (2) identify and remove any methods, fields, or imports that were added in prior iterations but are no longer used by the current approach, and (3) include a note in the commit message about what was cleaned up. A prompt section like: 'Before pushing your fix, review the full diff of the PR branch against the base branch. If previous fix iterations added code (methods, struct fields, imports) that your current approach does not use, remove that dead code in your commit. The PR should be clean and self-consistent, not an archaeological record of abandoned strategies.'
Validation criteria
On the next 5 PRs where the fix agent runs 2+ iterations with strategy changes, the final commit should not contain unused methods or struct fields introduced by earlier iterations. The review agent's dead-code findings should drop to zero on fix-agent PRs within 60 days.
Generated by retro agent from konflux-ci/konflux-build-cli#145
What happened
Across 4 fix iterations on PR #145, the agent tried four different approaches: (1)
--privileged+stat /proc/1chown, (2) unifiedGetUser()+ removed:U, (3) restored:U+--security-opt, (4)STORAGE_DRIVER=vfs. Because the agent cannot force-push, each iteration was additive. The final state contained unused methods (SetPrivileged,WithPrivileged,WithSecurityOpt) from abandoned strategies. The review agent flagged these as low-severity 'dead-code' findings in its final review but still approved. Human reviewer chmeliik noted the PR had become 'a mess of commits that undo previous solutions and try a new one.'What could go better
When the fix agent changes strategy between iterations, it should clean up code added by previous approaches that is no longer used. Existing issue #2329 covers the review agent detecting dead code, but the fix agent should proactively remove it before pushing. This would keep PRs reviewable and reduce the 'wall of abandoned code' problem that contributed to this PR's rejection. Existing issue #819 (squash-merge) would help post-merge but doesn't solve the review-time problem. Confidence: MEDIUM — the fix agent may not always have full context about which code came from prior iterations, but in this case the methods were clearly unused and the agent itself introduced them.
Proposed change
Add guidance to the fix agent definition (agents/fix.md) instructing it to: (1) review all code added by previous fix iterations on the same PR before pushing a new commit, (2) identify and remove any methods, fields, or imports that were added in prior iterations but are no longer used by the current approach, and (3) include a note in the commit message about what was cleaned up. A prompt section like: 'Before pushing your fix, review the full diff of the PR branch against the base branch. If previous fix iterations added code (methods, struct fields, imports) that your current approach does not use, remove that dead code in your commit. The PR should be clean and self-consistent, not an archaeological record of abandoned strategies.'
Validation criteria
On the next 5 PRs where the fix agent runs 2+ iterations with strategy changes, the final commit should not contain unused methods or struct fields introduced by earlier iterations. The review agent's dead-code findings should drop to zero on fix-agent PRs within 60 days.
Generated by retro agent from konflux-ci/konflux-build-cli#145