Split out of #102, where CodeRabbit identified this and we agreed it is a follow-up rather than a blocker.
What
critique, security, tests and description each repeat the same shape: call every reviewer, warn on one that failed, handle a parse failure, accumulate spend, anchor per reviewer, clamp to the persona ceiling, pick the headline, then merge for corroboration.
Why it matters
The copies have already diverged once, and the divergence was a live bug: security.rs returned outcome.spend instead of its accumulated tally and so reported no model attribution at all, while the other three assigned it. That shipped green because nothing in the suite covered attribution. Fixed in #102 with a regression test, but the next divergence has the same properties — silent, lane-local, and invisible to a suite that tests each lane separately.
What makes it non-trivial
The loops differ for reasons that are not all accidental, and a helper has to keep these as explicit caller choices rather than flatten them:
- Failure behaviour. Per-file lanes (
critique, security) fail the file so the fan-out records it and the summary names it. Whole-PR lanes (tests, description) must return a skipped outcome instead — failing would report a lane nobody could review as a lane that found nothing.
- Anchoring.
Strict for the code lanes, Demote for description, whose subject has no line to point at.
- Post-processing.
security merges scanner findings afterwards; the others do not.
A helper that hides any of those is worse than the duplication.
Suggested shape
Extract the aggregation and leave placement, anchoring and outcome behaviour as parameters. Then add a test that asserts the invariants across lanes — every lane reports model attribution, every lane clamps a capped reviewer — because per-lane tests are exactly what let the last divergence through.
Split out of #102, where CodeRabbit identified this and we agreed it is a follow-up rather than a blocker.
What
critique,security,testsanddescriptioneach repeat the same shape: call every reviewer, warn on one that failed, handle a parse failure, accumulate spend, anchor per reviewer, clamp to the persona ceiling, pick the headline, then merge for corroboration.Why it matters
The copies have already diverged once, and the divergence was a live bug:
security.rsreturnedoutcome.spendinstead of its accumulated tally and so reported no model attribution at all, while the other three assigned it. That shipped green because nothing in the suite covered attribution. Fixed in #102 with a regression test, but the next divergence has the same properties — silent, lane-local, and invisible to a suite that tests each lane separately.What makes it non-trivial
The loops differ for reasons that are not all accidental, and a helper has to keep these as explicit caller choices rather than flatten them:
critique,security) fail the file so the fan-out records it and the summary names it. Whole-PR lanes (tests,description) must return a skipped outcome instead — failing would report a lane nobody could review as a lane that found nothing.Strictfor the code lanes,Demotefordescription, whose subject has no line to point at.securitymerges scanner findings afterwards; the others do not.A helper that hides any of those is worse than the duplication.
Suggested shape
Extract the aggregation and leave placement, anchoring and outcome behaviour as parameters. Then add a test that asserts the invariants across lanes — every lane reports model attribution, every lane clamps a capped reviewer — because per-lane tests are exactly what let the last divergence through.