What happened
On PR #7066, the review agent ran 4 times (run 1, run 2, run 3, run 4) at a total cost of $15.14. It found two real but cosmetic issues: missing flag documentation in docs/cli/repos.md (medium) and a confusingly similar test name (low). It never flagged the applyDeprecatedVendorBinaryFlag call as a no-op, even though this function looks up a flag (--vendor-fullsend-binary) that was never offered on repos install. The human reviewer identified this immediately and triggered a fix cycle ($5.38 additional cost). The review agent approved on iteration 3 with this dead code still present.
What could go better
The code-review skill's 'Correctness' dimension should cover dead/no-op code, especially when a PR adapts a pattern from one command to another. The review agent should trace function calls to determine whether they have any effect. applyDeprecatedVendorBinaryFlag is a deprecation shim — it applies a mapping from old flag name to new flag name. If the old flag was never offered on the target command, the mapping can never trigger. This is detectable by reading the function body and cross-referencing with the command's flag history. The 'Intent & coherence' dimension (scope-appropriateness check) is another angle: including a deprecation shim for a flag that never existed on the target command is out of scope. Confidence: moderate — detecting no-op code requires following control flow and understanding command history, which is harder than detecting the cosmetic issues the review agent did find. However, the function name itself contains 'Deprecated' and 'Binary' which should trigger deeper investigation. Related issue #3517 covers a different pattern (implementation vs. stated purpose) and does not address dead-code detection.
Proposed change
Add a heuristic to the code-review skill's Correctness dimension (skills/code-review/SKILL.md) in fullsend-ai/agents: 'When reviewing a PR that adapts a pattern from one command/module to another, verify that backwards-compatibility and deprecation code in the adapted pattern applies to the target context. Deprecation shims map old interfaces to new ones — if the target command never offered the old interface, the shim is a no-op and should be flagged. Signal words in function names (Deprecated, Legacy, Compat, Migration) indicate context-dependent code that requires applicability verification.' This complements the existing consumer-completeness and runtime-mechanism checks in the Correctness dimension.
Validation criteria
On the next 3 PRs where a pattern is copied between commands and includes backwards-compatibility or deprecation code, the review agent should evaluate whether each such component applies to the target context. If a deprecation shim references a flag or interface that was never offered on the target command, it should be flagged as a no-op finding at medium severity or higher.
Generated by retro agent from fullsend-ai/fullsend#7066
What happened
On PR #7066, the review agent ran 4 times (run 1, run 2, run 3, run 4) at a total cost of $15.14. It found two real but cosmetic issues: missing flag documentation in docs/cli/repos.md (medium) and a confusingly similar test name (low). It never flagged the
applyDeprecatedVendorBinaryFlagcall as a no-op, even though this function looks up a flag (--vendor-fullsend-binary) that was never offered onrepos install. The human reviewer identified this immediately and triggered a fix cycle ($5.38 additional cost). The review agent approved on iteration 3 with this dead code still present.What could go better
The code-review skill's 'Correctness' dimension should cover dead/no-op code, especially when a PR adapts a pattern from one command to another. The review agent should trace function calls to determine whether they have any effect.
applyDeprecatedVendorBinaryFlagis a deprecation shim — it applies a mapping from old flag name to new flag name. If the old flag was never offered on the target command, the mapping can never trigger. This is detectable by reading the function body and cross-referencing with the command's flag history. The 'Intent & coherence' dimension (scope-appropriateness check) is another angle: including a deprecation shim for a flag that never existed on the target command is out of scope. Confidence: moderate — detecting no-op code requires following control flow and understanding command history, which is harder than detecting the cosmetic issues the review agent did find. However, the function name itself contains 'Deprecated' and 'Binary' which should trigger deeper investigation. Related issue #3517 covers a different pattern (implementation vs. stated purpose) and does not address dead-code detection.Proposed change
Add a heuristic to the code-review skill's Correctness dimension (
skills/code-review/SKILL.md) infullsend-ai/agents: 'When reviewing a PR that adapts a pattern from one command/module to another, verify that backwards-compatibility and deprecation code in the adapted pattern applies to the target context. Deprecation shims map old interfaces to new ones — if the target command never offered the old interface, the shim is a no-op and should be flagged. Signal words in function names (Deprecated, Legacy, Compat, Migration) indicate context-dependent code that requires applicability verification.' This complements the existing consumer-completeness and runtime-mechanism checks in the Correctness dimension.Validation criteria
On the next 3 PRs where a pattern is copied between commands and includes backwards-compatibility or deprecation code, the review agent should evaluate whether each such component applies to the target context. If a deprecation shim references a flag or interface that was never offered on the target command, it should be flagged as a no-op finding at medium severity or higher.
Generated by retro agent from fullsend-ai/fullsend#7066