render pr-reviewer stage 3 output as structured markdown instead of one prose blob - #5971
Merged
Conversation
…ne prose blob Stage 3 returned a single `summary` string that the coordinator posted verbatim as the review body, so a thorough review arrived as one unbroken paragraph mixing the verdict, the test evidence, every verified claim, and the blocking problem — unreadable on the PR page (see #5929). The stage now returns the review as separate fields (summary, scope, testEvidence, verified, concerns) and findings gain a title and an optional one-line suggestion. `server/lib/prReviewReport.js` renders those into the markdown a human reads: verdict banner, scope line, a blocking/non-blocking index anchored to path:line, test-evidence bullets with pass/fail/not-run icons, notes, and a collapsed verified-claims list. Inline comments get a blocking label, their title, and a GitHub ```suggestion block when the fix is one line. Rendering stays on the deterministic side of the model boundary: the model emits plain prose only, single-line fields collapse whitespace so a field cannot inject headings, a fenced suggestion is dropped rather than allowed to break out of the code block, and `reviewReportText` feeds every new field to the existing model-abuse scan. The body budget drops whole low-priority sections rather than truncating mid-sentence. A plain-string `summary` still renders, so an older stage body degrades to the previous single paragraph.
…drift Two producers ask for this envelope — the pr-reviewer stage-3 body and the issue-watcher reasoning pass — and both feed one normalizer. Each had hand-written its own copy of the field spec, so adding a field to prReviewReport.js and only one prompt would have silently degraded the other's reviews with nothing failing. Both now interpolate the exported PR_REVIEW_DECISION_CONTRACT, following the ISSUE_QUALITY_GUIDANCE precedent, and a test asserts the contract names every field the normalizer and the finding renderer read. Also from the /simplify pass: - renderFindingBody + normalizeFindingPresentation collapse into one renderFinding() returning the comment body and the index label, so a finding is normalized once instead of twice and callers carry a string, not a presentation object. - reviewReportText walks the findings itself, so the abuse scan no longer re-enumerates title/body/suggestion at the call site where a new field would have gone unscanned. - The approve-path default summary joins the other verdicts in VERDICT_DEFAULT_SUMMARY instead of being spread in by the coordinator. - renderReviewBody takes a downgraded boolean rather than a free-form appendix, keeping that sentence with the rest of the rendering. - The section list goes through one listSection helper so it reads as an ordered priority list; the verified block renders with its closing tag attached so the budget can never drop a </details> on its own.
…e fence A finding body carrying an odd number of ``` fences leaves one open, and the suggestion block appended after it is then swallowed as fence content — GitHub renders it as inert text instead of a change the author can apply. Drop the suggestion in that case rather than emit a dead one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stage 3 of the pr-reviewer pipeline returned one prose
summarystring that the coordinator posted verbatim as the GitHub review body — so a thorough review landed as a single unbroken paragraph mixing the verdict, the test evidence, every verified claim, and the blocking problem. Unreadable on the PR page (PR #5929 is the example that prompted this).summary,scope,testEvidence,verified,concerns, and findings carrying atitleand an optional one-linesuggestion.server/lib/prReviewReport.jsrenders it — verdict banner, scope line, a blocking/non-blocking index anchored topath:line, test-evidence bullets with pass/fail/not-run icons, notes, and a collapsed verified-claims block. Inline comments get a blocking label, their title, and a GitHub ```suggestion block when the fix is one line.PR_REVIEW_DECISION_CONTRACTboth prompts interpolate — following theISSUE_QUALITY_GUIDANCEprecedent — rather than two hand-written copies that drift.Rendering stays on the deterministic side of the model boundary: single-line fields collapse whitespace so a field cannot inject headings, a suggestion containing a fence (or one whose body leaves a fence open) is dropped rather than allowed to break out of the code block,
reviewReportTextfeeds every model-authored string to the existing abuse scan, and the body budget drops whole low-priority sections instead of truncating mid-sentence. A plain-stringsummarystill renders, so an older stage body degrades to the previous single paragraph rather than losing its review.No
PROMPT_VERSIONSbump:getStagePromptresolves stage bodies live from the catalog and never persists them, so the integrity-snapshot hash is the complete obligation. Onlypr-reviewer-reviewmoved; no historical body changed.Test plan
cd server && npm test— 37,955 passed, 24 skipped, 1,884 files.server/lib/prReviewReport.test.js(17 cases) pins the rendered sections, the legacy plain-string degrade, single-line injection collapse, both fence break-out guards, the budget dropping a whole section, abuse-scan coverage of every field, and that the contract names every field the normalizer and finding renderer read.server/services/issueWatcher.test.jsupdated for the labeled inline-comment bodies.