What happened
On PR #3393 (fixing companion-script resolution for pre/post-code/fix scripts), the author initially submitted a Go CLI subcommand approach adding new files (internal/cli/postrun_fixsummary.go, internal/cli/postrun_precommit.go), new CLI surface area, and release sequencing dependencies. The review agent ran at 17:54 UTC and approved at 18:08 UTC (commit 023a190). A second push (commit cd9ff41) was also approved at 18:41 UTC. At 18:17 UTC — just 9 minutes after the first approval — human reviewer @ifireball pushed back: 'I'm not sure I am on-page with the approach of converting all the sibling scripts into fullsend CLI sub-commands. We should have probably taken #3182 as a warning that we've taken a bad turn.' The author agreed at 18:58 UTC and explained a much simpler shell-only fix. The rework was pushed at 20:24 UTC — a minimal +114/-1 change across 4 shell scripts with no Go code. All initial Go code was discarded. Meanwhile, @ralphbean had also approved the Go approach (commit cd9ff41) at 20:01 UTC before the rework landed.
What could go better
The review agent's intent-coherence sub-agent evaluates scope authorization and architectural fit but has no guidance to consider whether the implementation approach is proportionate to the problem. On this PR, a bug fixable with a ~20-line shell fallback (leveraging existing workspace infrastructure) was initially addressed with new Go files, a new CLI subcommand tree, and cross-repo release sequencing — a significantly larger surface area. The human reviewer caught this by citing prior art (#3182, which had the same class of problem and was fixed by inlining rather than adding new subcommands). The review agent treated the Go approach as architecturally valid (it did fit the forge-abstraction pattern) without questioning whether it was necessary. Confidence in this gap is medium: the review agent correctly evaluated code-level concerns (Qodo caught forge-abstraction violations, security issues), but missed the higher-level 'is this the right approach?' question that humans excelled at.
Proposed change
Add approach-proportionality evaluation guidance to the intent-coherence sub-agent (skills/pr-review/sub-agents/intent-coherence.md). Under the existing architectural coherence responsibilities, add a subsection instructing the sub-agent to: (1) when a PR introduces new CLI surface area, new source files in a different language, or new inter-repo dependencies to fix a bug, consider whether the fix could be achieved with changes scoped to existing files using existing infrastructure; (2) when the PR body or linked issue describes a root cause, verify the implementation addresses it directly rather than building new abstraction layers around it; (3) when prior PRs in the same area (referenced in issue comments or PR body) took a different approach, consider whether the current PR's approach is consistent with lessons learned. This would be a medium-severity finding category (approach-disproportionate). Note: false positive risk exists — some problems genuinely require new infrastructure, so the guidance should emphasize 'clearly simpler alternative exists' rather than 'always prefer minimal changes.'
Validation criteria
On a future PR where a simpler approach clearly exists (e.g., a bug fixable with a config or script change is addressed with new Go files and CLI surface), the intent-coherence sub-agent should emit an approach-disproportionate finding noting the simpler alternative. The finding should not fire on PRs where the more complex approach has legitimate justification (new feature requiring new files, refactoring that simplifies future work). Measure by checking whether the review agent flags approach concerns on at least 1 of the next 5 PRs where a human reviewer pushes back on approach (currently ~0/5 based on this PR's evidence).
Generated by retro agent from #3393
What happened
On PR #3393 (fixing companion-script resolution for pre/post-code/fix scripts), the author initially submitted a Go CLI subcommand approach adding new files (
internal/cli/postrun_fixsummary.go,internal/cli/postrun_precommit.go), new CLI surface area, and release sequencing dependencies. The review agent ran at 17:54 UTC and approved at 18:08 UTC (commit 023a190). A second push (commit cd9ff41) was also approved at 18:41 UTC. At 18:17 UTC — just 9 minutes after the first approval — human reviewer @ifireball pushed back: 'I'm not sure I am on-page with the approach of converting all the sibling scripts into fullsend CLI sub-commands. We should have probably taken #3182 as a warning that we've taken a bad turn.' The author agreed at 18:58 UTC and explained a much simpler shell-only fix. The rework was pushed at 20:24 UTC — a minimal +114/-1 change across 4 shell scripts with no Go code. All initial Go code was discarded. Meanwhile, @ralphbean had also approved the Go approach (commit cd9ff41) at 20:01 UTC before the rework landed.What could go better
The review agent's intent-coherence sub-agent evaluates scope authorization and architectural fit but has no guidance to consider whether the implementation approach is proportionate to the problem. On this PR, a bug fixable with a ~20-line shell fallback (leveraging existing workspace infrastructure) was initially addressed with new Go files, a new CLI subcommand tree, and cross-repo release sequencing — a significantly larger surface area. The human reviewer caught this by citing prior art (#3182, which had the same class of problem and was fixed by inlining rather than adding new subcommands). The review agent treated the Go approach as architecturally valid (it did fit the forge-abstraction pattern) without questioning whether it was necessary. Confidence in this gap is medium: the review agent correctly evaluated code-level concerns (Qodo caught forge-abstraction violations, security issues), but missed the higher-level 'is this the right approach?' question that humans excelled at.
Proposed change
Add approach-proportionality evaluation guidance to the intent-coherence sub-agent (
skills/pr-review/sub-agents/intent-coherence.md). Under the existing architectural coherence responsibilities, add a subsection instructing the sub-agent to: (1) when a PR introduces new CLI surface area, new source files in a different language, or new inter-repo dependencies to fix a bug, consider whether the fix could be achieved with changes scoped to existing files using existing infrastructure; (2) when the PR body or linked issue describes a root cause, verify the implementation addresses it directly rather than building new abstraction layers around it; (3) when prior PRs in the same area (referenced in issue comments or PR body) took a different approach, consider whether the current PR's approach is consistent with lessons learned. This would be a medium-severity finding category (approach-disproportionate). Note: false positive risk exists — some problems genuinely require new infrastructure, so the guidance should emphasize 'clearly simpler alternative exists' rather than 'always prefer minimal changes.'Validation criteria
On a future PR where a simpler approach clearly exists (e.g., a bug fixable with a config or script change is addressed with new Go files and CLI surface), the intent-coherence sub-agent should emit an
approach-disproportionatefinding noting the simpler alternative. The finding should not fire on PRs where the more complex approach has legitimate justification (new feature requiring new files, refactoring that simplifies future work). Measure by checking whether the review agent flags approach concerns on at least 1 of the next 5 PRs where a human reviewer pushes back on approach (currently ~0/5 based on this PR's evidence).Generated by retro agent from #3393