feat(pr-review): add prominent verdict summary to PR review comments (#780)
* feat(pr-review): add prominent verdict summary to PR review comments Add a "Bottom Line" summary that appears prominently right after the review header, making it easy to quickly scan the key outcome without scrolling through the full review. The summary intelligently distinguishes between: - Ready to merge (all clear) - Ready once CI passes (only waiting on CI, no code issues) - Needs revision (actual code issues to fix) - Blocked (merge conflicts, failing CI, etc.) This improves UX by showing the verdict at a glance - especially helpful when CI is pending but the code review is actually approved. Changes: - parallel_followup_reviewer.py: Add ci_status param and _generate_bottom_line() - orchestrator.py: Add matching _generate_bottom_line() for initial reviews 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR feedback - logic and consistency improvements Fixes based on Gemini, Cursor Bot, and Auto Claude PR Review feedback: - HIGH: Reorder NEEDS_REVISION conditions to check code issues (blocking_findings, code_blockers, new_count) BEFORE checking pending CI. This prevents misleading "Ready once CI passes" when code issues actually exist. - MEDIUM: Standardize emojis across both reviewers: - BLOCKED: Use 🔴 consistently (was 🚫 in followup) - MERGE_WITH_CHANGES: Use 🟡 consistently (was ⚠️ in followup) - MEDIUM: Fix type inconsistency - awaiting_approval default changed from False (bool) to 0 (int) to match the integer count returned by CI status. - FIX: Apply ruff formatting for CI compliance (line wrapping). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Gemini Code Assist <coderabbit@users.noreply.github.com> * fix: complete emoji standardization for full consistency Align all emojis in parallel_followup_reviewer.py with orchestrator.py: - status_emoji dict: Use 🟠 for NEEDS_REVISION (was 🔄), 🟡 for MERGE_WITH_CHANGES (was ⚠️), 🔴 for BLOCKED (was 🚫) - _generate_bottom_line: Use 🟠 for NEEDS_REVISION (was 🔄) Now both files use identical emoji conventions: - ✅ READY_TO_MERGE - 🟡 MERGE_WITH_CHANGES - 🟠 NEEDS_REVISION - 🔴 BLOCKED 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Gemini Code Assist <coderabbit@users.noreply.github.com> Co-authored-by: Alex <63423455+AlexMadera@users.noreply.github.com>
This commit is contained in:
Andy
@@ -450,6 +450,7 @@ class GitHubOrchestrator:
|
||||
structural_issues=structural_issues,
|
||||
ai_triages=ai_triages,
|
||||
risk_assessment=risk_assessment,
|
||||
ci_status=ci_status,
|
||||
)
|
||||
|
||||
# Get HEAD SHA for follow-up review tracking
|
||||
@@ -1026,6 +1027,7 @@ class GitHubOrchestrator:
|
||||
structural_issues: list[StructuralIssue],
|
||||
ai_triages: list[AICommentTriage],
|
||||
risk_assessment: dict,
|
||||
ci_status: dict | None = None,
|
||||
) -> str:
|
||||
"""Generate enhanced summary with verdict, risk, and actionable next steps."""
|
||||
verdict_emoji = {
|
||||
@@ -1035,8 +1037,19 @@ class GitHubOrchestrator:
|
||||
MergeVerdict.BLOCKED: "🔴",
|
||||
}
|
||||
|
||||
# Generate bottom line for quick scanning
|
||||
bottom_line = self._generate_bottom_line(
|
||||
verdict=verdict,
|
||||
ci_status=ci_status,
|
||||
blockers=blockers,
|
||||
findings=findings,
|
||||
)
|
||||
|
||||
lines = [
|
||||
f"### Merge Verdict: {verdict_emoji.get(verdict, '⚪')} {verdict.value.upper().replace('_', ' ')}",
|
||||
"",
|
||||
f"> {bottom_line}",
|
||||
"",
|
||||
verdict_reasoning,
|
||||
"",
|
||||
"### Risk Assessment",
|
||||
@@ -1103,6 +1116,70 @@ class GitHubOrchestrator:
|
||||
|
||||
return "\n".join(lines)
|
||||
|
||||
def _generate_bottom_line(
|
||||
self,
|
||||
verdict: MergeVerdict,
|
||||
ci_status: dict | None,
|
||||
blockers: list[str],
|
||||
findings: list[PRReviewFinding],
|
||||
) -> str:
|
||||
"""Generate a one-line summary for quick scanning at the top of the review."""
|
||||
# Check CI status
|
||||
ci = ci_status or {}
|
||||
pending_ci = ci.get("pending", 0)
|
||||
failing_ci = ci.get("failing", 0)
|
||||
awaiting_approval = ci.get("awaiting_approval", 0)
|
||||
|
||||
# Count blocking findings and issues
|
||||
blocking_findings = [
|
||||
f for f in findings if f.severity.value in ("critical", "high", "medium")
|
||||
]
|
||||
code_blockers = [
|
||||
b for b in blockers if "CI" not in b and "Merge Conflict" not in b
|
||||
]
|
||||
has_merge_conflicts = any("Merge Conflict" in b for b in blockers)
|
||||
|
||||
# Determine the bottom line based on verdict and context
|
||||
if verdict == MergeVerdict.READY_TO_MERGE:
|
||||
return (
|
||||
"**✅ Ready to merge** - All checks passing, no blocking issues found."
|
||||
)
|
||||
|
||||
elif verdict == MergeVerdict.BLOCKED:
|
||||
if has_merge_conflicts:
|
||||
return "**🔴 Blocked** - Merge conflicts must be resolved before merge."
|
||||
elif failing_ci > 0:
|
||||
return f"**🔴 Blocked** - {failing_ci} CI check(s) failing. Fix CI before merge."
|
||||
elif awaiting_approval > 0:
|
||||
return "**🔴 Blocked** - Awaiting maintainer approval for fork PR workflow."
|
||||
elif blocking_findings:
|
||||
return f"**🔴 Blocked** - {len(blocking_findings)} critical/high/medium issue(s) must be fixed."
|
||||
else:
|
||||
return "**🔴 Blocked** - Critical issues must be resolved before merge."
|
||||
|
||||
elif verdict == MergeVerdict.NEEDS_REVISION:
|
||||
# Key insight: distinguish "waiting on CI" from "needs code fixes"
|
||||
# Check code issues FIRST before checking pending CI
|
||||
if blocking_findings:
|
||||
return f"**🟠 Needs revision** - {len(blocking_findings)} issue(s) require attention."
|
||||
elif code_blockers:
|
||||
return f"**🟠 Needs revision** - {len(code_blockers)} structural/other issue(s) require attention."
|
||||
elif pending_ci > 0:
|
||||
# Only show "Ready once CI passes" when no code issues exist
|
||||
return f"**⏳ Ready once CI passes** - {pending_ci} check(s) pending, no blocking code issues."
|
||||
else:
|
||||
return "**🟠 Needs revision** - See details below."
|
||||
|
||||
elif verdict == MergeVerdict.MERGE_WITH_CHANGES:
|
||||
if pending_ci > 0:
|
||||
return (
|
||||
"**🟡 Can merge once CI passes** - Minor suggestions, no blockers."
|
||||
)
|
||||
else:
|
||||
return "**🟡 Can merge** - Minor suggestions noted, no blockers."
|
||||
|
||||
return "**📝 Review complete** - See details below."
|
||||
|
||||
def _format_review_body(self, result: PRReviewResult) -> str:
|
||||
"""Format the review body for posting to GitHub."""
|
||||
return result.summary
|
||||
|
||||
@@ -627,6 +627,7 @@ The SDK will run invoked agents in parallel automatically.
|
||||
dismissed_false_positive_count=dismissed_count,
|
||||
confirmed_valid_count=confirmed_count,
|
||||
needs_human_review_count=needs_human_count,
|
||||
ci_status=context.ci_status,
|
||||
)
|
||||
|
||||
# Map verdict to overall_status
|
||||
@@ -977,13 +978,15 @@ The SDK will run invoked agents in parallel automatically.
|
||||
dismissed_false_positive_count: int = 0,
|
||||
confirmed_valid_count: int = 0,
|
||||
needs_human_review_count: int = 0,
|
||||
ci_status: dict | None = None,
|
||||
) -> str:
|
||||
"""Generate a human-readable summary of the follow-up review."""
|
||||
# Use same emojis as orchestrator.py for consistency
|
||||
status_emoji = {
|
||||
MergeVerdict.READY_TO_MERGE: "✅",
|
||||
MergeVerdict.MERGE_WITH_CHANGES: "⚠️",
|
||||
MergeVerdict.NEEDS_REVISION: "🔄",
|
||||
MergeVerdict.BLOCKED: "🚫",
|
||||
MergeVerdict.MERGE_WITH_CHANGES: "🟡",
|
||||
MergeVerdict.NEEDS_REVISION: "🟠",
|
||||
MergeVerdict.BLOCKED: "🔴",
|
||||
}
|
||||
|
||||
emoji = status_emoji.get(verdict, "📝")
|
||||
@@ -991,6 +994,15 @@ The SDK will run invoked agents in parallel automatically.
|
||||
", ".join(agents_invoked) if agents_invoked else "orchestrator only"
|
||||
)
|
||||
|
||||
# Generate a prominent bottom-line summary for quick scanning
|
||||
bottom_line = self._generate_bottom_line(
|
||||
verdict=verdict,
|
||||
ci_status=ci_status,
|
||||
unresolved_count=unresolved_count,
|
||||
new_count=new_count,
|
||||
blockers=blockers,
|
||||
)
|
||||
|
||||
# Build validation section if there are validation results
|
||||
validation_section = ""
|
||||
if (
|
||||
@@ -1016,6 +1028,8 @@ The SDK will run invoked agents in parallel automatically.
|
||||
|
||||
summary = f"""## {emoji} Follow-up Review: {verdict.value.replace("_", " ").title()}
|
||||
|
||||
> {bottom_line}
|
||||
|
||||
### Resolution Status
|
||||
- ✅ **Resolved**: {resolved_count} previous findings addressed
|
||||
- ❌ **Unresolved**: {unresolved_count} previous findings remain
|
||||
@@ -1031,3 +1045,65 @@ Agents invoked: {agents_str}
|
||||
*This is an AI-generated follow-up review using parallel specialist analysis with finding validation.*
|
||||
"""
|
||||
return summary
|
||||
|
||||
def _generate_bottom_line(
|
||||
self,
|
||||
verdict: MergeVerdict,
|
||||
ci_status: dict | None,
|
||||
unresolved_count: int,
|
||||
new_count: int,
|
||||
blockers: list[str],
|
||||
) -> str:
|
||||
"""Generate a one-line summary for quick scanning at the top of the review."""
|
||||
# Check CI status
|
||||
ci = ci_status or {}
|
||||
pending_ci = ci.get("pending", 0)
|
||||
failing_ci = ci.get("failing", 0)
|
||||
awaiting_approval = ci.get("awaiting_approval", 0)
|
||||
|
||||
# Count blocking issues (excluding CI-related ones)
|
||||
code_blockers = [
|
||||
b for b in blockers if "CI" not in b and "Merge Conflict" not in b
|
||||
]
|
||||
has_merge_conflicts = any("Merge Conflict" in b for b in blockers)
|
||||
|
||||
# Determine the bottom line based on verdict and context
|
||||
if verdict == MergeVerdict.READY_TO_MERGE:
|
||||
return "**✅ Ready to merge** - All checks passing and findings addressed."
|
||||
|
||||
elif verdict == MergeVerdict.BLOCKED:
|
||||
if has_merge_conflicts:
|
||||
return "**🔴 Blocked** - Merge conflicts must be resolved before merge."
|
||||
elif failing_ci > 0:
|
||||
return f"**🔴 Blocked** - {failing_ci} CI check(s) failing. Fix CI before merge."
|
||||
elif awaiting_approval > 0:
|
||||
return "**🔴 Blocked** - Awaiting maintainer approval for fork PR workflow."
|
||||
elif code_blockers:
|
||||
return f"**🔴 Blocked** - {len(code_blockers)} blocking issue(s) require fixes."
|
||||
else:
|
||||
return "**🔴 Blocked** - Critical issues must be resolved before merge."
|
||||
|
||||
elif verdict == MergeVerdict.NEEDS_REVISION:
|
||||
# Key insight: distinguish "waiting on CI" from "needs code fixes"
|
||||
# Check code issues FIRST before checking pending CI
|
||||
if unresolved_count > 0:
|
||||
return f"**🟠 Needs revision** - {unresolved_count} unresolved finding(s) from previous review."
|
||||
elif code_blockers:
|
||||
return f"**🟠 Needs revision** - {len(code_blockers)} blocking issue(s) require fixes."
|
||||
elif new_count > 0:
|
||||
return f"**🟠 Needs revision** - {new_count} new issue(s) found in recent changes."
|
||||
elif pending_ci > 0:
|
||||
# Only show "Ready once CI passes" when no code issues exist
|
||||
return f"**⏳ Ready once CI passes** - {pending_ci} check(s) pending, all findings addressed."
|
||||
else:
|
||||
return "**🟠 Needs revision** - See details below."
|
||||
|
||||
elif verdict == MergeVerdict.MERGE_WITH_CHANGES:
|
||||
if pending_ci > 0:
|
||||
return (
|
||||
"**🟡 Can merge once CI passes** - Minor suggestions, no blockers."
|
||||
)
|
||||
else:
|
||||
return "**🟡 Can merge** - Minor suggestions noted, no blockers."
|
||||
|
||||
return "**📝 Review complete** - See details below."
|
||||
|
||||
Reference in New Issue
Block a user