Fix/azure devops url parsing#2269
Open
Tankatronic wants to merge 3 commits intobmad-code-org:mainfrom
Open
Conversation
Added support for Azure DevOps URL parsing, including both modern and legacy formats, to extract relevant repository information.
This test file verifies the functionality of the CustomModuleManager's parseSource method for Azure DevOps Git URLs, including both dev.azure.com and legacy visualstudio.com formats. It includes various assertions to check the validity and expected outputs for different URL formats.
Add Azure DevOps URL parsing tests
📝 WalkthroughWalkthroughAdded test coverage for Azure DevOps Git URL parsing and extended Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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: 1
🧹 Nitpick comments (1)
test/test-azure-devops-url-parsing.js (1)
66-89: Add regression tests for?path=URLs and dotted repo namesPlease add Azure cases like
...?path=/foo/barand repo names such asmy.repofor both host formats. These are common inputs and should be locked in by tests.Also applies to: 95-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-azure-devops-url-parsing.js` around lines 66 - 89, Add regression tests in test/test-azure-devops-url-parsing.js that cover Azure URLs with ?path=/foo/bar query parameters and repository names containing dots (e.g., my.repo) for both Azure host formats exercised elsewhere in this file; for each new case call manager.parseSource(...) and assert result.isValid === true, assert result.cloneUrl is the repository clone URL with any .git suffix and any path/query stripped, and assert result.subdir === 'foo/bar' (or the corresponding path) to ensure query-path parsing and dotted repo names are handled correctly by parseSource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/installer/modules/custom-module-manager.js`:
- Around line 79-85: The ADO URL regex used for adoModernMatch and
adoLegacyMatch currently disallows dots in repo names and only captures
slash-based remainders; update the patterns used where adoModernMatch and
adoLegacyMatch are created so the repo capture uses a more permissive token
(allowing dots — e.g., replace `([^/.]+?)` with a pattern that permits dots) and
extend the final capture group to include query strings and `?path=` style
params (capture `(?:[/?].*)?` or equivalent) so links like `...?path=/src` are
accepted; also apply the same change to the later parsing logic referenced
around the block handling lines ~102-108 to ensure both slash remainder and
query-based remainders are handled consistently.
---
Nitpick comments:
In `@test/test-azure-devops-url-parsing.js`:
- Around line 66-89: Add regression tests in
test/test-azure-devops-url-parsing.js that cover Azure URLs with ?path=/foo/bar
query parameters and repository names containing dots (e.g., my.repo) for both
Azure host formats exercised elsewhere in this file; for each new case call
manager.parseSource(...) and assert result.isValid === true, assert
result.cloneUrl is the repository clone URL with any .git suffix and any
path/query stripped, and assert result.subdir === 'foo/bar' (or the
corresponding path) to ensure query-path parsing and dotted repo names are
handled correctly by parseSource.
🪄 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: CHILL
Plan: Pro
Run ID: d547ae65-b0ff-41dd-9689-6ae2e747520a
📒 Files selected for processing (2)
test/test-azure-devops-url-parsing.jstools/installer/modules/custom-module-manager.js
Comment on lines
+79
to
+85
| /^https?:\/\/(dev\.azure\.com)\/([^/]+)\/([^/]+)\/_git\/([^/.]+?)(?:\.git)?(\/.*)?$/, | ||
| ); | ||
| const adoLegacyMatch = | ||
| !adoModernMatch && | ||
| trimmed.match( | ||
| /^https?:\/\/([^/]+\.visualstudio\.com)\/([^/]+)\/_git\/([^/.]+?)(?:\.git)?(\/.*)?$/, | ||
| ); |
There was a problem hiding this comment.
Azure DevOps regex currently rejects valid repo URLs and misses ?path= subdirs
Line 79 and Line 84 use ([^/.]+?), which rejects repo names containing dots, and Line 102-108 only handles slash-based remainder, not common Azure DevOps query links like ...?path=/src. This can still fail valid user inputs.
Proposed fix
- const adoModernMatch = trimmed.match(
- /^https?:\/\/(dev\.azure\.com)\/([^/]+)\/([^/]+)\/_git\/([^/.]+?)(?:\.git)?(\/.*)?$/,
- );
+ const adoModernMatch = trimmed.match(
+ /^https?:\/\/(dev\.azure\.com)\/([^/]+)\/([^/]+)\/_git\/([^/]+?)(?:\.git)?(?:\/([^?#]*))?(?:\?([^#]*))?$/,
+ );
const adoLegacyMatch =
!adoModernMatch &&
trimmed.match(
- /^https?:\/\/([^/]+\.visualstudio\.com)\/([^/]+)\/_git\/([^/.]+?)(?:\.git)?(\/.*)?$/,
+ /^https?:\/\/([^/]+\.visualstudio\.com)\/([^/]+)\/_git\/([^/]+?)(?:\.git)?(?:\/([^?#]*))?(?:\?([^#]*))?$/,
);
@@
- let host, org, project, repo, remainder;
+ let host, org, project, repo, remainderPath, query;
if (adoModernMatch) {
- [, host, org, project, repo, remainder] = adoModernMatch;
+ [, host, org, project, repo, remainderPath, query] = adoModernMatch;
} else {
// Legacy: org is in the hostname, path is /{project}/_git/{repo}
- [, host, project, repo, remainder] = adoLegacyMatch;
+ [, host, project, repo, remainderPath, query] = adoLegacyMatch;
org = null;
}
@@
- if (remainder) {
- // Azure DevOps uses ?path=/subdir or /path/subdir patterns
- const subdirMatch = remainder.match(/^\/(.+)$/);
- if (subdirMatch) {
- subdir = subdirMatch[1].replace(/\/$/, '');
- }
+ if (remainderPath) {
+ subdir = remainderPath.replace(/\/$/, '');
+ } else if (query) {
+ const pathParam = new URLSearchParams(query).get('path');
+ if (pathParam) subdir = pathParam.replace(/^\/+|\/+$/g, '');
}Also applies to: 102-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/installer/modules/custom-module-manager.js` around lines 79 - 85, The
ADO URL regex used for adoModernMatch and adoLegacyMatch currently disallows
dots in repo names and only captures slash-based remainders; update the patterns
used where adoModernMatch and adoLegacyMatch are created so the repo capture
uses a more permissive token (allowing dots — e.g., replace `([^/.]+?)` with a
pattern that permits dots) and extend the final capture group to include query
strings and `?path=` style params (capture `(?:[/?].*)?` or equivalent) so links
like `...?path=/src` are accepted; also apply the same change to the later
parsing logic referenced around the block handling lines ~102-108 to ensure both
slash remainder and query-based remainders are handled consistently.
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
1 participant
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.
What
Added Azure DevOps URL support to CustomModuleManager.parseSource(), fixing clone failures for dev.azure.com and legacy visualstudio.com _git URLs.
Why
The HTTPS regex only handles 2-segment paths
owner/repo, but Azure DevOps usesorg/project/_git/repo. The_git/repoportion was silently discarded, causing git clone to target the wrong URL.Fixes #2268
How
Added dedicated Azure DevOps URL matching before the generic HTTPS regex in
parseSource(), handling both moderndev.azure.com/{org}/{project}/_git/{repo}and legacy{org}.visualstudio.com/{project}/_git/{repo}formatsCorrectly reconstructs
cloneUrl,cacheKey, anddisplayNamefor both URL structuresAdded
test/test-azure-devops-url-parsing.jswith 24 tests covering both formats, .git suffix stripping, subdir extraction, and regression checks for existing GitHub/GitLab/SSH parsingTesting
Ran node
test/test-azure-devops-url-parsing.js— 24 passed, 0 failed. Existing test-workflow-path-regex.js still passes (5/5).