fix(pr-review): never publish a clean verdict when the reviewer drops its findings - #541
Conversation
![mogplex[bot]](https://avatars.githubusercontent.com/in/3107927?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Approve-ready: the dropped-findings fix works end-to-end — readReviewReportState refuses to trust a clearing report after an issues claim, the SDK's invalid flag survives into the normalized steps, the one-shot repair covers both the missing and dropped shapes, publication is a neutral "Review incomplete" check that blocks auto-merge, and tests cover the state rules, both repair kinds, the in-run refusal, and the published comment. One caveat: droppedFindingsRefusal in lib/agents/pr-reviewer.ts only fires after an accepted report, so the all-rejected incident shape can still merge in-run under an auto-merge flow before the post-run gate runs.
Warnings
-
In-run merge refusal doesn't cover the all-rejected report shape (
lib/agents/pr-reviewer.ts:L104)
In-run merge refusal doesn't cover the all-rejected report shape.droppedFindingsRefusal()(around L102-111) only refuses a merge whenclaimedIssues && acceptedReport !== null && clearsReviewWithoutFindings(acceptedReport).In the all-rejected shape this PR also fixes — a run where every
reportReviewclaim fails schema validation (#1647-style) —acceptedReportstaysnull, so a reviewer that then callsmergePullRequestorqueuePullRequestForMergemerges in-run, beforefinalizeFlowSuccesscan rungetPrReviewAutoMergeBlockReason(lib/workflows/automation-job-flow-success.ts). The check would then publish "Review incomplete" on an already-merged PR — the same hazard the refusal exists for, on a path where the post-run gate cannot intervene.Suggested tightening: refuse whenever issues were claimed and no clear-with-findings report was accepted, i.e.
claimedIssues && (acceptedReport === null || clearsReviewWithoutFindings(acceptedReport)), which matches whatreadReviewReportStatealready does with the raw tool-call inputs. A test for the all-rejected-then-merge sequence would pin it down.
Suggestions
-
claimedIssues tracker misses malformed reportReview inputs (
lib/agents/pr-reviewer.ts:L91)
claimedIssuestracker misses malformedreportReviewinputs.claimedIssuesis set inside.superRefine(around L88-99), which Zod 3 skips when the base object itself fails to parse — a claim like{ hasIssues: true }withsummarymissing is rejected without the refinement ever running, soclaimedIssuesstays false.The pipeline-side reader does not share this blind spot because it reads the raw tool-call input (
call.input.hasIssues === trueinlib/workflows/pr-review-report-state.ts), so only the in-run merge mirror is weakened: a malformed issue claim followed by an accepted clean report would let an in-run merge proceed while the published verdict isdropped_findings.A short comment documenting the asymmetry would help future readers; the runner is the only place that could observe rejected raw inputs directly.
-
Doc comment narrows what the implementation checks (
lib/workflows/pr-review-report-state.ts:L36)
Doc comment narrows what the implementation checks. The doc onreadReviewReportState(L24-33) says the report clears "after an earlier call said there were issues", but the implementation checkscalls.some((call) => call.input.hasIssues === true)— any issues claim in the run, including a rejected call placed after the accepted clearing report, yieldsdropped_findings.That conservative reading looks right and is pinned by the tests, but someone debugging a review that was republished as incomplete will search for an "earlier" claim and not find one. Consider rewording to "any call in the run, accepted or rejected, before or after the report".
-
No test for a repair whose own reportReview call fails validation (
lib/workflows/pr-review-report-repair.ts:L130)
No test for a repair whose ownreportReviewcall fails validation. The incident reviewer fumbledreportReviewfive times in a row, so a repair call that also comes back schema-invalid is realistic.When that happens,
readReviewReportStateskips the invalid repair call and falls back to the original clearing report, leaving the reviewdropped_findingsand published as incomplete, with no second repair — correct and loop-free, but untested.A case shaped like
runReview([DROPPED_FINDINGS, INVALID_REPAIR])would pin that branch.
|
Addressed the review in 'fix(pr-review): refuse in-run merges when every issue report was rejected':
|
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Well-engineered fix with strong tests: verdict trust now derives from raw reportReview calls via readReviewReportState, with repair routing, neutral "Review incomplete" publication, and auto-merge blocking all wired correctly. Two warning-level gaps need discussion: cross-node claim matching in multi-review-node flows can void a legitimate post-fix clean re-review and block auto-merge, and the in-run merge guard misses claims rejected by base-schema validation (e.g., invalid finding severity) because claimedIssues is only set inside Zod's superRefine.
3 findings were added inline.
| * accepted or rejected, before or after it, said there were issues has lost | ||
| * them: reviewers told that hasIssues=true needs findings sometimes flip | ||
| * hasIssues to false instead of listing them, and that report would | ||
| * otherwise publish as a clean verdict with its warnings gone. A report that |
There was a problem hiding this comment.
Warning: Dropped-findings matching spans multiple flow nodes, voiding legitimate re-review clean verdicts
readReviewReportState (lib/workflows/pr-review-report-state.ts:L27-L36) computes claimedIssues and the last accepted report across ALL merged steps. In flows, the merged result spans every agent node, and the flow finalization in automation-job-flow-success.ts re-extracts the verdict from it. A flow with two review-role nodes — e.g., review -> fix -> re-review — yields: node A files hasIssues:true with findings, the fix node repairs the PR, then node B re-reviews the fixed PR and legitimately files a clean report. The merged evaluation sees A's claim plus B's clearing report, classifies dropped_findings, and treats the verdict as missing, so the flow's auto-merge is blocked ("Mogplex review finished without a structured verdict") and the flow-level review publication is skipped. No repair runs in this case (fileMissingReviewReport runs per node, never on the merged result). Pre-PR behavior (last report wins) allowed this clean merge.
The heuristic "a clearing report after a claim lost the findings, since nothing says the findings were wrong" is right within one reviewer's session, but between nodes something did say the findings were wrong: the intervening fix.
Suggestion: scope claim-vs-clear matching to a single review node's own steps. executeFlowAgentNode already computes a per-node extractPrReviewHarnessResult(result).reviewOutcome; gating flow auto-merge and flow-level publication on the last review node's verdict (plus per-node verdictMissing) rather than re-reading the whole merged transcript avoids the false block. If the current fail-safe behavior across multiple review nodes is intended, a note in readReviewReportState's doc comment would make that explicit.
| @@ -58,6 +59,8 @@ export function buildPRReviewTools(config: { | |||
| const request = config.fetch ?? fetch; | |||
| const contentOwner = config.headOwner ?? config.owner; | |||
| const contentRepo = config.headRepo ?? config.repo; | |||
There was a problem hiding this comment.
Warning: In-run merge guard blind spot: claims rejected by base schema never set claimedIssues
claimedIssues is set inside reportReviewInputSchema's superRefine (lib/agents/pr-reviewer.ts:L61-L79), which Zod only runs when the base object parses successfully. A reportReview call that fails base validation — missing summary, an invalid severity string like "blocker" in findings, line: 0, or more than 20 findings — never sets claimedIssues, so droppedFindingsRefusal() returns null and mergePullRequest/queuePullRequestForMerge proceed even though the reviewer claimed issues in this run (e.g., malformed-issues report, then clean accepted report, then merge). Post-run publication still detects dropped_findings via readReviewReportState, but the merge will already have executed before the post-run gate can block it — the in-run guard is the only control that acts before the merge.
The in-code comment acknowledges the blind spot ("Zod skips it for input missing a required field... the published verdict does not depend on this"), but that reassurance covers publication only; the merge refusal does depend on it. Observed retry behavior usually re-triggers superRefine and sets the flag, so this needs a single malformed claim followed by a flip to clean to slip through.
Suggestion: make the claim observable before strict validation rejects it — e.g., move required-field and finding-shape enforcement into superRefine itself (with the base object typed permissively so superRefine can set claimedIssues before adding its rejection issues), or record the flag from raw step data outside the schema. A regression test with a claim containing an invalid severity would pin the guarantee either way.
| * The reviewer's report as the pipeline should trust it. Only calls the SDK | ||
| * accepted count as filed; a call whose input failed the schema was never | ||
| * recorded. A report that clears the review when any call in the run, | ||
| * accepted or rejected, before or after it, said there were issues has lost |
There was a problem hiding this comment.
Suggestion: Claim-after-filed-report ordering: documented rule and behavior diverge for reports that list findings
The doc comment on readReviewReportState (lib/workflows/pr-review-report-state.ts:L24-L36) says a clearing report is dropped "when any call in the run, accepted or rejected, before or after it, said there were issues". In behavior, an accepted report that lists findings (e.g., hasIssues:false with suggestions) followed by a rejected hasIssues:true claim is treated as filed and published clean, while the identical rejected claim after a no-findings report yields dropped_findings. If a late claim deserves the repair ask in the lists-nothing case, consider whether it deserves it here too — or note the asymmetry in the comment so future readers don't puzzle over it.',
…the merged transcript
|
Addressed the second review:
|
Mogplex PR ReviewStatus: No material issues found Approve-ready: the fix is correct and layered across claim detection, per-session verdict state, a one-shot repair, a per-node flow verdict, and in-run merge refusals; the AI SDK behaviors it depends on (invalid tool calls keep parsed input with Affected files
Suggestions
|
|
Answers to the three suggestions in the last review (no warnings, so merging as is):
|
1 participant
The review on #535 said "one non-blocking warning and three minor suggestions, detailed in the comment", passed as "No material issues found", and listed nothing. The findings were never recorded anywhere, including the check run's output.
The stored ai_call shows why. The reviewer (zai/glm-5.3) called
reportReviewsix times. The first five sethasIssues: truewithout afindingsarray, and the schema rejected each one. The sixth sethasIssues: false, still without findings; that passed validation and was published as a clean verdict. The same sequence appears in about ten reviews over the last day, including #530 and #536 and several webrenew repos. A second variant, where every call was rejected, left the rejected input as the report: webrenew/vmotif#1647 was published as "Attention needed" with nothing listed, and the missing-report repair never ran because a rejected call counted as filed.Changes:
invalidflag on tool calls, so the pipeline can tell an accepted report from a rejected one.readReviewReportState(new,lib/workflows/pr-review-report-state.ts) is the one reader of the report. It uses the last accepted call. If an earlier call claimed issues and the accepted one clears the review with no findings, it returnsdropped_findings.dropped_findingsreview gets one follow-up asking for the findings as entries infindings, using the same path as the missing-report repair. If the answer still lists nothing, the review is published as "Review incomplete" with a note saying why, gets a neutral check, and blocks auto-merge. A review whose every report was rejected now goes to the missing-report repair.mergePullRequestandqueuePullRequestForMergerefuse to act on a dropped-findings report. They run before the post-run gate, so a flow with auto-merge could otherwise merge on the switched verdict.hasIssuesto false leaves the review without a verdict.One judgment call: a review that files findings and later files a clean report listing none is also treated as dropped. The reviewer keeps a way to report a clean review with suggestions:
hasIssues: falsewith suggestion findings is accepted as before.Tests cover the report-state rules, what a dropped-findings review publishes, the repair for both the all-rejected and dropped cases, and the merge-tool refusal. Each new behavior was mutation-checked: breaking it fails at least one test.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.