Repository navigation
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
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>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
|
||
| def test_a_rewrite_on_one_base_gives_the_tree_diff(self): | ||
| with tempfile.TemporaryDirectory() as repo: | ||
| git, commit, at = self.history(repo) |
There was a problem hiding this comment.
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>
| git, commit, at = self.history(repo) | |
| _, _, at = self.history(repo) |
There was a problem hiding this comment.
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.
|
|
| if [ -z "$old_base" ]; then | ||
| old_base=$(git merge-base "$old" "$target") || return 1 | ||
| fi |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
--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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A reviewer who asks for a rebase or a squash gets a force push back, and --since then fell back to the whole diff, so the most common update during review was the one that cost the most to read again. Comparing the old and new heads in the checkout answers it: a tree diff when the base did not move, and a range-diff when it did. That fetches objects the bundle did not need before, and the commit pairing is a heuristic, so both files only say what to read first and the whole diff is still reviewed. An old head nobody can resolve keeps the earlier behavior. Close sysprog21#587 Change-Id: Ibf7947f75441fdbdb799d3ea0a37b5745412f8db
jserv
added
the
explained
e47d958 to
b22232f
Compare
jserv
left a comment
jserv
left a comment
There was a problem hiding this comment.
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.
| # whenever it is there. | ||
| rm -rf "$dir/head" "$dir/since.diff" "$dir/since.txt" "$dir/pr.json" \ | ||
| rm -rf "$dir/head" "$dir/since.diff" "$dir/since.txt" \ | ||
| "$dir/since.range-diff.txt" "$dir/pr.json" \ |
There was a problem hiding this comment.
An interrupted run (Ctrl-C between git range-diff and the rm after awk) leaves since.range-diff.raw in the bundle, and this list does not clear it, so a later run that takes the ahead or rewritten path ships it beside fresh files. Clear it here with the rest:
| "$dir/since.range-diff.txt" "$dir/pr.json" \ | |
| "$dir/since.range-diff.txt" "$dir/since.range-diff.raw" "$dir/pr.json" \ |
| compare_rewrite() | ||
| { | ||
| case $status in | ||
| diverged | behind) ;; |
There was a problem hiding this comment.
When the compare API fails (a network error, or GitHub no longer resolving the old head), status is unknown and the comparison is skipped even when every commit is already in the local clone, which is the usual case for the reviewer's own checkout. unknown can take the same path, since compare_rewrite already falls back when it cannot resolve or fetch the commits:
| diverged | behind) ;; | |
| diverged | behind | unknown) ;; |
| --arg old_base "$old_base" --arg new_base "$new_base" \ | ||
| '{from: $from, status: $status, old_base: $old_base, new_base: $new_base}') | ||
| else | ||
| rm -f "$dir/since.diff" "$dir/since.txt" "$dir/since.range-diff.txt" "$dir/since.range-diff.raw" |
There was a problem hiding this comment.
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.
| commit_of "$sha" > /dev/null && continue | ||
| GIT_TERMINAL_PROMPT=0 git fetch -q --no-tags --no-write-fetch-head \ | ||
| "https://github.com/$repo.git" \ | ||
| "$old" "$head" "$target" ${old_base:+"$old_base"} 2> /dev/null || return 1 |
There was a problem hiding this comment.
The loop only checks whether one SHA is missing, then fetches all of $old, $head, $target and $old_base in one call. The usual reviewer clone already holds the old head, and a fetch that names an object the remote no longer serves fails as a whole. In that case compare_rewrite returns 1 and the bundle falls back to the full diff even though only the new head or base was missing. Collect the SHAs that commit_of cannot resolve and fetch only those, keeping the fallback when one of them cannot be fetched.
|
Some review comments on this pull request are not settled yet. Please refine the change per the review comments listed below, reply on each thread with what you changed or why you did not, and mark each thread resolved yourself once it is settled (Resolve conversation, at the bottom of the thread). The pull request stays blocked while review threads are open. Not addressed at the current head:
Addressed at the current head; reply with what changed and resolve:
Checked at b22232f. |
Problem and behavior
A reviewer who asks for a rebase or a squash gets a force push back, and
--sincethen recordsnot an ancestor of the head, so the whole diff is reviewed. After this change the bundle says what the author changed:since.diffholds the tree diff of the two heads and the manifest saysrewritten. For Work through review findings across pushes #585 fromd8d4fe8that is 22 lines instead of 1130.since.range-diff.txtholdsgit range-diffof the old commits against the new ones and the manifest saysrebased. A commit whose only change is itsChange-Idreads as unchanged.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-basenames the old base when the old head's merge base with the PR's base is not where it started.--creation-factor=100keeps 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, soSKILL.mdandlenses.mdsay 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 allpasses exceptshell-syntax, which reports SC2319 inscripts/git-post-commit.sh;shellcheckgives identical output onmain. Removing the new branch fails the new tests.rewrittenwith 22 lines,rebasedwith 624 lines of 1284, andrebasedwith 925 lines of 1200.e64598e, which touches none of their files, read as=in 4 lines, with or without everyChange-Idreplaced. One more file added to the second commit marks only that commit!.awkorgitwere run by hand, with injected failures, and have no test in the tree.Close #587
Summary by cubic
After a force push, the review bundle now shows what the author changed since the earlier review instead of treating the whole diff as new. Same-base rewrites (squash, amend, reorder) produce a tree diff in
since.diff(manifestrewritten); a rebase produces agit range-diffinsince.range-diff.txt(manifestrebased). An old head that cannot be resolved keeps the previous whole-diff behavior.Details
--since-base <sha>names the old base when the old head's merge base with the PR's base is not where it started.Change-Idread as unchanged; comparison fetches objects the bundle previously did not need.Closes #587.
Written for commit b22232f. Summary will update on new commits.