feat(review): enrich Gerrit review agent context#76
Open
fshehadeh-sfl wants to merge 13 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the Gerrit-backed review flow by enriching the prompt context sent to the review agent (commit message body + inter-patchset delta on re-reviews) and reducing re-review noise by decoupling verdict re-posting from discussion-thread replies.
Changes:
- Add an optional “## Commit message” section to review prompts when a non-empty description/body exists.
- Add an optional “## Changes since last reviewed patchset (PS X → Y)” section for re-reviews, wired through a new optional
ReviewProvider.getInterPatchsetDiff()(implemented for Gerrit SSH via shallow-fetch +git diff). - Adjust re-review “silent verdict” gating so delivering discussion replies no longer forces reposting the summary/vote.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/reviewPromptBuilder.test.ts | Adds unit tests covering commit-message rendering and since-last-review delta prompt sections. |
| tests/unit/reviewOrchestrator.test.ts | Adds orchestrator tests for delta-fetch wiring and for decoupling verdict posting from reply delivery. |
| tests/unit/gerritSshReviewProvider.test.ts | Adds tests for Gerrit SSH inter-patchset diff fetching and cleanup behavior. |
| src/review/reviewPromptBuilder.ts | Extends the prompt builder to optionally render commit message body and inter-patchset delta sections. |
| src/review/reviewOrchestrator.ts | Wires optional inter-patchset delta fetching into prompt construction; adjusts verdict skip gating to ignore pending replies. |
| src/interfaces.ts | Extends ReviewProvider with optional getInterPatchsetDiff(changeId, fromPatchset, toPatchset). |
| src/connectors/gerritSshReviewProvider.ts | Implements Gerrit SSH getInterPatchsetDiff() via shallow fetching both patchset refs and diffing their tips. |
Charles-Sevan
force-pushed
the
feat/review-context-enrichment
branch
from
July 3, 2026 09:33
2daa09f to
0244437
Compare
Charles-Sevan
force-pushed
the
feat/review-context-enrichment
branch
from
July 6, 2026 08:14
4dd983c to
e97b124
Compare
Contributor
|
@fshehadeh-sfl I added some tests/fix. |
cwenger-sfl
force-pushed
the
feat/review-context-enrichment
branch
from
July 13, 2026 08:49
86b6dea to
742f039
Compare
Collaborator
|
LGTM. I will rebase it. |
Render the change's commit-message body ("the why") as a dedicated
`## Commit message` section in the review prompt so the agent reasons
about intent, not just the diff. Omitted when the description is empty
or whitespace-only to avoid injecting a blank section.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The silent-re-review gate previously required `repliesToPost.length === 0`, so delivering a discussion reply on an otherwise-unchanged re-review forced the summary + vote to be re-posted, spamming the change. Drop that clause so the verdict is skipped purely on whether anything new was found and the vote is unchanged, while pending replies are always delivered through their own independent path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On a re-review, surface the diff between the patchset VE last reviewed and the current one as a focused "Changes since last reviewed patchset" section so the agent concentrates new findings on the delta while still seeing the full change. - Add optional ReviewProvider.getInterPatchsetDiff(changeId, from, to). - Implement it for Gerrit (SSH): shallow-fetch both patchset refs, diff tips. - Render the delta section in the prompt (capped per-section, rebase caveat); omitted on first reviews and when the delta is empty. - Wire the orchestrator to fetch the delta on re-reviews, degrading gracefully when the provider lacks the method or the fetch fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A global Gerrit commit-msg hook on the host was injecting Change-Ids into every commit, causing assertion failures (changeId unexpectedly non-empty) and timeouts (hook made slow network calls per commit). Set core.hooksPath to an empty directory in initRepo() to isolate test repos from any host-level git hooks.
When both a sinceLastReview delta and a full diff are present, renderSinceLastReview and renderDiffSections each independently consume maxDiffChars, allowing the prompt to reach 2×maxDiffChars. Test: given two ~600-char patches with maxDiffChars=1000, at least one section must be truncated after the shared-budget fix is applied.
When sinceLastReview is present, both renderSinceLastReview and renderDiffSections were independently allowed to consume the full maxDiffChars budget, letting the prompt grow to 2×maxDiffChars. Fix: detect whether a delta will be rendered and split the budget equally (floor(max/2) for delta, remainder for full diff). When no delta is present the full budget is unchanged.
mkdtemp(join(baseDir, ...)) throws ENOENT when baseDir does not yet
exist. Both getDiff and getInterPatchsetDiff assumed /tmp/ve-review-diffs
(or workspaceBaseDir) was already present, which could silently break
delta computation on host/dev runs.
Add mkdir(baseDir, { recursive: true }) before mkdtemp in both methods.
Update the node:fs/promises mock in the test suite to include mkdir.
The review prompt already limits rendered diffs to avoid blowing up the model context, but the Gerrit commit message was inserted without any size guard. A very large commit body could therefore consume most of the prompt before the diff and review instructions are added. Truncate oversized commit messages and add a clear truncation marker, so the review agent keeps enough context for the actual code changes.
ReviewOrchestrator already fetches Gerrit change details before requesting the inter-patchset delta. The Gerrit SSH provider was fetching the same details again only to recover the change number, project and change id. Pass the existing ReviewChangeDetails into getInterPatchsetDiff instead. This removes the redundant Gerrit SSH query and keeps the delta path using the same change metadata as the rest of the review run.
cwenger-sfl
force-pushed
the
feat/review-context-enrichment
branch
from
July 13, 2026 08:54
742f039 to
f2f82a6
Compare
Collaborator
|
@fshehadeh-sfl The PR is ready to be merged if you agree. |
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
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
Enriches the context handed to the Gerrit review agent, fixes a prompt-size bug introduced by the delta feature, and removes a source of re-review noise. Four functional changes plus their test coverage:
feat(prompts): inject commit message into review prompt— adds a## Commit messagesection to the review prompt carrying the change's description ("the why"). Omitted when the change has no description beyond its subject so no empty section is injected.feat(review): add inter-patchset delta in re-reviews— on a re-review, surfaces the diff between the patchset VE last reviewed and the current one as a focused## Changes since last reviewed patchset (PS X → Y)section, so the agent concentrates new findings on the delta while still seeing the full change.ReviewProvider.getInterPatchsetDiff(changeId, from, to).fix(review): split maxDiffChars budget between delta and full diff— when both a delta and a full diff are present,renderSinceLastReviewandrenderDiffSectionseach independently consumed the fullmaxDiffCharsbudget, letting the prompt grow to 2×maxDiffChars. Fixed by splitting the budget equally (⌊max/2⌋ for delta, remainder for full diff) when a delta will be rendered; unchanged when no delta is present.fix(review): decouple verdict re-post from discussion replies— the silent-re-review gate previously requiredrepliesToPost.length === 0, so delivering a discussion reply on an otherwise-unchanged re-review forced the summary + vote to be re-posted. The gate now skips the verdict purely on "nothing new found + vote unchanged", while pending replies are always delivered through their own independent path.Additional test hardening:
test(gerrit): single-digit change numbers now correctly pad the shard ref.test(worker): isolates test repos from host-level git hooks (Change-Id injection causing false assertion failures).Testing
npm test— 1682 passed (106 files)npm run typecheck— cleannpm run lint— clean🤖 Co-authored with GitHub Copilot.