Review a force-pushed pull request incrementally instead of falling back to the whole diff

Aberta
  • Algo errado?
#587 7 comentários 0 reações 1 responsável Ver no GitHub

Mantenedores costumam responder em até 1 dia

@Alan52254 já está trabalhando nisso.

Desde 6/11/2026.

  • #605 de @Alan52254 — aberto

Avaliação

Dificuldade
4/5
Tempo estimado
3-5 dias
Facilidade para iniciantes
48/100
Tipo de issue
Funcionalidade
Clareza
Claramente especificada
Status de atividade
Ativa
Stack de tecnologia
git, shell
Domínio
tooling

Direção de pesquisa

Start with .claude/skills/codetrial-review/review-bundle.sh around lines 338–363, then read the related SKILL.md and lenses.md guidance. Trace how --since resolves heads and writes the manifest, and inspect the existing shell test setup before implementing the four described cases. Done means the bundle reports rewritten or rebased updates as specified, falls back for an unresolved head, and ./scripts/test.sh passes.

Escrita pelo modelo de indexação a partir do texto da issue.

Descrição

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.sh takes an optional --since-base <sha> next to --since. When --since is not an ancestor and the old head can be resolved locally or fetched by SHA, it writes since.diff for the same-merge-base case and since.range-diff.txt for the moved-base case, and records which one in the manifest as status: "rewritten" or status: "rebased". A short --since is expanded to the full SHA first, because git fetch does 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.md and lenses.md say 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 #585, --since d8d4fe8 writes a since.diff containing the 22-line diff of the author's change instead of recording not an ancestor, and the manifest says rewritten.
  • After a conflict-free rebase in the new test, since.range-diff.txt shows 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.sh passes, 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.

Linguagem predominante
Rust
Estrelas
223
Forks
148
Merge médio
1d 10h
PRs com merge (30d)
227

Preparar o ambiente

Primeiros passos

  1. Leia a issue inteira e depois o guia de contribuição do projeto.
  2. Comente na issue dizendo que vai assumir — evita que duas pessoas façam o mesmo trabalho.
  3. Faça um fork do repositório e trabalhe em uma branch.
  4. Abra um pull request que referencie o número da issue.

Mais de sysprog21/codetrial

Todas as issues de sysprog21/codetrial

Issues semelhantes

Mais issues de Rust

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.