Repository navigation
Review a force-pushed pull request incrementally instead of falling back to the whole diff #587
Description
Problem
review-bundle.sh --since <sha> gives a reviewer only the work added since the head an earlier review read, but only when that head is an ancestor of the new head and no merge commit lies between them (.claude/skills/codetrial-review/review-bundle.sh, lines 338-363). Otherwise the manifest records not an ancestor of the head, so the whole diff is reviewed, and no since.diff is written.
Maintainers routinely ask for a rebase or for commits to be squashed and the author answers with a force push, which is exactly the case where the old head stops being an ancestor. So the update that happens most often during review is the one that gets no incremental range, and the reviewer rereads a diff they have already read.
Current behavior
I ran review-bundle.sh 585 <dir> sysprog21/codetrial --since <sha> on main b2c2bc9 for three earlier heads of #585, which was force pushed before it merged: 9bfab6c, d8d4fe8 and 533bc5a (taken from the commit ids of its reviews and review comments). Each run records status: "diverged" with not an ancestor of the head, so the whole diff is reviewed, and writes no since.diff. These heads can still be fetched by full SHA (git fetch rejects the short form), and all four heads share the base efdd342.
For that pull request the whole diff is 1130 lines. What the author changed between d8d4fe8 and the final head is a 22-line diff that deletes 6 lines in one file (git diff d8d4fe8 6bf2003).
git range-diff efdd342..d8d4fe8 efdd342..6bf2003 is 133 lines for the same change, because every rewritten commit also gets a new Change-Id trailer and the commit message hunks show it. So range-diff alone is not the right first tool when the base did not move.
Desired outcome
When --since is not an ancestor, the bundle still tells the reviewer what the author changed since the head they read:
- If the old head and the new head have the same merge base with the target branch (a squash, an amend, a reorder), the answer is the plain tree diff
git diff <old head> <new head>. - If the base moved (a rebase), the answer is
git range-diff <old base>..<old head> <new base>..<new head>, so that a conflict-free rebase shows no change in any commit and a conflict resolution or an amended commit shows up as a change to read first.
Proposed scope
One pull request, limited to .claude/skills/codetrial-review/:
review-bundle.shtakes an optional--since-base <sha>next to--since. When--sinceis not an ancestor and the old head can be resolved locally or fetched by SHA, it writessince.difffor the same-merge-base case andsince.range-diff.txtfor the moved-base case, and records which one in the manifest asstatus: "rewritten"orstatus: "rebased". A short--sinceis expanded to the full SHA first, becausegit fetchdoes not accept the short form.- In the range-diff output, lines that only differ by a
Change-Id:trailer are not shown, so a hook-driven rechain is not reported as a change. - When the old head cannot be resolved, the behavior is unchanged: the manifest says why and the whole diff is reviewed.
SKILL.mdandlenses.mdsay that these files are a pointer to what to read first, not a replacement for the full diff, because the pairing is heuristic and merge commits are ignored by default.- A shell test that builds a small repository and checks four outcomes: a squash on an unchanged base, a conflict-free rebase, a rebase with a conflict resolved, and an unresolvable old head falling back.
Out of scope: a merge of the base into the branch (the whole diff is still reviewed), Change-Id tracking, and any change to memory.py or the findings ledger.
Open question
The old base is not recorded anywhere today; the manifest stores only the base of the current bundle. Should the caller pass --since-base explicitly, or should the manifest of the previous bundle be read when it is present? I would start with the explicit flag, which needs no new state.
Acceptance criteria
- For Work through review findings across pushes #585,
--since d8d4fe8writes asince.diffcontaining the 22-line diff of the author's change instead of recordingnot an ancestor, and the manifest saysrewritten. - After a conflict-free rebase in the new test,
since.range-diff.txtshows every commit as unchanged. - After a rebase with a conflict resolved, it shows that change and nothing else.
- With an unresolvable old head, the output is the same as today.
./scripts/test.shpasses, and the new test fails if the new branch is removed.
Related
#585 added a findings ledger that places findings again after a push; it does not change which diff a reviewer reads after a force push. I searched open and closed issues and pull requests for range-diff and found none.
Activity
jserv commentedon Nov 4, 2026
Don't make such statements, as installing
jqis straghtforward.jserv commentedon Nov 5, 2026
Either approach is fine for enforcing the review-before-submission workflow.
Alan52254 commentedon Nov 5, 2026
Installed
jqand ran the script onmainb2c2bc9. For #585,--sincewith each of9bfab6c,d8d4fe8and533bc5arecordsstatus: "diverged"andnot an ancestor of the head, so the whole diff is reviewed, and writes nosince.diff. I have replaced that sentence in the issue with this result, and corrected two details: the change fromd8d4fe8to the final head is a 22-line diff that deletes 6 lines, and earlier heads must be fetched by full SHA.On the open question, I will use the explicit
--since-baseflag, since it needs no new state. The proposed scope is otherwise unchanged. If it is acceptable, please assign this issue to me; I will not write code before then.jserv commentedon Nov 5, 2026
Avoid statements like the one above. We are here to collaborate and improve CodeTrial, and your contributions demonstrate your professionalism. You may need to write a proof of concept before the issue is assigned to you to help clarify the scope and assess the proposed approach.
Alan52254 commentedon Nov 6, 2026
Proof of concept: main...Alan52254:codetrial:review-bundle-rewritten-since (e47d958, on
mainat b2c2bc9). It changesreview-bundle.sh,SKILL.mdandlenses.mdin.claude/skills/codetrial-review/, plustests/test_skill_scripts.pyand its stubtests/stubs/skill-scripts/gh-bundle.sh.--sinced8d4fe8rewrittensince.diff, 22 linese64598erebasedsince.range-diff.txt, 624 linescaff855rebasedsince.range-diff.txt, 925 linesAfter a rebase the tree diff is longer than the whole diff, because it also carries what the base brought in. Neither rebase above is free of real changes, so I rebased the four commits of #585 onto
e64598e, which touches none of their files. All four read as=and the file is 4 lines, and the same holds with everyChange-Idreplaced. With one more file added to the second commit, only that commit reads as!.It differs from the proposal in five ways:
tests/test_skill_scripts.py, where the bundle's other tests live, written in Python rather than shell.--since-baseis optional; the old base defaults to the old head's merge base with the PR's base.behindis compared as well asdiverged.git range-diffruns with--creation-factor=100. The default listed a small commit with a resolved conflict as unrelated commits.!or a message line shaped like a header cannot mark a changed commit as unchanged.The range-diff saves little when a commit was reworked: 925 of 1200 lines on #586. The files only say what to read first, and the whole diff is still reviewed.
since.diffuses plaingit diff, so it does not count lines the waycommit-sizesdoes. The fetch path and the cleanup after a failedawkorgitwere checked by running them, in a clean clone and with injected failures, and have no test in the tree../scripts/test.sh allpasses exceptshell-syntax, which reports SC2319 inscripts/git-post-commit.sh;shellcheckgives identical output onmain. Removing the new branch fails the new tests.jserv commentedon Nov 6, 2026
We may need incremental diffing or semantic code comparison along the lines of OmniDiff. Don't copy/invoke OmniDiff directly, as its license is incompatible with CodeTrial.
Alan52254 commentedon Nov 6, 2026
The pull request keeps to the tree diff and the commit pairing. Both read the objects in the checkout, add no dependency, and fall back to the whole diff when they cannot answer.
A semantic comparison needs a parser.
diffmodel.pypairs moved units by indentation, whichworkbench.mdcalls a guess at syntax and not a parse, so a comparison that survives a rewrite belongs on top of that pairing and has no use for OmniDiff. I will open it as its own issue, with the rewrites of #585, #584 and #586 as its cases.Show what a force push changed since a review
jserv commentedon Nov 7, 2026
Agreed. Next time, avoid unnecessary hyperlinks such as
[#586](https://github.com/sysprog21/codetrial/pull/586). Write#586instead.