fix(pr-review): resolve verdict message contradiction and blocked status limbo (#1151)
* fix(pr-review): resolve verdict message contradiction and blocked status limbo Backend fixes: - Fix verdict reasoning to include high/medium findings when branch is behind - Previously, when branch was behind AND there were medium findings, the reasoning said "you can merge" while bottom line said "3 issues require attention" - a contradiction - Now properly combines branch-behind message with findings count Frontend fixes: - Add "Post Status" button for BLOCKED/NEEDS_REVISION verdicts with no findings - Handles edge case where structured output parsing fails but verdict exists - Users were stuck in limbo with "Pending Post" status but no actionable button - Button posts the review summary (with blockers) as a comment to GitHub Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(pr-review): address code review findings - Reset blocked status state variables when switching PRs (prevents stale state) - Keep blockedStatusMessageFooter in English for French locale (GitHub comments policy) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
@@ -1097,7 +1097,16 @@ class GitHubOrchestrator:
|
||||
# Branch behind is a soft blocker - NEEDS_REVISION, not BLOCKED
|
||||
elif is_branch_behind:
|
||||
verdict = MergeVerdict.NEEDS_REVISION
|
||||
reasoning = BRANCH_BEHIND_REASONING
|
||||
if high or medium:
|
||||
# Branch behind + code issues that need addressing
|
||||
total = len(high) + len(medium)
|
||||
reasoning = (
|
||||
f"{BRANCH_BEHIND_REASONING} "
|
||||
f"{total} issue(s) must be addressed ({len(high)} required, {len(medium)} recommended)."
|
||||
)
|
||||
else:
|
||||
# Just branch behind, no code issues
|
||||
reasoning = BRANCH_BEHIND_REASONING
|
||||
if low:
|
||||
reasoning += f" {len(low)} non-blocking suggestion(s) to consider."
|
||||
else:
|
||||
|
||||
@@ -967,7 +967,16 @@ The SDK will run invoked agents in parallel automatically.
|
||||
# Branch behind is a soft blocker - NEEDS_REVISION, not BLOCKED
|
||||
elif is_branch_behind:
|
||||
verdict = MergeVerdict.NEEDS_REVISION
|
||||
reasoning = BRANCH_BEHIND_REASONING
|
||||
if high or medium:
|
||||
# Branch behind + code issues that need addressing
|
||||
total = len(high) + len(medium)
|
||||
reasoning = (
|
||||
f"{BRANCH_BEHIND_REASONING} "
|
||||
f"{total} issue(s) must be addressed ({len(high)} required, {len(medium)} recommended)."
|
||||
)
|
||||
else:
|
||||
# Just branch behind, no code issues
|
||||
reasoning = BRANCH_BEHIND_REASONING
|
||||
if low:
|
||||
reasoning += f" {len(low)} non-blocking suggestion(s) to consider."
|
||||
else:
|
||||
|
||||
@@ -100,6 +100,10 @@ export function PRDetail({
|
||||
const [cleanReviewPosted, setCleanReviewPosted] = useState(false);
|
||||
const [cleanReviewError, setCleanReviewError] = useState<string | null>(null);
|
||||
const [showCleanReviewErrorDetails, setShowCleanReviewErrorDetails] = useState(false);
|
||||
// Blocked status posting state (for BLOCKED/NEEDS_REVISION verdicts with no findings)
|
||||
const [isPostingBlockedStatus, setIsPostingBlockedStatus] = useState(false);
|
||||
const [blockedStatusPosted, setBlockedStatusPosted] = useState(false);
|
||||
const [blockedStatusError, setBlockedStatusError] = useState<string | null>(null);
|
||||
const [isMerging, setIsMerging] = useState(false);
|
||||
// Initialize with store value, then sync and update via local checks
|
||||
const [newCommitsCheck, setNewCommitsCheck] = useState<NewCommitsCheck | null>(initialNewCommitsCheck ?? null);
|
||||
@@ -270,6 +274,10 @@ export function PRDetail({
|
||||
setCleanReviewError(null);
|
||||
setIsPostingCleanReview(false);
|
||||
setShowCleanReviewErrorDetails(false);
|
||||
// Reset blocked status state as well
|
||||
setBlockedStatusPosted(false);
|
||||
setBlockedStatusError(null);
|
||||
setIsPostingBlockedStatus(false);
|
||||
}, [pr.number]);
|
||||
|
||||
// Check for workflows awaiting approval (fork PRs) when PR changes or review completes
|
||||
@@ -681,6 +689,46 @@ ${t('prReview.cleanReviewMessageFooter')}`;
|
||||
}
|
||||
};
|
||||
|
||||
// Post blocked status comment when verdict is BLOCKED/NEEDS_REVISION but no findings
|
||||
// This handles the edge case where structured output parsing fails but we still have a verdict
|
||||
const handlePostBlockedStatus = async () => {
|
||||
if (!reviewResult) return;
|
||||
|
||||
// Capture current PR number to prevent state leaks across PR switches
|
||||
const currentPr = pr.number;
|
||||
|
||||
setIsPostingBlockedStatus(true);
|
||||
setBlockedStatusError(null);
|
||||
try {
|
||||
// Format the blocked status comment - post the summary which contains blockers
|
||||
const blockedStatusMessage = `${t('prReview.blockedStatusMessageTitle')}
|
||||
|
||||
${reviewResult.summary}
|
||||
|
||||
---
|
||||
|
||||
${t('prReview.blockedStatusMessageFooter')}`;
|
||||
|
||||
await Promise.resolve(onPostComment(blockedStatusMessage));
|
||||
|
||||
// Only mark as posted on success if PR hasn't changed
|
||||
if (pr.number === currentPr) {
|
||||
setBlockedStatusPosted(true);
|
||||
setBlockedStatusError(null);
|
||||
}
|
||||
} catch (err) {
|
||||
console.error('Failed to post blocked status comment:', err);
|
||||
const fullError = err instanceof Error ? err.message : String(err);
|
||||
if (pr.number === currentPr) {
|
||||
setBlockedStatusError(fullError);
|
||||
}
|
||||
} finally {
|
||||
if (pr.number === currentPr) {
|
||||
setIsPostingBlockedStatus(false);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
const handleMerge = async () => {
|
||||
setIsMerging(true);
|
||||
try {
|
||||
@@ -782,6 +830,29 @@ ${t('prReview.cleanReviewMessageFooter')}`;
|
||||
</Button>
|
||||
)}
|
||||
|
||||
{/* Post Blocked Status button - shows when verdict is BLOCKED/NEEDS_REVISION but no findings */}
|
||||
{/* This handles the edge case where structured output parsing fails but we still have a verdict */}
|
||||
{selectedCount === 0 && !hasPostedFindings && !blockedStatusPosted && reviewResult?.overallStatus === 'request_changes' && (
|
||||
<Button
|
||||
onClick={handlePostBlockedStatus}
|
||||
disabled={isPostingBlockedStatus || isPosting}
|
||||
variant="secondary"
|
||||
className="flex-1 sm:flex-none"
|
||||
>
|
||||
{isPostingBlockedStatus ? (
|
||||
<>
|
||||
<Loader2 className="h-4 w-4 mr-2 animate-spin" />
|
||||
{t('prReview.postingBlockedStatus')}
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
<AlertTriangle className="h-4 w-4 mr-2" />
|
||||
{t('prReview.postBlockedStatus')}
|
||||
</>
|
||||
)}
|
||||
</Button>
|
||||
)}
|
||||
|
||||
{/* Approve button - consolidated logic to avoid duplicate buttons */}
|
||||
{/* Don't show when overallStatus is 'request_changes' (e.g., workflows blocked, or other issues) */}
|
||||
{isCleanReview && !hasPostedFindings && reviewResult?.overallStatus !== 'request_changes' && (
|
||||
@@ -897,6 +968,22 @@ ${t('prReview.cleanReviewMessageFooter')}`;
|
||||
{cleanReviewError}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Blocked status posted success message */}
|
||||
{blockedStatusPosted && !postSuccess && !cleanReviewPosted && (
|
||||
<div className="ml-auto flex items-center gap-2 text-amber-600 text-sm font-medium animate-pulse">
|
||||
<CheckCircle className="h-4 w-4" />
|
||||
{t('prReview.blockedStatusPosted')}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Blocked status error display */}
|
||||
{blockedStatusError && (
|
||||
<div className="ml-auto flex items-center gap-2 text-destructive text-sm font-medium">
|
||||
<XCircle className="h-4 w-4" />
|
||||
{t('prReview.failedPostBlockedStatus')}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
|
||||
|
||||
@@ -339,6 +339,12 @@
|
||||
"failedPostCleanReview": "Failed to post clean review",
|
||||
"viewErrorDetails": "View details",
|
||||
"hideErrorDetails": "Hide details",
|
||||
"postBlockedStatus": "Post Status",
|
||||
"postingBlockedStatus": "Posting...",
|
||||
"blockedStatusPosted": "Status posted to PR",
|
||||
"blockedStatusMessageTitle": "## 🤖 Auto Claude PR Review",
|
||||
"blockedStatusMessageFooter": "*This review identified blockers that must be resolved before merge. Generated by Auto Claude.*",
|
||||
"failedPostBlockedStatus": "Failed to post status",
|
||||
"branchSynced": "Branch synced ({{count}} commit from base)",
|
||||
"branchSynced_plural": "Branch synced ({{count}} commits from base)",
|
||||
"newCommitsOverlap": "{{count}} new commit ({{files}} finding file(s) modified)",
|
||||
|
||||
@@ -348,6 +348,12 @@
|
||||
"failedPostCleanReview": "Échec de la publication de la révision",
|
||||
"viewErrorDetails": "Voir les détails",
|
||||
"hideErrorDetails": "Masquer les détails",
|
||||
"postBlockedStatus": "Publier le statut",
|
||||
"postingBlockedStatus": "Publication...",
|
||||
"blockedStatusPosted": "Statut publié sur la PR",
|
||||
"blockedStatusMessageTitle": "## 🤖 Auto Claude PR Review",
|
||||
"blockedStatusMessageFooter": "*This review identified blockers that must be resolved before merge. Generated by Auto Claude.*",
|
||||
"failedPostBlockedStatus": "Échec de la publication du statut",
|
||||
"logs": {
|
||||
"agentActivity": "Activité des agents",
|
||||
"showMore": "Afficher {{count}} de plus",
|
||||
|
||||
Reference in New Issue
Block a user