fix/setup vscode command failing with url and repo key - #527
Conversation
…epo-key-for-ai-editor-extensions-repositories
📝 WalkthroughWalkthroughThe change updates server-detail resolution, supports same-host URL overrides with credential reuse, normalizes Artifactory base URLs, and validates normalized details in AI editor setup parsing. Tests cover configuration paths, URL formats, validation, and constants. ChangesServer configuration and URL flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ParseBaseSetupConfig
participant GetServerDetails
participant NormalizeArtifactoryBaseUrl
participant RepositoryValidation
ParseBaseSetupConfig->>GetServerDetails: resolve server details
GetServerDetails-->>ParseBaseSetupConfig: return server details
ParseBaseSetupConfig->>NormalizeArtifactoryBaseUrl: normalize URL and repository key
NormalizeArtifactoryBaseUrl-->>ParseBaseSetupConfig: return normalized URL
ParseBaseSetupConfig->>RepositoryValidation: validate normalized details
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ide/commands/aieditorextensions/base.go (1)
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the split comment sentence.
The second line starts mid-sentence with a stray space.
✏️ Proposed change
- // If --url was supplied, normalize it before we make any network call. - // and rejects obviously wrong shapes like a full /api/… URL. + // If --url was supplied, normalize it before we make any network call. + // Normalization strips a trailing repo-key segment and rejects wrong + // shapes such as a full /api/… URL.🤖 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 `@ide/commands/aieditorextensions/base.go` around lines 64 - 65, Fix the adjacent comments in the URL normalization section so they form one grammatically complete sentence, removing the stray leading space and sentence fragment on the second line.ide/common/base.go (1)
25-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not discard the config read error.
Line 27 merges a real failure from
config.GetDefaultServerConfwith the "no default server" case. If the JFrog config file is corrupt or unreadable, the user sees "no default server is configured" and loses the actual cause. Report the underlying error.Also consider updating
defaults.Urltogether withdefaults.ArtifactoryUrl, so that both fields stay consistent for any consumer that readsUrl.🔧 Proposed change
if c.IsFlagSet("url") { defaults, err := config.GetDefaultServerConf() - if err != nil || defaults == nil { + if err != nil { + return nil, fmt.Errorf("failed reading the configured servers: %w", err) + } + if defaults == nil { return nil, fmt.Errorf( "--url was provided but no default server is configured and no credentials were supplied. " + "Either run 'jf config add' to configure a server, or pass --access-token " + "(or --user and --password) alongside --url") }🤖 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 `@ide/common/base.go` around lines 25 - 43, Update the url-handling branch around config.GetDefaultServerConf to distinguish a non-nil read error from a nil defaults result, returning an error that preserves the underlying configuration failure instead of reporting only a missing default server. When overriding the configured host, update both defaults.ArtifactoryUrl and defaults.Url so consumers of either field see the same URL.ide/commands/aieditorextensions/base_test.go (1)
104-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the external network dependency from these two tests.
Both tests reach
common.ValidateRepository, which sends a real HTTP request toacme.jfrog.io. The tests therefore depend on DNS and on how that external host responds. They are slow in CI, and they fail if the host ever returns a success response. TheNotContainsassertions also pass for any unrelated failure, so they do not confirm that the flow reached validation.Point
ArtifactoryUrland--urlat anhttptest.Serverthat returns a controlled repository response. You can then assert the exact outcome, including the normalized base URL that reached the server.Also applies to: 152-172
🤖 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 `@ide/commands/aieditorextensions/base_test.go` around lines 104 - 125, Update TestParseBaseSetupConfig_UrlAlone_DefaultConfig_ReachesValidation and the corresponding test around ParseBaseSetupConfig to use an httptest.Server for both ArtifactoryUrl and --url instead of acme.jfrog.io. Configure the server to return a controlled repository response, record the request, and replace broad NotContains assertions with exact validation outcomes and an assertion that the normalized base URL reached the test server.
🤖 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 `@ide/common/base.go`:
- Around line 197-215: Update the Case B logic in NormalizeArtifactoryBaseUrl so
it strips a matching repoKey only when it is directly after the artifactory
anchor or when no anchor exists; otherwise let Case C reject deep paths. Add a
TestNormalizeArtifactoryBaseUrl table case for
https://acme.jfrog.io/artifactory/some-repo/my-repo with repoKey "my-repo" and
assert the validation error.
---
Nitpick comments:
In `@ide/commands/aieditorextensions/base_test.go`:
- Around line 104-125: Update
TestParseBaseSetupConfig_UrlAlone_DefaultConfig_ReachesValidation and the
corresponding test around ParseBaseSetupConfig to use an httptest.Server for
both ArtifactoryUrl and --url instead of acme.jfrog.io. Configure the server to
return a controlled repository response, record the request, and replace broad
NotContains assertions with exact validation outcomes and an assertion that the
normalized base URL reached the test server.
In `@ide/commands/aieditorextensions/base.go`:
- Around line 64-65: Fix the adjacent comments in the URL normalization section
so they form one grammatically complete sentence, removing the stray leading
space and sentence fragment on the second line.
In `@ide/common/base.go`:
- Around line 25-43: Update the url-handling branch around
config.GetDefaultServerConf to distinguish a non-nil read error from a nil
defaults result, returning an error that preserves the underlying configuration
failure instead of reporting only a missing default server. When overriding the
configured host, update both defaults.ArtifactoryUrl and defaults.Url so
consumers of either field see the same URL.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6be41a83-bba4-4a09-8a4d-e3819418d848
📒 Files selected for processing (4)
ide/commands/aieditorextensions/base.goide/commands/aieditorextensions/base_test.goide/common/base.goide/common/base_test.go
| // Case B: trailing segment matches --repo-key → strip it. | ||
| last := segments[len(segments)-1] | ||
| if repoKey != "" && strings.EqualFold(last, repoKey) { | ||
| return rebuildUrlWithoutLastSegment(u, rawUrl), nil | ||
| } | ||
|
|
||
| // Case C: extra segments after "/artifactory/" | ||
| if artIndex >= 0 && artIndex < len(segments)-1 { | ||
| extra := strings.Join(segments[artIndex+1:], "/") | ||
| hint := "" | ||
| if repoKey != "" { | ||
| hint = fmt.Sprintf(" (--repo-key is set to %q, which does not match)", repoKey) | ||
| } | ||
| return "", fmt.Errorf( | ||
| "--url %q includes a path segment after '/artifactory/' (%q)%s. "+ | ||
| "--url must be the Artifactory base URL (e.g. https://acme.jfrog.io/artifactory), "+ | ||
| "without a repository key. Move the repository name to --repo-key", | ||
| rawUrl, extra, hint) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Case B bypasses the Case C validation for deep paths.
Case B returns as soon as the last segment matches repoKey, so Case C never inspects the remaining path. For --url https://acme.jfrog.io/artifactory/some-repo/my-repo with --repo-key my-repo, the function returns https://acme.jfrog.io/artifactory/some-repo and reports no error. The caller then treats some-repo as part of the Artifactory base URL, so repository validation and the built service URL both target a wrong path. The equivalent input without the repo-key suffix (/artifactory/some-repo/nested/path) is correctly rejected.
Strip the trailing segment only when it sits directly after the artifactory anchor, or when no anchor exists.
🐛 Proposed fix
// Case B: trailing segment matches --repo-key → strip it.
last := segments[len(segments)-1]
- if repoKey != "" && strings.EqualFold(last, repoKey) {
+ // Only strip when the repo key is the single segment below the anchor, so
+ // deeper paths still fall through to the Case C rejection below.
+ if repoKey != "" && strings.EqualFold(last, repoKey) &&
+ (artIndex < 0 || artIndex == len(segments)-2) {
return rebuildUrlWithoutLastSegment(u, rawUrl), nil
}Please add a table case for https://acme.jfrog.io/artifactory/some-repo/my-repo with repoKey: "my-repo" in TestNormalizeArtifactoryBaseUrl.
📝 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.
| // Case B: trailing segment matches --repo-key → strip it. | |
| last := segments[len(segments)-1] | |
| if repoKey != "" && strings.EqualFold(last, repoKey) { | |
| return rebuildUrlWithoutLastSegment(u, rawUrl), nil | |
| } | |
| // Case C: extra segments after "/artifactory/" | |
| if artIndex >= 0 && artIndex < len(segments)-1 { | |
| extra := strings.Join(segments[artIndex+1:], "/") | |
| hint := "" | |
| if repoKey != "" { | |
| hint = fmt.Sprintf(" (--repo-key is set to %q, which does not match)", repoKey) | |
| } | |
| return "", fmt.Errorf( | |
| "--url %q includes a path segment after '/artifactory/' (%q)%s. "+ | |
| "--url must be the Artifactory base URL (e.g. https://acme.jfrog.io/artifactory), "+ | |
| "without a repository key. Move the repository name to --repo-key", | |
| rawUrl, extra, hint) | |
| } | |
| // Case B: trailing segment matches --repo-key → strip it. | |
| last := segments[len(segments)-1] | |
| // Only strip when the repo key is the single segment below the anchor, so | |
| // deeper paths still fall through to the Case C rejection below. | |
| if repoKey != "" && strings.EqualFold(last, repoKey) && | |
| (artIndex < 0 || artIndex == len(segments)-2) { | |
| return rebuildUrlWithoutLastSegment(u, rawUrl), nil | |
| } | |
| // Case C: extra segments after "/artifactory/" | |
| if artIndex >= 0 && artIndex < len(segments)-1 { | |
| extra := strings.Join(segments[artIndex+1:], "/") | |
| hint := "" | |
| if repoKey != "" { | |
| hint = fmt.Sprintf(" (--repo-key is set to %q, which does not match)", repoKey) | |
| } | |
| return "", fmt.Errorf( | |
| "--url %q includes a path segment after '/artifactory/' (%q)%s. "+ | |
| "--url must be the Artifactory base URL (e.g. https://acme.jfrog.io/artifactory), "+ | |
| "without a repository key. Move the repository name to --repo-key", | |
| rawUrl, extra, hint) | |
| } |
🤖 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 `@ide/common/base.go` around lines 197 - 215, Update the Case B logic in
NormalizeArtifactoryBaseUrl so it strips a matching repoKey only when it is
directly after the artifactory anchor or when no anchor exists; otherwise let
Case C reject deep paths. Add a TestNormalizeArtifactoryBaseUrl table case for
https://acme.jfrog.io/artifactory/some-repo/my-repo with repoKey "my-repo" and
assert the validation error.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests