Fix SSH shorthand URLs joined with '/' instead of ':' (#1247)#1248
Merged
Conversation
When `url-base` is an SSH SCP-style shorthand like `git@git.mycompany.com`, dfetch was joining it with `repo-path` using '/' (producing the invalid `git@git.mycompany.com/my-repo.git`) instead of ':' (the correct `git@git.mycompany.com:my-repo.git`). Add `_is_ssh_shorthand` helper to detect these URLs (has '@', no '://'), use ':' as separator in `remote_url`, and fix `set_remote` to strip both '/' and ':' separators when decomposing a full SSH URL against a remote base. https://claude.ai/code/session_01MhZACpoBQKKtSFVwjZcuo7
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughDetect SSH SCP-style shorthand remotes (e.g., git@host:path) and use ':' when joining ChangesSSH shorthand remote URL support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
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/project.py`:
- Around line 495-497: The concatenation in the method that builds the remote
path can emit a trailing ":" for SSH shorthand when self._repo_path is empty;
detect when self._repo_path is falsy and avoid leaving a terminal separator by
trimming a trailing ":" (for SSH) or "/" (for HTTPS) after composing base +
separator + self._repo_path, or alternatively skip adding the separator when
self._repo_path is empty; use the existing _is_ssh_shorthand(base) check and
self._remote_obj.url/self._repo_path to implement this defensive handling.
In `@tests/test_project_entry.py`:
- Around line 55-76: Tests currently cover core SSH vs HTTPS separators and
round-trip preservation but miss edge cases; add tests for trailing slash in
Remote({"url-base": ...}) and for empty repo-path behavior. Create new test
functions (e.g., test_remote_url_with_trailing_slash_in_base and
test_remote_url_with_empty_repo_path) that construct ProjectEntry and Remote,
call ProjectEntry.set_remote(Remote(...)), and assert ProjectEntry.remote_url
handles a base with a trailing slash (should not produce double slashes) and
handles an empty repo-path without adding an extra separator; reference
ProjectEntry.set_remote, Remote, remote_url and repo-path when locating where to
exercise the behavior.
🪄 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: 18c051cb-1a3e-4e8c-8f2b-1c125f1f58e9
📒 Files selected for processing (3)
CHANGELOG.rstdfetch/manifest/project.pytests/test_project_entry.py
When _repo_path is empty, the previous code emitted a trailing ':' for
SSH shorthand bases (strip("/") only removes slashes, not colons).
Return base directly when there is no repo-path to join.
Also add edge-case tests for trailing slash in url-base and empty repo-path.
https://claude.ai/code/session_01MhZACpoBQKKtSFVwjZcuo7
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/manifest/project.py (1)
357-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle SSH bases that already include a path segment.
_is_ssh_shorthand()also returnsTruefor valid bases likegit@host:org, soremote_urlnow recomposes them asgit@host:org:repo.git. That breaks the path-prefixed SSH remotes the import/matching logic already supports, which expectgit@host:org/repo.gitinstead. Please only use:for host-only shorthand bases and keep/once the shorthand already contains its host/path separator.Proposed fix
def _is_ssh_shorthand(url: str) -> bool: - """Return True if *url* is an SSH SCP-style shorthand (``git@host``, no scheme).""" - return "@" in url and "://" not in url + """Return True if *url* is a host-only SSH SCP-style shorthand (``git@host``).""" + if "@" not in url or "://" in url: + return False + return ":" not in url.rsplit("@", maxsplit=1)[1]if self._remote_obj: base = self._remote_obj.url.strip("/") if not self._repo_path: return base separator = ":" if _is_ssh_shorthand(base) else "/" return (base + separator + self._repo_path).strip("/")A regression test with
url-base="git@host:org"andrepo-path="repo.git"would lock this down.Also applies to: 495-499
🤖 Prompt for 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. In `@dfetch/manifest/project.py` around lines 357 - 359, The SSH-shorthand detection currently used by _is_ssh_shorthand() treats strings like "git@host:org" as shorthand and causes recomposition into "git@host:org:repo.git"; change _is_ssh_shorthand to only return True for host-only shorthands (e.g., "git@host") by requiring "@" present, "://" absent, and no ":" present in the URL so path-prefixed SSH bases (those already containing the host/path separator) are not treated as host-only; update any recomposition logic that relies on _is_ssh_shorthand (the remote_url construction around the current remote recomposition and the block referenced at the other occurrence) to preserve the existing "/" path separator when the base already contains ":"; add a regression test with url-base="git@host:org" and repo-path="repo.git".
🤖 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.
Outside diff comments:
In `@dfetch/manifest/project.py`:
- Around line 357-359: The SSH-shorthand detection currently used by
_is_ssh_shorthand() treats strings like "git@host:org" as shorthand and causes
recomposition into "git@host:org:repo.git"; change _is_ssh_shorthand to only
return True for host-only shorthands (e.g., "git@host") by requiring "@"
present, "://" absent, and no ":" present in the URL so path-prefixed SSH bases
(those already containing the host/path separator) are not treated as host-only;
update any recomposition logic that relies on _is_ssh_shorthand (the remote_url
construction around the current remote recomposition and the block referenced at
the other occurrence) to preserve the existing "/" path separator when the base
already contains ":"; add a regression test with url-base="git@host:org" and
repo-path="repo.git".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31af8c7c-e94f-413e-aa1d-b59cbceac106
📒 Files selected for processing (2)
dfetch/manifest/project.pytests/test_project_entry.py
A url-base such as 'git@host:org' already contains ':' as the SCP
separator between host and path; treating it as a host-only shorthand
caused remote_url to produce a double colon ('git@host:org:repo.git').
Add ':' not in url to _is_ssh_shorthand so only pure host shorthands
(e.g. 'git@host') get ':' as separator; bases that already carry a
path component continue to use '/'.
https://claude.ai/code/session_01MhZACpoBQKKtSFVwjZcuo7
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
When
url-baseis an SSH SCP-style shorthand likegit@git.mycompany.com,dfetch was joining it with
repo-pathusing '/' (producing the invalidgit@git.mycompany.com/my-repo.git) instead of ':' (the correctgit@git.mycompany.com:my-repo.git).Add
_is_ssh_shorthandhelper to detect these URLs (has '@', no '://'),use ':' as separator in
remote_url, and fixset_remoteto strip both'/' and ':' separators when decomposing a full SSH URL against a remote base.
https://claude.ai/code/session_01MhZACpoBQKKtSFVwjZcuo7
Summary by CodeRabbit