[Alan52254]

Problem and behavior

A reviewer who asks for a rebase or a squash gets a force push back, and --since then records not an ancestor of the head, so the whole diff is reviewed. After this change the bundle says what the author changed:

• On the same base (a squash, an amend, a reorder), since.diff holds the tree diff of the two heads and the manifest says rewritten. For Work through review findings across pushes #585 from d8d4fe8 that is 22 lines instead of 1130.
• After a rebase, since.range-diff.txt holds git range-diff of the old commits against the new ones and the manifest says rebased. A commit whose only change is its Change-Id reads as unchanged.
• An old head that cannot be resolved or compared keeps the earlier behavior.

Approach

A tree diff after a rebase also carries what the base brought in: for #584 it is 1754 lines against a whole diff of 1284. So a moved base gets a range-diff, and --since-base names the old base when the old head's merge base with the PR's base is not where it started.

--creation-factor=100 keeps a small commit with a resolved conflict paired with its old version, at the cost of pairing commits that may be unrelated. The pairing is a heuristic and skips merge commits, so SKILL.md and lenses.md say both files only point to what to read first and the whole diff is still reviewed. The comparison also fetches objects the bundle did not need before, by full SHA.

A commit's marker is read only from its header line, so a subject containing ! or a message line shaped like a header cannot mark a changed commit as unchanged.

The semantic comparison jserv raised on #587 is not part of this change.

Validation

• ./scripts/test.sh all passes except shell-syntax, which reports SC2319 in scripts/git-post-commit.sh; shellcheck gives identical output on main. Removing the new branch fails the new tests.
• Replayed on Work through review findings across pushes #585, Preselect the configured interview length #584 and Expand the Example drawing over the editor panel #586: rewritten with 22 lines, rebased with 624 lines of 1284, and rebased with 925 lines of 1200.
• Four commits of Work through review findings across pushes #585 rebased onto e64598e, which touches none of their files, read as = in 4 lines, with or without every Change-Id replaced. One more file added to the second commit marks only that commit !.
• The fetch path in a clean clone and the cleanup after a failed awk or git were run by hand, with injected failures, and have no test in the tree.

Close #587

---

Summary by cubic

After a force push, --since compares the old and new heads instead of always reviewing the whole diff: same-base rewrites produce since.diff (rewritten), and rebases produce since.range-diff.txt (rebased). If comparison fails, the bundle keeps the whole-diff behavior.

• Use --since-base <sha> when the old head's original base cannot be inferred.
• These files only point to what to read first; range-diff pairing is heuristic and skips merge commits, and the full diff still needs review.
• Comparison may fetch missing Git objects; commits changed only by a Change-Id are treated as unchanged.

Closes #587.

^{Written for commit a6b6833. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/test_skill_scripts.py">

<violation number="1" location="tests/test_skill_scripts.py:2105">
P2: This new test assigns `git` and `commit` but never uses them, which triggers the repository's Ruff `F841` lint gate. Unpack the unused values as `_` so the test suite remains lint-clean.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic</sub>

[cubic-dev-ai]

P2: This new test assigns git and commit but never uses them, which triggers the repository's Ruff F841 lint gate. Unpack the unused values as _ so the test suite remains lint-clean.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/test_skill_scripts.py, line 2105:

<comment>This new test assigns `git` and `commit` but never uses them, which triggers the repository's Ruff `F841` lint gate. Unpack the unused values as `_` so the test suite remains lint-clean.</comment>

<file context>
@@ -2050,6 +2053,193 @@ def test_an_earlier_head_gives_the_new_work_only_when_it_is_an_ancestor(self):
+
+    def test_a_rewrite_on_one_base_gives_the_tree_diff(self):
+        with tempfile.TemporaryDirectory() as repo:
+            git, commit, at = self.history(repo)
+            git("checkout", "-qb", "new", at["main0"])
+            head = commit("one\nTWO\nthree\nfour\n", "Change two", "b")
</file context>

Suggested change

	git, commit, at = self.history(repo)
	_, _, at = self.history(repo)

[Alan52254]

Not changed: Ruff does not report F841 for the unused names of a tuple unpacking. ruff check --select F841 tests/test_skill_scripts.py passes on this file with ruff 0.17.3, and the gates (portable) check passes.

[Alan52254]

interactive-review fails on this pull request at actions/checkout with "Refusing to check out fork pull request code from a 'pull_request_target' workflow". It also fails on #584, #586, #590 and #603, which come from forks, and it passed on three runs for the fix branch of this repository, so the change here is not the cause.

[jserv]

A --since-base that the old head does not descend from is accepted as is. Then old_base..$old and the tree diff both describe a history that never existed. test_a_named_old_base_is_the_old_base shows it: old sits on main0, --since-base main1 makes old_base equal new_base, and since.diff reports b.js from main1 as "rewritten" by the author. That is the base drift this change is meant to keep out. Refuse a base the old head does not contain, so a wrong flag falls back to the whole diff instead of a misleading pointer, and change that test to build old on the base it names.

Suggested change

	if [ -z "$old_base" ]; then
	old_base=$(git merge-base "$old" "$target") || return 1
	fi
	if [ -z "$old_base" ]; then
	old_base=$(git merge-base "$old" "$target") || return 1
	fi
	git merge-base --is-ancestor "$old_base" "$old" || return 1

[Alan52254]

--since-base is now refused when the old head does not contain it, and the bundle falls back to the whole diff: the base resolution in compare_rewrite ends with git merge-base --is-ancestor "$old_base" "$old" || return 1. test_a_named_old_base_is_the_old_base now builds old on main1 and names main0, so the flag still takes effect when the old head contains the base. test_a_rewrite_that_cannot_be_compared_reviews_the_whole_diff gets a case that names main1 for an old head on main0; the class fails without that line.

[jserv]

Every way compare_rewrite can fail lands here with why saying "not an ancestor of the head", which is wrong for most of them: a --since-base the old head does not contain (the new test passes badbad1 and main1), a fetch refused or rejected by a git older than 2.29 (--no-write-fetch-head), a missing merge base in a shallow clone, or a failed range-diff. SKILL.md promises the manifest says why. The smallest fix is one accurate reason for a diverged or behind head whose comparison failed, such as "the rewrite could not be compared, so the whole diff is reviewed"; a why set at each return 1 is the fuller version if callers need to tell the cases apart.

[Alan52254]

The why now follows the status: merged keeps its text, diverged and behind say "the rewrite could not be compared, so the whole diff is reviewed", and anything else, which is unknown when the compare call fails and the commits are not in the clone, says "the comparison with that head failed, so the whole diff is reviewed". test_an_old_head_nobody_can_resolve_reviews_the_whole_diff asserts the second text and the new unknown case asserts the third. A why set at each return 1 is not added in this change. Fixed in 1f9bae7.

[jserv]

The interactive-review check changed in #610: the explained label no longer passes it, and this pull request is over the large-change thresholds, so its next run needs a signed record of the interactive review.

1. Once: set up a signing key, as "Before your first large change" in CONTRIBUTING.md lists.
2. Rebase this branch onto main; the pre-push hook that signs and sends the record is there.
3. Run /codetrial-review on the branch and push. If the branch did not change, send the record with sh scripts/git-pre-push.sh --publish-review <fork-remote> [<branch>], then close and reopen this pull request.

Each refusal and its fix: https://github.com/sysprog21/codetrial/blob/main/docs/large-change-explanation.md#troubleshooting

[jserv]

Rebase the current branch onto the upstream default branch and resolve any conflicts. Run /codetrial-review in AI agents (use $codetrial-review in Codex) and resolve all identified issues. Rework the series into minimal, logically coherent commits, squash closely related commits, and ensure all commit messages follow the project’s rules.

[jserv]

@Alan52254 This pull request makes review-bundle.sh fetch commits the clone lacks by SHA, which #587 accepts. .claude/skills/codetrial-triage/review-prs.md (step 1 of "What each agent does") still says of a review that "Nothing is checked out, fetched, built or edited", so once this merges that sentence is false for the unattended sweep.

Please amend that sentence in this pull request: keep "checked out, built or edited", and say that the bundle may fetch commits the clone lacks by SHA into its object store, which changes no ref and no working tree. Reply here if you think the sentence belongs in a separate change.

[jserv]

"Is refused" promises an error, but a --since-base the old head does not contain makes compare_rewrite return 1, and the run exits 0 with status: diverged and the generic "the rewrite could not be compared" (which test_a_rewrite_that_cannot_be_compared_reviews_the_whole_diff pins). A reviewer who mistyped the base gets a full review with no hint that the argument was the reason. Either exit 2 there like the other bad arguments, or keep the fallback and have why name --since-base, then reword this sentence to match.

[jserv]

This line is the boundary between "only the Change-Id moved" and "the author changed something", and no test exercises its message side: the conflict test changes code, and the rebase test changes only the Change-Id. The most common answer to a review is a reworded commit body under a new Change-Id; add a rebase where only the message body changes and assert the header keeps ! and the reworded line is shown, so a later tightening of the strip pattern cannot quietly turn that into =.