Fix detection of changed packages #564
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughRefactors PR/build logic to explicitly handle base, head, and merge commits; updates checkout paths based on selected mode; adds shallow fetch depth control; adjusts build_pr to fetch merge commit and derive parents; updates tests to use a shared repo setup helper returning base/head/merge SHAs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI/User
participant Review as Review.build_pr
participant Git as Git Repo
participant Builder as Review.build_commit
Note over Review,Git: PR evaluation (local/ofborg/action)
User->>Review: build_pr(pr, options)
Review->>Git: fetch_refs(pr.merge_commit_sha, shallow_depth=2)
Review->>Git: verify_commit_hash(pr.merge_commit_sha)
Git-->>Review: merge_rev
Review->>Git: derive parents (merge_rev^1, merge_rev^2)
Git-->>Review: base_rev, head_rev
alt CheckoutOption.MERGE
Review->>Git: checkout worktree at merge_rev
else
Review->>Git: checkout worktree at head_rev
end
Review->>Builder: build_commit(base_rev, head_rev, merge_rev, staged)
alt only-packages path
Builder->>Git: checkout/merge per mode (COMMIT/MERGE)
else
Builder->>Git: merge base_rev with head/merge as needed
end
opt COMMIT mode with head_commit present
Builder->>Git: final checkout head_commit
end
Builder-->>Review: build results
Review-->>User: results
sequenceDiagram
autonumber
participant Review as Review.build_commit
participant Git as Git Repo
Note over Review,Git: Worktree control flow
alt only_packages
alt COMMIT and head_commit
Review->>Git: checkout(head_commit)
else MERGE and merge_commit
Review->>Git: checkout(merge_commit)
else MERGE without merge_commit
Review->>Git: merge(head_commit)
else head_commit None (unstaged)
Review->>Git: apply unstaged changes
end
else full build
alt merge_commit provided
Review->>Git: merge base_commit..merge_commit
else
Review->>Git: merge base_commit..head_commit
end
end
opt COMMIT mode and head_commit present
Review->>Git: checkout(head_commit)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
thefossguy
approved these changes
Sep 13, 2025
thefossguy
left a comment
thefossguy
left a comment
There was a problem hiding this comment.
Verified that the PR fixes detection of changed packages (as per the comment just above with the output).
Defelo
force-pushed
the
push-plquszmtolwo
branch
from
7b680e1 to
558b6d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nixpkgs_review/review.py (3)
871-912: Make fetch independent of CWD, support “no depth” fetch, and improve error messages.
- Current calls assume the process CWD is the repository. Use
--git-dirto target the right repo.- Allow
shallow_depth=Noneto disable the depth flag (used by the fallback above).- Error mentions
{repo}when probing local shallow state; clarify.-def fetch_refs(repo: str, *refs: str, shallow_depth: int = 1) -> list[str]: - shallow = subprocess.run( - ["git", "rev-parse", "--is-shallow-repository"], +def fetch_refs(repo: str, *refs: str, shallow_depth: int | None = 1) -> list[str]: + dotgit = resolve_git_dir() + shallow = subprocess.run( + ["git", "--git-dir", str(dotgit), "rev-parse", "--is-shallow-repository"], text=True, stdout=subprocess.PIPE, check=False, ) if shallow.returncode != 0: - msg = f"Failed to detect if {repo} is shallow repository" + msg = "Failed to detect if local repository is shallow" raise NixpkgsReviewError(msg) - fetch_cmd = [ + fetch_cmd = [ "git", + "--git-dir", + str(dotgit), "-c", "fetch.prune=false", "fetch", "--no-tags", "--force", repo, ] - if shallow.stdout.strip() == "true": - fetch_cmd.append(f"--depth={shallow_depth}") + if shallow.stdout.strip() == "true" and shallow_depth is not None: + fetch_cmd.append(f"--depth={shallow_depth}") for i, ref in enumerate(refs): fetch_cmd.append(f"{ref}:refs/nixpkgs-review/{i}") - dotgit = resolve_git_dir() with locked_open(dotgit / "nixpkgs-review", "w"): - res = sh(fetch_cmd) + res = sh(fetch_cmd) if res.returncode != 0: msg = f"Failed to fetch {refs} from {repo}. git fetch failed with exit code {res.returncode}" raise NixpkgsReviewError(msg) shas = [] for i, ref in enumerate(refs): - rev_parse_cmd = ["git", "rev-parse", "--verify", f"refs/nixpkgs-review/{i}"] + rev_parse_cmd = ["git", "--git-dir", str(dotgit), "rev-parse", "--verify", f"refs/nixpkgs-review/{i}"] out = subprocess.run( rev_parse_cmd, text=True, stdout=subprocess.PIPE, check=False ) if out.returncode != 0: msg = f"Failed to fetch {ref} from {repo} with command: {''.join(rev_parse_cmd)}" raise NixpkgsReviewError(msg) shas.append(out.stdout.strip()) return shas
283-289: Delta preview receives no stdin; nothing gets rendered.Pipe the prepared diff to delta via
input=limited_diff.- subprocess.run( + subprocess.run( [delta_cmd, "--side-by-side", "--line-numbers", "--paging=never"], - stdin=subprocess.PIPE, + stdin=subprocess.PIPE, stderr=subprocess.PIPE, text=True, - check=True, + input=limited_diff, + check=True, )
319-324: URLError is referenced but urllib.error isn’t imported.This can raise during exception handling. Import the module.
- try: + try: with urllib.request.urlopen(diff_url) as response: # noqa: S310 diff_content = response.read().decode("utf-8") self._display_diff_preview(diff_content) - except (urllib.error.URLError, OSError): + except (urllib.error.URLError, OSError): passAdd import near other imports:
-import urllib.request +import urllib.request +import urllib.error
🧹 Nitpick comments (2)
tests/test_pr.py (2)
71-74: Prefer BytesIO over mock_open for urlopen responses.
urllib.request.urlopen(...).read()returns bytes.mock_openis text-oriented and can behave inconsistently with bytes. Useio.BytesIOfor robustness.- mocks = [ - mock_open(read_data=json.dumps(pr_response).encode())(), - mock_open(read_data=diff_content.encode())(), - ] + mocks = [ + io.BytesIO(json.dumps(pr_response).encode("utf-8")), + io.BytesIO(diff_content.encode("utf-8")), + ]
89-96: Minor: use the repo test helper or absolute git path to silence S607 (partial executable path).Low impact, but you can reuse
conftest.run(...)for consistency.- subprocess.run(["git", "checkout", "-b", "pull/1/head"], check=True) + run(["git", "checkout", "-b", "pull/1/head"]) - subprocess.run(["git", "add", "."], check=True) + run(["git", "add", "."]) - subprocess.run(["git", "commit", "-m", "example-change"], check=True) + run(["git", "commit", "-m", "example-change"]) - subprocess.run(["git", "checkout", "-b", "pull/1/merge", "master"], check=True) + run(["git", "checkout", "-b", "pull/1/merge", "master"]) - subprocess.run(["git", "merge", "--no-ff", "pull/1/head"], check=True) + run(["git", "merge", "--no-ff", "pull/1/head"]) - subprocess.run(["git", "push", str(nixpkgs.remote), "pull/1/merge"], check=True) + run(["git", "push", str(nixpkgs.remote), "pull/1/merge"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nixpkgs_review/review.py(9 hunks)tests/test_pr.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_pr.py (1)
tests/conftest.py (6)
Helpers(163-241)Nixpkgs(22-24)nixpkgs(192-241)run(27-28)helpers(245-246)read_asset(169-170)
nixpkgs_review/review.py (1)
nixpkgs_review/git.py (1)
verify_commit_hash(34-41)
🪛 Ruff (0.12.2)
tests/test_pr.py
89-89: Starting a process with a partial executable path
(S607)
91-91: Starting a process with a partial executable path
(S607)
92-92: Starting a process with a partial executable path
(S607)
93-93: Starting a process with a partial executable path
(S607)
94-94: Starting a process with a partial executable path
(S607)
95-95: subprocess call: check for execution of untrusted input
(S603)
95-95: Starting a process with a partial executable path
(S607)
🔇 Additional comments (2)
nixpkgs_review/review.py (1)
437-439: Post-diff checkout in COMMIT mode looks good.Ensures builds run against PR head when requested.
tests/test_pr.py (1)
88-101: Good test helper to create a realistic base/head/merge triple.This centralizes repo setup and makes tests deterministic.
Comment on lines
+405
to
411
| if head_commit is None: | ||
| self.apply_unstaged(staged) | ||
| elif self.checkout == CheckoutOption.COMMIT: | ||
| self.git_checkout(reviewed_commit) | ||
| elif merge_commit: | ||
| self.git_checkout(merge_commit) | ||
| else: | ||
| self.git_merge(reviewed_commit) | ||
| self.git_merge(head_commit) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
CheckoutOption.COMMIT is ignored in local-eval diffs when a merge_commit exists (mis-detection risk).
In the non-only-packages path, you always prefer the merge commit if provided. This diverges from COMMIT semantics and makes the diff include base-branch changes, which can overbuild. Prefer merging head_commit into base_commit when checkout == COMMIT, even if merge_commit is available.
- if head_commit is None:
+ if head_commit is None:
self.apply_unstaged(staged)
- elif merge_commit:
- self.git_checkout(merge_commit)
- else:
- self.git_merge(head_commit)
+ elif self.checkout == CheckoutOption.COMMIT:
+ # Evaluate diffs against base+PR only (ignore target branch head)
+ self.git_merge(head_commit)
+ elif merge_commit:
+ # Evaluate diffs against the actual GitHub merge result
+ self.git_checkout(merge_commit)
+ else:
+ self.git_merge(head_commit)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if head_commit is None: | |
| self.apply_unstaged(staged) | |
| elif self.checkout == CheckoutOption.COMMIT: | |
| self.git_checkout(reviewed_commit) | |
| elif merge_commit: | |
| self.git_checkout(merge_commit) | |
| else: | |
| self.git_merge(reviewed_commit) | |
| self.git_merge(head_commit) | |
| if head_commit is None: | |
| self.apply_unstaged(staged) | |
| elif self.checkout == CheckoutOption.COMMIT: | |
| # Evaluate diffs against base+PR only (ignore target branch head) | |
| self.git_merge(head_commit) | |
| elif merge_commit: | |
| # Evaluate diffs against the actual GitHub merge result | |
| self.git_checkout(merge_commit) | |
| else: | |
| self.git_merge(head_commit) |
🤖 Prompt for AI Agents
In nixpkgs_review/review.py around lines 405-411, the current logic prefers
using merge_commit if present which ignores CheckoutOption.COMMIT semantics and
pulls base-branch changes into local-eval diffs; change the branch-selection
logic so that when checkout == CheckoutOption.COMMIT you explicitly check out
the base commit and then merge head_commit into it (i.e.,
git_checkout(base_commit) followed by git_merge(head_commit)), only falling back
to using merge_commit when checkout is not COMMIT and merge_commit is the
intended behavior; keep the existing apply_unstaged(head_commit is None)
behavior for unstaged cases.
Comment on lines
+513
to
516
| [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=2) | ||
| base_rev = git.verify_commit_hash(f"{merge_rev}^1") | ||
| head_rev = git.verify_commit_hash(f"{merge_rev}^2") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fetching only the merge commit with depth=2 can miss parent(2) in shallow repos. Add fallback or fetch parents robustly.
rev-parse <merge>^2 can fail if the second parent isn't present. Add a fallback unshallow fetch (or disable depth) when parent resolution fails.
- [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=2)
- base_rev = git.verify_commit_hash(f"{merge_rev}^1")
- head_rev = git.verify_commit_hash(f"{merge_rev}^2")
+ try:
+ [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=2)
+ base_rev = git.verify_commit_hash(f"{merge_rev}^1")
+ head_rev = git.verify_commit_hash(f"{merge_rev}^2")
+ except NixpkgsReviewError:
+ # Fallback: fetch without depth-limit to ensure both parents are available
+ [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=None)
+ base_rev = git.verify_commit_hash(f"{merge_rev}^1")
+ head_rev = git.verify_commit_hash(f"{merge_rev}^2")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=2) | |
| base_rev = git.verify_commit_hash(f"{merge_rev}^1") | |
| head_rev = git.verify_commit_hash(f"{merge_rev}^2") | |
| try: | |
| [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=2) | |
| base_rev = git.verify_commit_hash(f"{merge_rev}^1") | |
| head_rev = git.verify_commit_hash(f"{merge_rev}^2") | |
| except NixpkgsReviewError: | |
| # Fallback: fetch without depth-limit to ensure both parents are available | |
| [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=None) | |
| base_rev = git.verify_commit_hash(f"{merge_rev}^1") | |
| head_rev = git.verify_commit_hash(f"{merge_rev}^2") |
🤖 Prompt for AI Agents
In nixpkgs_review/review.py around lines 513 to 516, resolving the merge commit
parents with shallow_depth=2 can fail because the second parent may not be
present in shallow clones; wrap the parent resolution in a try/except and on
failure perform a corrective fetch (e.g., fetch the missing parents or run an
unshallow/fetch without depth, or explicitly fetch merge_commit^2) then retry
git.verify_commit_hash for f"{merge_rev}^1" and f"{merge_rev}^2"; ensure the
fallback only runs when parent resolution fails and preserve existing behavior
otherwise.
thefossguy
approved these changes
Sep 13, 2025
thefossguy
left a comment
thefossguy
left a comment
There was a problem hiding this comment.
No changes in 558b6d4 -> 7b680e1.
GaetanLepage
approved these changes
Sep 13, 2025
Collaborator
GaetanLepage
left a comment
GaetanLepage
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix!
Collaborator
|
@Mic92 do you think we should make a (minor) release to ship this fix? |
Owner
|
@GaetanLepage go ahead. |
Collaborator
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #563
cc @GaetanLepage @thefossguy
Summary by CodeRabbit