feat(ai,pr-review): converge AI reviews in fewer rounds
Brought to you by:
thebguy
Menu
▾
▴
Review prompt (
Re-review behavior (
#121 feat(ai,pr-review): converge AI reviews in fewer rounds
Originally created by: theBGuy
Originally owned by: theBGuy
Tunes the AI code-review prompts so a review lands more of what it knows in the first pass and re-reviews close out instead of staying open. Previously a suggested fix could omit its knock-on work (making one round's fix the next round's finding), confident nits trickled out across rounds, and dropping GitDesktop's own prior comments under budget pressure read to the model as "nothing on record".
Review prompt (src/lib/ai/prompt.ts)
- Suggested fixes must now be stated completely, including knock-on obligations the fix creates (imports to adjust, doc comments to move or reword with their symbol, sibling call sites, cache keys, docs to sync).
- Rewrites the signal-over-volume rule to "nothing confident held back": every finding the reviewer is confident about goes in the current review, nits included (one line each, ordered last), on the reasoning that a nit not worth a line now isn't worth a round later. Speculation is still what gets filtered out.
- Repeated patterns are reported as ONE finding naming every affected file and line visible in the diff, with an explicit caveat when the diff is truncated — replacing the older "don't restate the same nit across files" instruction that left siblings unmentioned.
- Widens the deliberate-decision check from convention/docs-sync/scope/hardening omissions to any finding: acknowledged decisions get a one-line acknowledgement, acknowledged nits need no further mention in later rounds, blocker/should-fix items keep a one-clause "still a recorded decision" note, and a note contradicted by visible code is still flagged in full.
Re-review behavior (ITERATIVE_REVIEW_CLAUSE)
- Verified fixes are now reviewed as first-class new code in the same round — their own hunks, the collateral they mint, and how multiple applied fixes interact.
- Adds a
### Leftover polish (non-blocking)list for nits noticed late on code the "Changes since that review" section shows unchanged: captured the round they're noticed, explicitly not grounds for another round, and severity may not be inflated to escape the list. - Defines carry-over rules for leftovers — re-listed under the same heading if still applicable, moved to
### Resolved since last reviewif a later push fixed them, never promoted to a normal finding and never dropped silently — and disables the routing entirely when the branch was rewritten or the delta is unavailable/omitted. - When nothing substantive remains, the review gives the resolved list and wraps up in a line or two.
Prompt assembly for dropped own-comments
- In
buildReviewPrompt, when GitDesktop has prior comments but they were dropped for budget (extras.ownDropped), the prompt now carries an explicit "omitted" marker section telling the model that recorded dispositions and refutations may exist on the PR thread, instead of dropping the section silently.renderedOwnis deliberately left unset since the marker carries no actual comment content.
Docs
src/features/help/content.ts: the review-context section now explains that when the budget leaves no room at all, the review is told the comments were omitted rather than losing them silently.- Adds
changelog.d/changed-review-prompt-convergence.mddescribing the convergence behavior and the non-blocking leftover list.
Related
Originally posted by: cloudflare-workers-and-pages[bot]
Deploying gitdesktop with
Cloudflare Pages
161047fView logs
Originally posted by: theBGuy
Reviewer context — deliberate calls on the record, numbered for reference (evidence inline):
ITERATIVE_REVIEW_CLAUSEappends in BOTH modes (buildReviewPrompt, prompt.ts ~:718), and a second security audit must neither suppress a reportable Low (the "clears this review's own reporting bar" override exists for exactly that) nor assert merge-readiness from a security-scoped pass. Don't suggest re-introducing blocker/should-fix vocabulary or a "ready" verdict there.head-unchangeddeltaState stays bound by the leftover routing — deliberate. Only rewritten / delta-unavailable / delta-omitted release it. A base-advance can surface genuinely new issues, and those pass through the always-reportable override; releasing the routing on unchanged heads would rebuild the one-nit-per-round treadmill this PR exists to kill.truncate.tsfit()keeps the head). Don't suggest a hard cap; the decay is the documented behavior.ownDroppedmarker deliberately does NOT appendOWN_COMMENTS_CLAUSE(renderedOwnstays unset): the clause describes comment content the marker doesn't carry; the marker is self-contained.ownDroppedfires only whencap <= 0(fitOwn, truncate.ts) — which is why the help copy says "the budget leaves no room for it at all" rather than "can't fit" (ledger overflow with budget remaining head-slices and renders[distilled summary truncated]instead).reviewSystemFornoun-swaps"pull request"(10 occurrences) and"GitHub-flavored Markdown"(7) for non-GitHub providers viareplaceAll. Every new sentence deliberately avoids both phrases ("hold this change open", "review round"). Any suggested rewording must preserve those counts.{{ai}}region, so Hide AI strips it) plus the changelog fragment.biome checkformat-red on that file is the known artifact. CI (biome cion fresh LF checkouts) is unaffected.tsc -b --noEmit, scopedbiome checkon the touched files, fullpnpm build,pnpm changelog:check— all green on the final head. The only executable change in the PR is theelse if (hasOwn && extras.ownDropped)branch; everything else is string constants.Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedThe change edits three prompt surfaces — a stronger "report everything you're confident about" rule and knock-on-obligations requirement in the general review system prompt, a convergence/leftover-routing expansion of
ITERATIVE_REVIEW_CLAUSE, and a new "own comments omitted" marker inbuildReviewPrompt— plus help and changelog copy. The code change is sound (ownDroppedis true exactly whencap <= 0infitOwn, so the newelse ifis reachable, mutually exclusive with the render branch, and correctly withholdsrenderedOwnsoOWN_COMMENTS_CLAUSEnever describes content the marker doesn't carry); the issues are internal contradictions in the new prompt text.Correctness (prompt semantics)
should-fix —
src/lib/ai/prompt.ts,ITERATIVE_REVIEW_CLAUSE(:431), final sentence. The wrap-up names only one list: "If nothing remains beyond optional polish — leftover or otherwise — still give the### Resolved since last reviewlist, then say there is nothing further to raise in a line or two and stop." But the condition "nothing remains beyond optional polish" is exactly the state in which leftovers do exist, and an earlier sentence in the same clause commands "if it still applies, re-list it under the same heading … never drop it silently." Concrete case: round 3 where the only outstanding items are two leftovers carried in from round 2 and one resolved item — the model hits the terminal sentence, emits### Resolved since last reviewplus "nothing further to raise", and stops, silently dropping the leftovers the carry-forward rule requires. Secondary defect in the same sentence: "still give the### Resolved since last reviewlist" is unconditional, so a round where every prior finding was an invalid one dropped silently yields an empty heading. Fix: replace the sentence with "If nothing remains beyond optional polish — leftover or otherwise — still give the### Resolved since last reviewand### Leftover polish (non-blocking)lists, omitting either heading when it has no items, then say there is nothing further to raise in a line or two and stop." No other edit is needed — both heading strings already appear verbatim earlier in the clause, so they stay in lockstep, and the clause's doc comment (:425–428) describes framing only and needs no change.should-fix —
src/lib/ai/prompt.ts,GENERAL_REVIEW_SYSTEM(:381), the new repeated-pattern rule's tail "and if the diff is truncated say the list covers only what is shown". This fires unconditionally on truncation, including agentic runs, where the rest of the prompt forbids exactly that hedge:agenticReviewClause(:478) says "Never hedge with 'could not verify' about code you could have opened", and the truncation note built at :699–:708 appends "Coverage is your responsibility — do not report partial coverage without first closing this gap." Concrete case: a CLI repo-aware review (cliRepoAware,filesOnDisktrue via :572) of a PR whose diff exceedsdiffCharBudget—budgeted.truncatedis true, so the model gets both "say the list covers only what is shown" and "do not report partial coverage without first closing this gap", and the new instruction pushes it back toward the coverage hedging that branch exists to eliminate. Fix: qualify the clause to the non-closable case — "and if the diff is truncated and you cannot pull or open the omitted parts, say the list covers only what is shown". That phrasing stays true for the non-agentic branch at :707 ("Review what is shown and note that coverage is partial") and defers to the agentic branch at :705, so no change is needed at either call site.should-fix —
src/lib/ai/prompt.ts,ITERATIVE_REVIEW_CLAUSE(:431) as appended at :718. The new text is written entirely in general-review vocabulary ("a nit you only now notice", "### Leftover polish (non-blocking)", "optional polish"), but:718appends the clause for both modes, so it lands onSECURITY_REVIEW_SYSTEM, whose output contract is the opposite: "ignore pre-existing issues outside the diff, style, and anything that isn't a concrete, exploitable security risk" (:386) and "Do not pad … just findings or a single 'no issues' line. Silence is better than noise." (:423). Concrete case: a second security audit on a PR that already has a stored security review —resolvePriorContextis called withmode(reviews.ts:500–506) andgetLatestReview(..., mode)returns it, sohasPrioris true and the audit is now instructed to open a polish list that its own contract bans. (Your note [#2] records that the clause appends in both modes and that the terminal wording and the reportable-Low override are deliberate — that part I'm not re-raising; the polish-list emission isn't covered by it.) Fix: split the clause — keep the re-verification, fix-hunk-review, and the terminal wrap-up inITERATIVE_REVIEW_CLAUSE, move the two sentences from "Bias a re-review toward convergence:" through "…this routing does not apply.)" into a newconst LEFTOVER_ROUTING_CLAUSEwith its own doc comment naming it as general-mode-only, and at :718 append it conditionally:if (hasPrior) { system += ITERATIVE_REVIEW_CLAUSE; if (mode !== "security") system += LEFTOVER_ROUTING_CLAUSE; }. The terminal sentence then must not reference the leftover heading in security mode — so pair this with the fix above by wording it "…give the### Resolved since last reviewlist (and the### Leftover polish (non-blocking)list when that routing applies), omitting either heading when it has no items…".Docs
src/features/help/content.ts, AI review section (:899–:903). The guide was updated for the omission marker but not for the more user-visible half of this PR: the changelog fragment tells users that "Re-reviews collect late-noticed polish in an explicitly non-blocking leftover list … and wrap up in a line once nothing substantive is left", and that new### Leftover polish (non-blocking)heading plus the "nothing further to raise" ending is what users will actually read in review output — with no explanation anywhere in-app that leftovers aren't blocking. Per the repo rule, a meaningfully altered user-facing surface gets its guide section updated in the same change. Fix: add one sentence after the omission sentence at :903, e.g. "On a re-review, polish noticed late on code that hasn't changed is listed separately as non-blocking leftovers — batch it with your next push or defer it — and once nothing substantive is left the review says so in a line instead of holding the round open." It sits between the{{ai}}at :886 and the{{/ai}}at :945, so it inherits the AI gate with no extra markers, and it uses no shortcut references so no{{kbd:…}}tokens are required.Nits
src/lib/ai/prompt.ts:431: the escape list says "the delta is unavailable", but theindeterminatebody rendered at :517 reads "The previous commit isn't available locally" — the other two arms ("branch was rewritten" :514, "delta was omitted" :525) match their rendered copy verbatim; align the third the same way.src/features/help/content.ts:902–903: dangling modifier — "the review is told the comments were omitted rather than losing them silently" implies the review is the thing losing them; "…rather than dropping them silently" (or "…rather than them being lost silently") fixes it.Recorded decisions (acknowledged, not re-flagged)
head-unchangedstaying bound by the leftover routing remains your recorded call (note [#3]); the always-reportable override it relies on is visible in the clause text at :431, so nothing contradicts it.Posted by GitDesktop — AI output, verify before acting on it.
Related
Tickets:
#2Tickets:
#3Tickets:
#4Originally posted by: theBGuy
🤖 GitDesktop AI security audit ·
opus· automatedNo genuine security issues in these changes — the diff is prompt-text tuning plus a constant "own comments omitted" marker (no untrusted data interpolated into any new sink, no guard removed from the security-review prompt or the own-comments/external clauses).
Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round-1 dispositions (all verified against the source before any fix; all fixes land in the next push):
LEFTOVER_ROUTING_CLAUSE(general-mode only) whose final sentence amends the wrap-up — "give the### Leftover polish (non-blocking)list alongside the resolved list, omitting the heading when it has no items — carried leftovers are re-listed there by the rule above, never dropped at the wrap-up" — following the file's own appended-clause idiom (OWN_COMMENTS_CLAUSE/EXTERNAL_REVIEW_CLAUSE). The shared terminal is now fully mode-neutral: Resolved list only, omit-when-empty, "nothing further to raise". Your round-3-only-leftovers scenario traces closed: the lifecycle rule re-lists, the amendment guards the wrap-up.mode === "general"(fail-closed if a third mode ever appears —!== "security"would opt it in silently), and the terminal-pairing was reworked so security never receives even a reference to the leftover heading. The rendered security prompt is asserted (node-render, throws on leak) to contain zero occurrences of "leftover" in any case. Four mode×prior assembly combinations enumerated: general+prior → both clauses; security+prior → the mode-neutral iterative clause only; either mode without priors → neither (first-ever reviews unchanged).Pre-empting the one residue: the shared terminal's "beyond optional polish" remains. In security mode it is vacuous — no polish category exists, so the condition reduces to "nothing reportable remains", and a Low@9 clears the explicit reporting bar and blocks the terminal. In general mode it is load-bearing — a round where only leftovers remain must still wrap up. Removing it needs a two-place rewrite for zero behavioral change; deliberate call, on the record.
deltaSectioncopy; all three named arms now match their rendered strings verbatim (head-unchangeddeliberately absent per context item 3).Copilot's checklist-singular finding: fixed, see its thread. Security audit: clean, nothing owed.
Verification on the outgoing batch:
tsc -b --noEmit·biome checkclean on both files · fullpnpm build·changelog:check·"pull request"/"GitHub-flavored Markdown"replaceAll counts pinned at 10/7 · per-constant escaped-heading counts asserted · both assembled mode variants node-rendered with leak assertions. Changelog fragment judged accurate post-split (its leftover claim is general-only by vacuity in security mode, same as its sibling claims — qualifying one clause would read as inconsistency).Meta, for the record: this round was itself the first live run of the prompt under review — it acknowledged three recorded decisions by number, front-loaded four should-fixes and two nits in one pass, and every suggested fix arrived with its knock-on scope stated. The remaining live discriminators (carried-leftover lifecycle on a later round; second-audit assembly) are exactly what the next rounds of this PR exercise.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedThe round-1 fixes all landed as described: the leftover routing is now its own
LEFTOVER_ROUTING_CLAUSEgated tomode === "general"(exact againstReviewMode = "general" | "security"), the shared terminal is mode-neutral with omit-when-empty, and the truncation hedge is qualified. Nothing blocking; one docs nit is collateral of the split.Resolved since last review
ITERATIVE_REVIEW_CLAUSE(:432) now reads "If nothing reportable remains beyond optional polish, still give the### Resolved since last reviewlist, omitting the heading when it has no items…", andLEFTOVER_ROUTING_CLAUSE's closing sentence (:443) amends the wrap-up to emit the leftover list alongside it, omit-when-empty. The round-3-only-leftovers path traces closed: the carry-forward rule re-lists, the amendment guards the wrap-up.GENERAL_REVIEW_SYSTEM:381 now says "if the diff is truncated and you cannot pull or open the omitted parts". Consistent with both truncation-note arms at :711–:720 (close-the-gap whencanReadFiles || canPullDiff || canDiffRefs, partial-coverage otherwise) and withagenticReviewClause's no-hedging line at :490.SECURITY_REVIEW_SYSTEM— split out at :441 and gated at :733; the security prompt's silence-over-noise contract (:423) is no longer contradicted, and the new doc comment (:434–:440) records why.content.ts:903–:906 now covers it, inside the{{ai}}/{{/ai}}span (:886/:948) with no shortcut tokens needed.LEFTOVER_ROUTING_CLAUSE:443 now says "the previous commit isn't available", matchingdeltaSection'sindeterminatebody at :529.Reviewing the fix hunks themselves: the clause split, its doc comment cross-reference ("A GENERAL re-review also gets LEFTOVER_ROUTING_CLAUSE below", :428–:429), and the assembly order (
ITERATIVE→LEFTOVER→OWN→EXTERNAL→ agentic) are all consistent — the amendment sentence lands after the terminal it amends, so the general-mode wrap-up reads in the right order. The two applied fixes don't collide: the truncation qualifier is base-prompt-only and the split is re-review-only.Docs
changelog.d/changed-review-prompt-convergence.md:7andsrc/features/help/content.ts:903–906: the split made the two doc surfaces scope the same pair of behaviors in opposite, both-wrong directions. The changelog says "Re-reviews collect late-noticed polish in an explicitly non-blocking leftover list … and wrap up in a line once nothing substantive is left" — but the leftover list is now general-only (:733), so that claim over-scopes; the help scopes the whole sentence with "On a general re-review", but the wrap-up terminal lives inITERATIVE_REVIEW_CLAUSE(:432), which still appends in both modes, so it under-scopes. Fix both in this change: changelog → "General re-reviews collect late-noticed polish in an explicitly non-blocking leftover list instead of letting it hold rounds open, and every re-review — general or security audit — wraps up in a line once nothing substantive is left."; help → end the leftover clause at "…batch it with your next push or defer it." and start a new sentence "Any re-review — review or security audit — wraps up in a line once nothing substantive is left, instead of holding the round open." ("general" matches the guide's existing vocabulary at :891, so no other wording needs syncing, and both edits stay inside the{{ai}}gate.)Copilot's singular file/line complaint on
:379is addressed in the current diff — :377 now reads "…or, for a repeated-pattern finding, every affected file and line."Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round-2 disposition (single item):
{{ai}}gate;changelog:checkandbiome checkgreen.Worth noting for the record: this nit is collateral of the round-1 split — exactly the fix-minted class the new re-review rule exists to catch — and it was caught in the immediately following round instead of three rounds later. The push carrying this fix is doc-only on these two surfaces; nothing else changes.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedResolved since last review
changelog.d/changed-review-prompt-convergence.md:7–9now leads "General re-reviews collect late-noticed polish…" and closes "…every re-review — general or security audit — wraps up in a line once nothing substantive is left", matching the general-only gate atprompt.ts:733and the mode-neutral terminal inITERATIVE_REVIEW_CLAUSE(prompt.ts:432, appended in both modes at :731).src/features/help/content.ts:903–907ends the leftover clause at "…batch it with your next push or defer it." and states the wrap-up in its own mode-neutral sentence, still inside the{{ai}}/{{/ai}}span (:886/:949) and with no shortcut tokens needed.Reviewing that fix's own hunks: the help edit only re-splits sentences inside one paragraph —
markedis instantiated withoutbreaks(src/components/ui/markdown.tsx:48,new Marked({ gfm: true })), so the new source-line break before "See AI & automations" keeps rendering as one paragraph, no stray<br>. Both claims still check out against the code: the omission marker exists atprompt.ts:663–669(fires only whenhasOwn && extras.ownDropped, andownDroppedis set only when the own cap hits zero —truncate.ts:224–226— so a partially-fitting block still renders the real section), and the general-only leftover routing matchesmode === "general"exactly againstReviewMode = "general" | "security".Docs
changelog.d/changed-review-prompt-convergence.md:9: the re-wording left this continuation line at ~155 chars while every other line in the fragment wraps at ~88. Nothing catches it: Biome has no markdown formatter configured (biome.jsonhas nomarkdownsection),changelog:checkvalidates only the 2-space continuation indent (scripts/changelog-lib.mjs:112–117), and the assembler concatenates fragment bodies "without re-wrapping, so what you write is exactly what ships" (changelog.d/README.md:24) — so the long line lands in releasedCHANGELOG.md. Re-wrap lines 8–11 keeping the 2-space indent, e.g.leftover list instead of letting it hold rounds open, and every re-review — general/or security audit — wraps up in a line once nothing substantive is left. Decisions/you recorded in the PR description or *Notes for reviewers* are respected for every/kind of finding, and when a large diff crowds out GitDesktop's own earlier comments/the review is told they were omitted rather than quietly losing the record.— text unchanged, sochangelog:checkstays green and no other surface needs syncing.Nothing further to raise: the prompt.ts changes are internally consistent, the delta-state strings the leftover clause names match
deltaSectionverbatim (:526/:529/:537), and every non-okprior-context path sets adeltaState(prior-context.ts:41–79), so the routing's escape hatches are reachable as written.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round-3 disposition (single item):
changelog:checkgreen and an explicit length sweep shows no line over 95 chars in the fragment. For the record: this was collateral of the round-2 doc fix (the edit ended mid-line and the remainder stayed attached) — the fix-minted class again, caught in the immediately following round.The push carrying this is a whitespace-only rewrap of one fragment; no other file changes.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedThe only change since my last review is the changelog fragment rewrap; it's word-identical to the previous text and the whole PR (prompt clauses, general-only leftover routing, omitted-own-comments marker, help + changelog sync) still checks out.
Resolved since last review
changelog.d/changed-review-prompt-convergence.md:8–12: the ~155-char continuation line is gone — lines 8–12 now wrap at ≤~88 chars, matching the rest of the fragment, with the wording unchanged. Reviewing the fix's own hunk: every continuation line still starts with the 2-space indent thatvalidateFragmentsrequires (scripts/changelog-lib.mjs:111–122accepts only"","- ", or" "after line 1), the body still starts with-(:102), the second top-level-bullet is explicitly permitted by that same loop, and both bullets ship as one adjacent item sincerenderGroupBlockssplits each fragment body as a unit (:137) — sochangelog:checkstays green and nothing else needs syncing.Copilot's
prompt.ts:379flag (checklist says "the symbol/line" singular, contradicting the repeated-pattern rule) is addressed in the current diff — line 377 now reads "…— or, for a repeated-pattern finding, every affected file and line", so it agrees with the consolidation rule at line 381.Nothing further to raise: the omission marker's branch is exactly the "dropped entirely" case (
fitOwnsetsdroppedonly whencap <= 0,truncate.ts:225–226, and otherwise always returns text), the leftover clause's escape strings matchdeltaSectionverbatim (prompt.ts:526/529/537), and the general-only gate at:733compares againstReviewMode = "general" | "security"as intended.Posted by GitDesktop — AI output, verify before acting on it.
Ticket changed by: theBGuy