Fix purl namespace corruption when path contains ".git" mid-path#1268
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 12 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughFour independent bug fixes for dfetch 0.14.1: Changesdfetch 0.14.1 Bug Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/manifest/version.py`:
- Around line 22-25: The `__hash__` method does not match the semantics of the
`__eq__` method in the Version class. The `__eq__` method prioritizes tag
comparison—if either version has a tag, only tags are compared, ignoring branch
and revision. However, `__hash__` currently hashes all three fields
unconditionally. This violates Python's hash/equality contract where equal
objects must have equal hashes. Update the `__hash__` method to hash only the
fields that determine equality: if the tag exists, hash only the tag; otherwise,
hash the tuple of branch and revision. This ensures two versions that are equal
under `__eq__` will always produce the same hash value.
In `@tests/test_project_version.py`:
- Around line 220-223: The test_version_remains_hashable function only verifies
that a single Version instance can be in a set, but doesn't validate the
critical __eq__/__hash__ contract for equal-but-distinct instances. Strengthen
the test by creating two separate Version objects with identical tag values
(both "1.0"), asserting they compare as equal, verifying they have the same hash
value, and confirming that a set containing one Version instance will correctly
identify the other equal Version instance as a member (using the 'in' operator).
This will catch violations of Python's rule that equal objects must have equal
hashes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 541af782-9a2f-47f8-ad81-cb804f1f2704
📒 Files selected for processing (9)
CHANGELOG.rstdfetch/manifest/manifest.pydfetch/manifest/version.pydfetch/util/purl.pydfetch/vcs/git.pytests/test_add.pytests/test_git_vcs.pytests/test_project_version.pytests/test_purl.py
spoorcc
force-pushed
the
claude/dfetch-bug-fixes-A5KXW
branch
from
91b2a19 to
29590cf
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
The generic VCS branch in _vcs_namespace_and_name used str.replace to strip a ".git" suffix from the path, which silently removed every occurrence anywhere in the path. A URL like https://gitlab.com/group/foo.github/project.git produced the namespace "group/foohub" instead of "group/foo.github". Use removesuffix so only the trailing .git is stripped. https://claude.ai/code/session_01KKvrvnVvsBChohuxbRRmzA
The fallthrough guard only handled falsy operands, so comparing a Version to any truthy non-Version (a plain tuple with matching arity, a string, an int) raised AttributeError when trying to read other.tag. Replace the guard with an isinstance check returning False — Version equality is domain-specific (tag has precedence over branch/revision) and should not silently fall back to tuple equality. Add an explicit __hash__ to keep the class hashable under pyright's static rules. https://claude.ai/code/session_01KKvrvnVvsBChohuxbRRmzA
target.startswith(remote_base) matched any URL whose host+path happened to share a leading string with the remote, regardless of path component boundaries. A remote base https://github.com/myorg therefore matched an unrelated URL https://github.com/myorg-private/repo, and dfetch add would attach the wrong remote (and compute a bogus repo-path). Require either an exact match or a "/" path boundary. https://claude.ai/code/session_01KKvrvnVvsBChohuxbRRmzA
sha.startswith(rev) matched the first reference unconditionally when rev was an empty string, returning a bogus branch/tag tuple for any caller that passed in an uninitialised SHA. Return early when rev is empty. https://claude.ai/code/session_01KKvrvnVvsBChohuxbRRmzA
git remote get-url emits the URL with a trailing newline. The decoded result was returned verbatim, leaving the newline embedded in the URL string used by submodule URL resolution and downstream string operations. https://claude.ai/code/session_01KKvrvnVvsBChohuxbRRmzA
spoorcc
force-pushed
the
claude/dfetch-bug-fixes-A5KXW
branch
from
29590cf to
ba1ff56
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.rst`:
- Line 6: The CHANGELOG.rst file contains duplicate entries describing the same
fix for the dfetch import namespace mangling issue (issue `#1268`) related to
generic VCS URLs containing .git in the path. Remove the duplicate entry on line
6 and keep only the clearer version from line 8 which explicitly specifies that
.git should be removed only as a suffix, consolidating them into a single
changelog entry.
In `@tests/test_add.py`:
- Around line 154-171: The test functions
test_determine_remote_requires_path_boundary and
test_determine_remote_matches_exact_and_subpath are testing
Manifest.find_remote_for_url but are currently in tests/test_add.py. Move these
two test functions to tests/test_manifest.py to follow the repository's test
layout convention where test files should mirror the module names they test.
Ensure any necessary imports (Mock, _make_remote, Manifest) are available in the
target test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c14579c8-ceac-482f-bccc-231887e70733
📒 Files selected for processing (9)
CHANGELOG.rstdfetch/manifest/manifest.pydfetch/manifest/version.pydfetch/util/purl.pydfetch/vcs/git.pytests/test_add.pytests/test_git_vcs.pytests/test_project_version.pytests/test_purl.py
2 participants
The generic VCS branch in _vcs_namespace_and_name used str.replace to
strip a ".git" suffix from the path, which silently removed every
occurrence anywhere in the path. A URL like
https://gitlab.com/group/foo.github/project.git produced the namespace
"group/foohub" instead of "group/foo.github". Use removesuffix so only
the trailing .git is stripped.
https://claude.ai/code/session_01KKvrvnVvsBChohuxbRRmzA
Summary by CodeRabbit
Release Notes
Bug Fixes
Versionequality when comparing to non-Versionvalues, and madeVersionhashable consistently with equality..gitsuffix.Tests
Versioncomparisons/hash behavior, and Git URL/SHA edge cases.