feat(#1885): add diff-size calibration to review sub-agents - #1903
Conversation
The style and intent sub-agents applied uniform investigation depth regardless of diff size, causing excessive tool calls on trivial changes (e.g., 59 tool calls for a 4-line YAML digest swap). Style sub-agent (style-conventions.md): - Raise trivial diff threshold from 10 to 20 lines - Prohibit reading files outside changed directories - Prohibit shell pipelines (awk/sed) for formatting analysis - Prohibit git log/blame searches on trivial diffs - Add value-only diff category with early-exit guidance Intent sub-agent (intent-coherence.md): - Add diff-size-aware investigation depth section - Skip CLAUDE.md/AGENTS.md/ADR reads for value-only changes - Treat mechanical changes without linked issues as implicitly authorized - Add revert PR recognition (revert-* branch, Revert "..." commit/title) as self-authorizing for scope - Focus revert review on completeness and extra changes Note: make lint and go test could not run due to sandbox Go toolchain permission error (unrelated to this change). Manual verification required. Closes #1885 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Site previewPreview: https://c56b6ae1-site.fullsend-ai.workers.dev Commit: |
|
/fs-review |
ReviewFindingsLow
Info
Previous runReviewFindingsLow
Info
Previous runReviewFindingsLow
Info
Previous run (2)ReviewFindingsHigh
Medium
Low
Info
|
|
/fs-fix |
- Require diff-inverse verification for revert PRs, not just metadata signals - Require at least two revert signals (branch, commit msg, title) to match - Replace silent "report no findings" with info-level finding for inferred auth - Align section heading to "Exploration budget" across both sub-agents - Consolidate "Value-only diffs" into "Early exit for mechanical changes" - Add precedence note for value-only changes vs size-based categories Addresses review feedback on #1903 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
🔧 Fix agent — iteration 1 (human-triggered)Addressed 6 of 7 review findings. The high-severity privilege-escalation finding was fixed by requiring diff-inverse verification and multiple metadata signals for revert PRs. The authorization-bypass finding was fixed by replacing silent skip with an info-level finding. Naming inconsistency was aligned to Exploration budget. Value-only/mechanical sections were consolidated with a precedence note. Disagreed with 1 finding (off-by-one ambiguity) as the phrasing is unambiguous. Fixed (6):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
|
/fs-fix |
- Add explicit missing-authorization guidance for non-trivial diffs without linked issues (edge-case finding) - Rename "Revert PRs" to "Revert PR authorization" for heading consistency (section-naming finding) - Clarify "directory of each changed file" for multi-directory PRs (instruction-specificity finding) - Shorten "Early exit for mechanical and value-only changes" to "Early exit criteria" (section-naming finding) Addresses review feedback on #1903 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
🔧 Fix agent — iteration 2 (human-triggered)Addressed 4 of 6 low-severity review findings with targeted fixes. Disagreed with 2 findings: design-direction (out of scope future improvement) and privilege-escalation residual (no actionable fix for LLM verification limitation). Fixed (4):
Disagreed (2):
Tests: passed Updated by fullsend fix agent |
|
|
||
| ## Revert PR authorization | ||
|
|
||
| A PR is a candidate revert if **at least two** of the following signals |
There was a problem hiding this comment.
[low] design-direction
Revert PR detection and verification lives entirely in the intent sub-agent. If other sub-agents later need revert awareness, the logic would need to be duplicated. Consider moving revert classification to the orchestrator's triage phase so sub-agents receive a pr_type: revert flag in their context package.
| Before exploring context files, assess the diff size and nature. | ||
|
|
||
| **Trivial diffs (under 10 changed lines, single concern):** | ||
| **Trivial diffs (under 20 changed lines, single concern):** |
There was a problem hiding this comment.
[info] design-direction
The trivial-diff threshold (20 changed lines) is consistent across both sub-agents but not anchored to project-wide architectural guidance. Consider documenting diff-size classification thresholds in a central location.
The style and intent sub-agents applied uniform investigation depth regardless of diff size, causing excessive tool calls on trivial changes (e.g., 59 tool calls for a 4-line YAML digest swap).
Style sub-agent (style-conventions.md):
Intent sub-agent (intent-coherence.md):
authorized
commit/title) as self-authorizing for scope
Note: make lint and go test could not run due to sandbox Go toolchain permission error (unrelated to this change). Manual verification required.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Closes #1885
Post-script verification
agent/1885-sub-agent-diff-calibration)7e768afe50ea067e39066e4adc5451f22f85cec0..HEAD)