Skip to content

Commit 6ecc656

Browse files
sirily11claude
andcommitted
fix(code-review): don't auto-retry a cancelled review
The Code Review hook re-prompted the original thread on both an explicit reviewer FAIL and an "unknown" verdict (a reply with no REVIEW_RESULT: marker). A manually cancelled review thread produces exactly that markerless reply, so it was mistaken for "reviewer wants changes" and kicked off an unwanted auto-retry fix turn. Split the cases: only an explicit FAIL re-prompts (bounded by maxReviewRounds). An unknown verdict now records not-passed and finishes the card without re-prompting, so a cancelled or interrupted review never auto-retries. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
1 parent b0d1c04 commit 6ecc656

1 file changed

Lines changed: 40 additions & 5 deletions

File tree

RxCode/Services/Hooks/hooks/CodeReviewHook.swift

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@ import RxCodeCore
88
/// files, the user's task, and the agent's final response. The reviewer ends its
99
/// reply with `REVIEW_RESULT: PASS` or `REVIEW_RESULT: FAIL`:
1010
/// - PASS → records the verdict so `CommitPushHook` may proceed.
11-
/// - FAIL (or no verdict) → sends the review notes back into the original
12-
/// thread as a follow-up prompt so the agent fixes the issues and is then
13-
/// re-reviewed. Bounded by `maxReviewRounds` to stop a fix→fail→fix loop.
11+
/// - FAIL → sends the review notes back into the original thread as a
12+
/// follow-up prompt so the agent fixes the issues and is then re-reviewed.
13+
/// Bounded by `maxReviewRounds` to stop a fix→fail→fix loop.
14+
/// - No verdict marker (a cancelled/interrupted review, or a reply missing the
15+
/// marker) → records not-passed but does NOT re-prompt, so a manually
16+
/// cancelled review never kicks off an auto-retry turn.
1417
///
1518
/// Runs on `.afterSessionStop` (after the thread is finalized/saved). Registered
1619
/// last so its (possibly long) work doesn't delay the response notification.
@@ -43,6 +46,11 @@ final class CodeReviewHook: Hook {
4346
.filter { $0.action == .codeReview }
4447
guard let hook = hooks.first else { return .ignored }
4548

49+
// Defer while the user still has queued messages — they'll run as further
50+
// turns, so don't review a half-finished change. The next stop (queue
51+
// drained) triggers the review.
52+
if payload.hasQueuedFollowups { return .ignored }
53+
4654
let changedFiles = controller.changedFilePaths(sessionId: payload.sessionId)
4755
guard !changedFiles.isEmpty else {
4856
// Nothing changed — treat as passed so a paired commit hook can no-op
@@ -70,6 +78,17 @@ final class CodeReviewHook: Hook {
7078
"summary": .string("Code review · \(changedFiles.count) changed file(s)"),
7179
]
7280
)
81+
// Persist the card in-progress so it survives a reload while the review
82+
// (which can take minutes) is still running. `finishCard` updates it.
83+
controller.persistHookStatus(
84+
sessionKey: payload.sessionKey,
85+
toolId: card.toolId,
86+
name: hook.name,
87+
trigger: hook.trigger.displayName,
88+
output: "",
89+
isError: false,
90+
isComplete: false
91+
)
7392

7493
logger.debug("[Hook] spawning code-review thread for session \(payload.sessionId, privacy: .public) files=\(changedFiles.count)")
7594
let result = await controller.spawnLinkedThread(
@@ -108,7 +127,7 @@ final class CodeReviewHook: Hook {
108127
controller.setReviewRound(0, sessionId: payload.sessionId)
109128
return .proceed
110129

111-
case .fail(let notes), .unknown(let notes):
130+
case .fail(let notes):
112131
recordVerdict(false, payload: payload, controller: controller)
113132
let round = controller.reviewRound(sessionId: payload.sessionId)
114133
if round + 1 >= Self.maxReviewRounds {
@@ -135,6 +154,21 @@ final class CodeReviewHook: Hook {
135154
"""
136155
)
137156
return .proceed
157+
158+
case .unknown:
159+
// The reviewer ended without a PASS/FAIL marker. The dominant cause
160+
// is a review thread the user manually cancelled (or one that was
161+
// interrupted) — its partial reply has no verdict. Don't auto-retry:
162+
// record not-passed (so a paired commit hook still holds off) and
163+
// finish the card, but leave the agent alone. A genuine "reviewer
164+
// forgot the marker" is rare and is better surfaced quietly here than
165+
// by silently kicking off an unwanted fix turn.
166+
recordVerdict(false, payload: payload, controller: controller)
167+
controller.setReviewRound(0, sessionId: payload.sessionId)
168+
finishCard(card, hook: hook, payload: payload, controller: controller,
169+
result: "⚠️ Code review ended without a verdict (it may have been cancelled or interrupted) — not retrying.\n\(reviewLink)\n\n\(body)",
170+
isError: true)
171+
return .ignored
138172
}
139173
}
140174

@@ -155,7 +189,8 @@ final class CodeReviewHook: Hook {
155189
name: hook.name,
156190
trigger: hook.trigger.displayName,
157191
output: result,
158-
isError: isError
192+
isError: isError,
193+
isComplete: true
159194
)
160195
}
161196

0 commit comments

Comments
 (0)