feat(review-pr): incremental reviews of new commits since last review… · docker/docker-agent-action@ddb0139 · GitHub
Skip to content

Commit ddb0139

Browse files
Sayt-0derekmisler
andauthored
feat(review-pr): incremental reviews of new commits since last review (#37)
Implements Phase 2D (incremental review): on PRs with multiple review cycles, re-reviews only the commits pushed since the last completed review instead of the full PR diff. Saves tokens, avoids duplicate comments, and skips unchanged code. Every unsafe or ambiguous state falls back to a full review (fail-open), so a review is never blocked or degraded. ## Issue mapping | # | Item | Implementation | | --- | --- | --- | | 31 | Incremental review state tracking | Last reviewed SHA read from the `commit_id` GitHub records on every posted review (`GET /pulls/{n}/reviews`), which is the PR head SHA at posting time. Survives across workflow runs with no extra writes and cannot be edited away like an HTML comment marker. Only completed runs count (`### Assessment:` or LGTM bodies); timeout and failure fallbacks leave commits unreviewed. | | 32 | Commit-range diffing | `git diff <last-reviewed-sha>..HEAD`, restricted at file level to the full PR diff so inline comments never anchor outside what GitHub accepts (422). Full diff fallback when no previous review exists or the base branch has been merged in (detected by comparing merge-bases at last review vs now). | | 33 | Rebases and force-pushes | `git cat-file -e` (SHA absent from the clone) and `git merge-base --is-ancestor` (history rewritten) both force a full review. | | 34 | Finding deduplication across cycles | `src/dedupe-findings/`: new comments matching an existing bot comment by file path + line proximity (tolerance 3) + finding-heading Jaccard similarity (threshold 0.5) are dropped before posting. Human comments and bot replies never suppress a finding. Runs on every posting path (staged at `/tmp/dedupe-findings.js`, existing comments pre-fetched by the action). | | 35 | `incremental` input | Added to `review-pr/action.yml` (default `"true"`) and to the reusable workflow (`type: boolean`, default `true`). `false` forces a full review. | ## Flow ``` pr.diff (full) -> incremental-review CLI |- no previous review / rebase / force-push / | base merged in / net-zero / any error -> full review (unchanged) |- otherwise: pr.diff <- git diff last-sha..HEAD (restricted to PR files) pr_full.diff <- preserved full diff (stale-thread resolution + suggestion validation) ``` ## Fallback reasons (all covered by unit tests) | Reason | Trigger | | --- | --- | | `no-previous-review` | no completed docker-agent review on the PR | | `no-new-commits` | HEAD is the last reviewed commit (explicit re-request) | | `unknown-sha` | last reviewed SHA not in the local clone (force-push) | | `history-rewritten` | last reviewed SHA not an ancestor of HEAD (rebase) | | `base-unresolved` / `base-merged-in` | base ref missing, or merge-base moved since last review | | `net-zero-changes` | changes since last review cancel out against the base | | `disabled` / `error` / `no-diff` | input off, API/git failure, missing pr.diff | ## Design notes - Anchor safety: in incremental mode the original full diff is preserved as `pr_full.diff`; stale-thread resolution and the suggestion validator run against it (GitHub validates anchors against the full PR diff, not the incremental range). - Per AGENTS.md, all decision logic lives in TypeScript (`src/incremental-review/`, `src/dedupe-findings/`) with Vitest coverage; the action YAML only orchestrates CLI calls. - The agent prompt gains an "Incremental Review" section instructing it to review only `pr.diff` contents and to scope the assessment to the new commits. ## Validation | Check | Result | | --- | --- | | `pnpm build` (tsup) | pass | | `tsc --noEmit` | pass | | `biome ci .` | pass | | `actionlint` | pass | | Unit tests | 742 passed (61 new) | | `tests/*.sh` shell integration | pass | | Bundled CLI smoke tests (dedupe drop/keep, disabled and fail-open paths) | pass | --------- Signed-off-by: Sayt0 <138035894+Sayt-0@users.noreply.github.com> Co-authored-by: Derek Misler <derek.misler@docker.com>
1 parent bfd00a3 commit ddb0139

14 files changed

Lines changed: 1666 additions & 8 deletions

File tree

‎.github/workflows/review-pr.yml‎

Lines changed: 6 additions & 0 deletions

‎AGENTS.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,18 @@ Anything else here (workflows under `.github/workflows/`, scripts, tests) exists
3939
│ │ ├── aws-credentials.ts
4040
│ │ ├── github-app.ts # Reads docker-agent-action/github-app from Secrets Manager; exports GITHUB_APP_TOKEN (a PAT) + ORG_MEMBERSHIP_TOKEN.
4141
│ │ └── __tests__/
42+
│ ├── dedupe-findings/ # Drops review comments duplicating findings from previous review cycles.
43+
│ │ ├── index.ts # CLI entry → bundled to dist/dedupe-findings.js (staged at /tmp/dedupe-findings.js for the agent).
44+
│ │ ├── dedupe-findings.ts # Core dedupeComments() pure function (path + line proximity + heading similarity).
45+
│ │ └── __tests__/
4246
│ ├── filter-diff/ # Strips excluded-path sections from a unified diff.
4347
│ │ ├── index.ts # CLI entry → bundled to dist/filter-diff.js
4448
│ │ ├── filter-diff.ts # Core filterDiff() pure function + applyFilter() I/O wrapper.
4549
│ │ └── __tests__/
50+
│ ├── incremental-review/ # Narrows pr.diff to commits since the last completed review.
51+
│ │ ├── index.ts # CLI entry → bundled to dist/incremental-review.js
52+
│ │ ├── incremental-review.ts # Core planIncrementalReview()/findLastReviewedSha() pure functions.
53+
│ │ └── __tests__/
4654
│ ├── score-confidence/ # Per-finding confidence scoring for the PR review pipeline.
4755
│ │ ├── index.ts # CLI entry → bundled to dist/score-confidence.js
4856
│ │ ├── score-confidence.ts # Core scoreFinding()/scoreFindings() pure functions + posting policy.
@@ -172,6 +180,7 @@ The action runs untrusted input (PR titles, bodies, comments, diffs) through an
172180
- `pull_request` action `review_requested` when `github.event.requested_reviewer.login == 'docker-agent'`
173181
- `@docker-agent` mentions on PR/issue comments — these run the `.github/actions/mention-reply` handler (sets `should-reply` and builds the context prompt) and then the `review-pr/mention-reply` sub-action (referenced from a pinned SHA, not present as a local path on every commit). The `pr-review-mention-reply.yaml` agent handles the actual reply.
174182
- Diffs over 1500 lines are **chunked at file boundaries** in `review-pr/action.yml` (see "Split diff into chunks"). Per-file **risk scoring** (security paths, line counts, error-handling patterns) prioritizes verifier attention.
183+
- **Incremental reviews** (default on, `incremental` input): a re-review only diffs `last-reviewed-SHA..HEAD` instead of the full PR diff. The last reviewed SHA is read from the `commit_id` GitHub records on the bot's completed reviews (assessment or LGTM bodies only — timeout/failure fallbacks don't count), so state survives across runs with no extra writes. `src/incremental-review/` plans the mode and falls back to a full review on force-push/rebase (SHA missing or not an ancestor of HEAD), base-branch merge-ins, net-zero changes, or any error. In incremental mode the original full diff is preserved at `pr_full.diff` for stale-thread resolution and suggestion-anchor validation (GitHub validates anchors against the full PR diff). On re-reviews, `src/dedupe-findings/` (staged at `/tmp/dedupe-findings.js`, run by the agent per `posting-format.md` against the pre-fetched `/tmp/existing_review_comments.json`) drops findings matching already-posted bot comments by file path + line proximity (±3) + finding-heading similarity, so a full re-review after a rebase doesn't duplicate threads.
175184
- Per-finding **confidence scoring** assigns each verified finding a precise 0–100 score (band: strong/moderate/weak/negligible) from the verifier's `verdict`, `evidence_strength`, and `context_completeness`, plus drafter↔verifier severity concordance and scope. `src/score-confidence/score-confidence.ts` is the **single source of truth** for the model (weights, bands, threshold, posting policy); the "Confidence Scoring" section of `review-pr/agents/pr-review.yaml` mirrors it as a strict lookup table so the orchestrator can apply it inline (the gitignored `dist/` is not available at agent runtime). Change one, change both — the unit tests pin every value. Security and high-severity CONFIRMED/LIKELY findings are always posted regardless of score; below-threshold findings are surfaced in a summary rather than silently dropped. The inline-posting cutoff is configurable via the `confidence-threshold` action input (a band name or a number clamped to 30–100, default `moderate` = 55); the action resolves it by invoking the bundled `dist/score-confidence.js resolve-threshold` CLI (so the resolution logic stays in TypeScript, not bash) and injects it into the agent prompt, and `scoreFinding`/`scoreFindings` accept a matching `postThreshold` option.
176185
- Stale review threads on lines no longer in the diff are auto-resolved via GraphQL `resolveReviewThread`. Threads with no `<!-- docker-agent-review -->` marker are never touched.
177186

‎review-pr/README.md‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ When using `docker/docker-agent-action/.github/workflows/review-pr.yml`:
262262
| `model` | Model override (e.g., `anthropic/claude-haiku-4-5`) | - |
263263
| `add-prompt-files` | Comma-separated files to append to the prompt | - |
264264
| `confidence-threshold` | Min confidence to post a finding inline: band (`strong`/`moderate`/`medium`/`weak`) or a number (clamped to 30–100) | `moderate` |
265+
| `incremental` | Review only the commits pushed since the last completed review (falls back to a full review when unsafe) | `true` |
265266

266267
### `review-pr` (Composite Action)
267268

@@ -285,6 +286,7 @@ PR number and comment ID are auto-detected from `github.event` when not provided
285286
| `github-token` | GitHub token | No |
286287
| `add-prompt-files` | Comma-separated files to append to the prompt | No |
287288
| `confidence-threshold` | Min confidence to post a finding inline: band (`strong`/`moderate`/`medium`/`weak`) or a number clamped to 30–100 (default `moderate`) | No |
289+
| `incremental` | Review only commits since the last completed review (default `true`; falls back to full when unsafe) | No |
288290

289291
\*API keys are optional when using the reusable workflow (credentials are fetched via OIDC). Only required when using the composite action directly without OIDC.
290292

@@ -357,6 +359,44 @@ AGENTS.md + PR Diff → Drafter (hypotheses) → Verifier (confirm + evidence si
357359
→ Confidence score (0–100) → Post Comments
358360
```
359361

362+
### Incremental Reviews
363+
364+
By default, a re-review only covers the **commits pushed since the last completed
365+
review** instead of re-reviewing the full PR diff. This saves tokens, avoids
366+
duplicate comments, and skips code that has not changed since the previous cycle.
367+
368+
How it works:
369+
370+
1. The last reviewed commit is read from the metadata GitHub records on every
371+
posted review (`commit_id` on the Reviews API is the PR head SHA at posting
372+
time), so the state survives across workflow runs with no extra bookkeeping.
373+
Only completed reviews count — a timed-out or failed run does not mark
374+
commits as reviewed.
375+
2. The diff handed to the agent becomes `git diff <last-reviewed-sha>..HEAD`,
376+
restricted to files that are still part of the full PR diff (so inline
377+
comments never anchor outside what GitHub accepts).
378+
3. The action **falls back to a full review** whenever incremental diffing
379+
would be unsafe or meaningless:
380+
381+
| Situation | Behavior |
382+
| --------- | -------- |
383+
| No previous completed review | Full review |
384+
| Force-push or rebase rewrote the history | Full review |
385+
| Base branch merged into the PR since last review | Full review |
386+
| Last reviewed SHA missing from the clone | Full review |
387+
| Changes since last review cancel out | Full review |
388+
| `incremental: false` input | Full review |
389+
390+
4. On any full re-review, findings are **deduplicated** against the comments
391+
already posted on the PR (matched by file path, line proximity, and finding
392+
heading similarity — see [`src/dedupe-findings/`](../src/dedupe-findings/dedupe-findings.ts)),
393+
so a rebase does not produce duplicate threads. The plumbing that decides
394+
between incremental and full mode is implemented and unit-tested in
395+
[`src/incremental-review/`](../src/incremental-review/incremental-review.ts).
396+
397+
Set `incremental: false` (workflow or action input) to force a full review on
398+
every trigger.
399+
360400
Each verified finding gets a precise **confidence score** (0–100) and a band
361401
(strong / moderate / weak / negligible), computed deterministically from the
362402
verifier's verdict, evidence strength, and context completeness, plus the

‎review-pr/action.yml‎

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ inputs:
7474
description: "Minimum confidence for posting a finding as an inline comment. Accepts a band name (`strong`=80, `moderate`/`medium`=55, `weak`=30) or a number (clamped to 30-100; the weak-band floor is the lowest meaningful cutoff, so e.g. `10` is raised to `30`). Findings that score below the threshold (and are not security or high-severity) are collapsed into the lower-confidence summary instead of posted inline; they are never silently dropped. Security findings and high-severity CONFIRMED/LIKELY findings are always posted regardless. Default: moderate (55), which preserves the prior behavior."
7575
required: false
7676
default: "moderate"
77+
incremental:
78+
description: "When 'true' (default), re-reviews only the commits pushed since the last completed docker-agent review instead of the full PR diff. Falls back to a full review when no previous review exists, the history was rewritten (force-push/rebase), or the base branch was merged in. Set to 'false' to force a full review."
79+
required: false
80+
default: "true"
7781

7882
outputs:
7983
exit-code:
@@ -266,6 +270,25 @@ runs:
266270
fi
267271
fi
268272
273+
# Incremental review: when a previous completed review exists, narrow pr.diff
274+
# to the commits pushed since it. The original full diff is preserved at
275+
# pr_full.diff for stale-thread resolution and suggestion-anchor validation.
276+
# All decision logic (state tracking via review commit_id metadata, rebase and
277+
# force-push detection, base-merge detection) lives in src/incremental-review.
278+
- name: Compute incremental review range
279+
if: hashFiles('pr.diff') != '' && steps.lock-check.outputs.skip != 'true'
280+
id: incremental
281+
shell: bash
282+
env:
283+
ACTION_PATH: ${{ github.action_path }}
284+
GITHUB_TOKEN: ${{ steps.resolve-token.outputs.token }}
285+
PR_NUMBER: ${{ steps.resolve-context.outputs.pr-number }}
286+
INCREMENTAL: ${{ inputs.incremental }}
287+
run: |
288+
BASE_REF=$(jq -r '.baseRefName // ""' pr_metadata.json 2>/dev/null || echo "")
289+
export BASE_REF
290+
node "$ACTION_PATH/../dist/incremental-review.js" pr.diff
291+
269292
- name: Filter excluded paths from diff
270293
if: hashFiles('pr.diff') != '' && inputs.exclude-paths != ''
271294
shell: bash
@@ -398,8 +421,14 @@ runs:
398421
# A. Parse diff → file:line set
399422
# Tracks current file from "diff --git" headers, line numbers from @@ hunks,
400423
# and emits "file:line" for each added (+) line.
401-
if [ ! -f pr.diff ]; then
402-
echo "ℹ️ No pr.diff found — skipping stale thread resolution"
424+
# In incremental mode pr.diff only covers commits since the last review, so
425+
# staleness is judged against the preserved full PR diff (pr_full.diff).
426+
DIFF_FILE=pr.diff
427+
if [ -f pr_full.diff ]; then
428+
DIFF_FILE=pr_full.diff
429+
fi
430+
if [ ! -f "$DIFF_FILE" ]; then
431+
echo "ℹ️ No $DIFF_FILE found — skipping stale thread resolution"
403432
exit 0
404433
fi
405434
@@ -426,7 +455,7 @@ runs:
426455
}
427456
/^ / { line++ }
428457
/^-/ { next }
429-
' pr.diff | sort -u > $DIFF_LINES_FILE
458+
' "$DIFF_FILE" | sort -u > $DIFF_LINES_FILE
430459
431460
DIFF_LINE_COUNT=$(wc -l < $DIFF_LINES_FILE | tr -d ' ')
432461
echo "📄 Found $DIFF_LINE_COUNT unique file:line pairs in diff"
@@ -709,6 +738,8 @@ runs:
709738
REPO: ${{ github.repository }}
710739
CONFIDENCE_SCORE: ${{ steps.resolve-confidence.outputs.score }}
711740
CONFIDENCE_LABEL: ${{ steps.resolve-confidence.outputs.label }}
741+
INCREMENTAL_MODE: ${{ steps.incremental.outputs.mode }}
742+
LAST_REVIEWED_SHA: ${{ steps.incremental.outputs.last-reviewed-sha }}
712743
run: |
713744
title=$(jq -r '.title' pr_metadata.json)
714745
author=$(jq -r '.author.login' pr_metadata.json)
@@ -740,6 +771,15 @@ runs:
740771
echo "- **Inline confidence threshold**: ${CONFIDENCE_SCORE}/100 (${CONFIDENCE_LABEL} band). When applying the Confidence Scoring posting policy, post a non-forced finding as an inline comment only when its confidence score is **${CONFIDENCE_SCORE} or higher**. A finding that is in scope and not dismissed but scores **below ${CONFIDENCE_SCORE}** must NOT be posted inline — surface it under \"Lower-confidence findings (not posted inline)\" in the review body instead, exactly as the posting policy already prescribes for below-threshold findings (the policy's negligible-band rules still govern what is summarized vs. dropped)."
741772
echo "- This threshold does NOT override the security floor or the high-severity always-post rule: \`security\` findings and high-severity CONFIRMED/LIKELY findings are ALWAYS posted inline regardless of the threshold."
742773
echo ""
774+
if [ "$INCREMENTAL_MODE" = "incremental" ]; then
775+
echo "## Incremental Review"
776+
echo ""
777+
echo "This is an INCREMENTAL review: \`pr.diff\` contains only the changes pushed since the last completed review (commit \`${LAST_REVIEWED_SHA}\`). Earlier commits were already reviewed — do NOT re-review them or fetch the full PR diff yourself."
778+
echo "- The \"Changed Files\" list above still describes the whole PR; review only the files present in \`pr.diff\`."
779+
echo "- Start the review body with a note that this review covers only the commits since \`${LAST_REVIEWED_SHA:0:12}\`, and scope the assessment label to those changes."
780+
echo "- If \`pr.diff\` is missing, do NOT fall back to fetching the full diff — post a COMMENT review explaining the incremental diff was unavailable."
781+
echo ""
782+
fi
743783
echo "---"
744784
echo ""
745785
echo "## Instructions"
@@ -786,6 +826,34 @@ runs:
786826
else
787827
echo "::warning::validate-suggestions.js not found in dist — suggestion validation will be skipped"
788828
fi
829+
# Stage the finding deduplicator the same way (see posting-format.md).
830+
if [ -f "$ACTION_PATH/../dist/dedupe-findings.js" ]; then
831+
cp "$ACTION_PATH/../dist/dedupe-findings.js" /tmp/dedupe-findings.js
832+
else
833+
echo "::warning::dedupe-findings.js not found in dist — finding deduplication will be skipped"
834+
fi
835+
836+
# Pre-fetch the PR's existing inline review comments so the agent can drop
837+
# findings already posted in a previous review cycle (the dedupe CLI is
838+
# fail-open: a missing or empty file simply skips deduplication).
839+
- name: Fetch existing review comments
840+
if: steps.lock-check.outputs.skip != 'true'
841+
continue-on-error: true
842+
shell: bash
843+
env:
844+
GH_TOKEN: ${{ steps.resolve-token.outputs.token }}
845+
PR_NUMBER: ${{ steps.resolve-context.outputs.pr-number }}
846+
REPO: ${{ github.repository }}
847+
run: |
848+
set -o pipefail
849+
if gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" \
850+
| jq -s 'map(if type == "array" then . else [.] end) | add // []' \
851+
> /tmp/existing_review_comments.json; then
852+
echo "✅ Fetched $(jq 'length' /tmp/existing_review_comments.json) existing review comment(s)"
853+
else
854+
rm -f /tmp/existing_review_comments.json
855+
echo "::warning::Failed to fetch existing review comments — deduplication will be skipped"
856+
fi
789857
790858
# ========================================
791859
# RUN REVIEW using root docker-agent-action

‎review-pr/agents/pr-review.yaml‎

Lines changed: 8 additions & 3 deletions

0 commit comments

Comments
 (0)