Skip to content

v1.12.3.0 fix: /review and /ship use three-dot diff (#1152)#1200

Open
dgrant wants to merge 4 commits intogarrytan:mainfrom
dgrant:fix/review-diff-three-dot
Open

v1.12.3.0 fix: /review and /ship use three-dot diff (#1152)#1200
dgrant wants to merge 4 commits intogarrytan:mainfrom
dgrant:fix/review-diff-three-dot

Conversation

@dgrant
Copy link
Copy Markdown

@dgrant dgrant commented Apr 25, 2026

Summary

  • Fixes /review and /ship show false deletions when origin/<base> is ahead of the feature branch #1152. /review and /ship ran git diff origin/<base> (two-dot, symmetric diff between tips). When origin/<base> had moved ahead of the feature branch, any commits that landed on base but were not yet merged into the branch appeared as deletions in the review output, and reviewers flagged them as regressions.
  • Switched every diff invocation in the review and ship pipelines to three-dot (git diff origin/<base>...HEAD), which diffs from the merge-base. This matches GitHub's "Files changed" tab and what the prose around these commands always claimed they were doing.
  • Scope expanded from /review to also include /ship, since ship's pre-landing review pass shares the same resolvers (review.ts, review-army.ts) and the standalone ship-template diff calls had the same bug.

Files

  • review/SKILL.md.tmpl — no-diff check (Step 1) and full-diff (Step 3)
  • scripts/resolvers/review.ts — adversarial diff-stat sizing, Claude subagent prompt, Codex challenge prompt
  • scripts/resolvers/review-army.ts — specialist diff-stat sizing, specialist dispatch prompt, Red Team prompt
  • ship/SKILL.md.tmpl — distribution pipeline check, prompt-files detection, pre-landing review
  • review/checklist.md, review/greptile-triage.md — doc references
  • review/SKILL.md, ship/SKILL.md — regenerated via bun run gen:skill-docs

Test plan

  • bun run gen:skill-docs regenerates cleanly; no unexpected diff
  • bun test test/skill-validation.test.ts test/gen-skill-docs.test.ts — passes (one pre-existing fixture-size failure unrelated to this change, reproduces on main)
  • Manual: on a branch where origin/<base> has commits not merged in, run /review and confirm those commits no longer appear as deletions in the output

Copilot AI review requested due to automatic review settings April 25, 2026 04:10
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes incorrect diff computation in the /review and /ship workflows by switching their git diff usage from two-dot (tip-to-tip) to three-dot (merge-base), matching GitHub’s “Files changed” semantics and avoiding base-branch-only commits showing up as deletions.

Changes:

  • Update /review and /ship templates/docs to use git diff origin/<base>...HEAD for full diff and file detection steps.
  • Update resolver-generated prompts (adversarial review + specialist army) to size and review against three-dot diffs.
  • Bump version to 1.12.3.0 and add changelog entry documenting the fix.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
review/SKILL.md.tmpl Switch review workflow diff checks to three-dot merge-base diff.
review/SKILL.md Regenerated review skill doc reflecting updated diff semantics.
scripts/resolvers/review.ts Update adversarial review sizing/prompt text to use three-dot diff.
scripts/resolvers/review-army.ts Update specialist sizing and prompts to use three-dot diff.
ship/SKILL.md.tmpl Switch ship workflow checks/review prompts to three-dot diff.
ship/SKILL.md Regenerated ship skill doc reflecting updated diff semantics.
review/checklist.md Update checklist instructions to reference <base>...HEAD diff.
review/greptile-triage.md Update triage instructions to cross-reference merge-base diff.
VERSION Bump version to 1.12.3.0.
CHANGELOG.md Add release notes describing the diff behavior fix (issue #1152).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ship/SKILL.md.tmpl Outdated
Comment thread ship/SKILL.md.tmpl
Comment thread ship/SKILL.md.tmpl Outdated
Comment thread scripts/resolvers/review.ts Outdated
Comment thread scripts/resolvers/review-army.ts Outdated
Comment thread review/greptile-triage.md Outdated
Comment thread review/SKILL.md.tmpl Outdated
@dgrant
Copy link
Copy Markdown
Author

dgrant commented Apr 25, 2026

@copilot-pull-request-reviewer — thanks, all 7 of your comments hit the same real issue: three-dot origin/<base>...HEAD excludes working-tree changes, so any review/ship step running mid-work would silently miss uncommitted edits. I caught this after pushing the initial three-dot commits and switched the entire pipeline to the merge-base form in commits 1c7686a3 (source) and 8bc5052e (regen):

git diff $(git merge-base origin/<base> HEAD)

This form diffs the working tree against the merge-base, which:

Empirically verified against a synthetic repo with one committed change + one uncommitted line + a base that moved ahead. Merge-base was the only form that got both right. Comparison table is in the rewritten CHANGELOG entry (commit 32e969d3) and the updated issue #1152 body.

Per-comment status:

  1. ship/SKILL.md.tmpl distribution check — fixed (uses merge-base, picks up uncommitted entry-point files).
  2. ship/SKILL.md.tmpl prompt-files detection (line 182) — fixed (uses merge-base, picks up uncommitted prompt edits).
  3. ship/SKILL.md.tmpl pre-landing review (Step 11) — fixed (uses merge-base; no separate cached/worktree pass needed).
  4. scripts/resolvers/review.ts adversarial size — fixed (uses merge-base; DIFF_TOTAL now reflects what reviewer sees).
  5. scripts/resolvers/review-army.ts specialist size — fixed (uses merge-base; 50-line threshold no longer bypassed by uncommitted-only branches).
  6. review/greptile-triage.md <base> definition — partially addressed: the doc still uses <base> as the placeholder. The skill template's Step 0 detects the actual base branch dynamically (gh pr view --json baseRefName etc.) and substitutes throughout. Triage is run from inside the skill, not standalone, so the substitution is upstream. Happy to add a clarifying note if that's preferred.
  7. review/SKILL.md.tmpl Step 1 no-diff check — fixed (uses merge-base, so a branch with only uncommitted changes no longer triggers "Nothing to review").

About to squash-rebase the 7 commits into a clean 4-commit sequence (fix → tests → regen → version) so the history doesn't show the three-dot mistake. Force-push incoming.

dgrant and others added 4 commits April 24, 2026 22:23
…an#1152)

The /review, /ship, /qa, and /design-review workflows ran
`git diff origin/<base>` to compute the branch diff. That two-dot form is
the symmetric diff between the two tips: when origin/<base> moved ahead
of the feature branch, any commits that landed on base but were not yet
merged into the branch appeared as deletions in the review output, and
reviewers flagged them as regressions.

The straightforward fix is three-dot (`origin/<base>...HEAD`), but it
silently drops uncommitted working-tree edits — `/review` mid-work would
show "Nothing to review" if HEAD was at the merge-base, even with
uncommitted changes.

Use the merge-base form instead:

    git diff $(git merge-base origin/<base> HEAD)

Equivalent to "working tree vs the merge-base." Excludes commits on
origin/<base> not yet merged into HEAD (no false deletions). Includes
uncommitted working-tree edits (no silent drop). Empirically verified on
a synthetic repo with one committed change + one uncommitted line + a
base that moved ahead.

Touches:
- review/SKILL.md.tmpl: no-diff check, full-diff step
- ship/SKILL.md.tmpl: distribution check, prompt-files detection,
  pre-landing review, ship-readiness summary, test coverage audit, plan
  completion audit, diff-size check
- scripts/resolvers/review.ts: scope-drift, plan completion, adversarial
  sizing, Claude subagent prompt, Codex challenge prompt
- scripts/resolvers/review-army.ts: specialist sizing, specialist
  dispatch prompt, Red Team prompt
- scripts/resolvers/testing.ts: codepath-trace prompt
- scripts/resolvers/utility.ts: /qa branch-diff scoping, changelog step
- scripts/resolvers/design.ts: /design-review branch-diff scoping
- scripts/resolvers/preamble/generate-test-failure-triage.ts: triage
  --name-only lookup
- scripts/slop-diff.ts: merge-base with three-dot fallback
- review/checklist.md, review/greptile-triage.md: doc references

Fixes garrytan#1152.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two skill-validation/gen-skill-docs assertions were pinned to the literal
substring `git diff origin/`. After switching to
`git diff $(git merge-base origin/<base> HEAD)`, the substring no longer
appears verbatim. Update both to match the new form.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mechanical regen via `bun run gen:skill-docs` to pick up the merge-base
diff change in the source. Affects review, ship, qa, qa-only, and
design-review (the latter three reach the same resolvers via shared
testing/utility/design templates).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bumps VERSION to 1.12.3.0, syncs package.json, adds CHANGELOG entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dgrant dgrant force-pushed the fix/review-diff-three-dot branch from 32e969d to 97caf53 Compare April 25, 2026 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/review and /ship show false deletions when origin/<base> is ahead of the feature branch

2 participants