feat: add per-project GitLab hosting support - #57
Closed
AdamKaabyia wants to merge 1 commit into
Closed
AdamKaabyia wants to merge 1 commit into
AdamKaabyia wants to merge 1 commit into
Conversation
Enable GitLab as a VCS backend alongside GitHub, selectable per workspace via the `hosting: gitlab` configuration field. A hosting router dispatches operations to the correct provider based on owner/repo mapping. Key changes: - Extract provider-agnostic git operations into shared GitOps helper - Add GitLabServiceImpl (PAT auth, MR creation, pipeline status, notes) - Add hosting router that satisfies all consumer interfaces - Make GitHub validation conditional (only when workspaces use GitHub) - Add GitLab config struct with environment variable bindings - Add comprehensive tests for GitLab service, router, and shared GitOps - Add docs/gitlab-support.md deployment and usage guide Design: - GitLab commits via local git push (vs GitHub's Git Data API) - PAT/Project/Group Access Token authentication - Workspace-level hosting field with fallback to GitHub (backward compat) - Zero changes to existing GitHub-only deployments
WalkthroughAdds GitLab configuration and hosting support, introduces shared Git operations, implements GitLab repository and merge-request APIs, and routes GitHub or GitLab workspaces through a provider-aware router. ChangesGitLab hosting support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (10 passed)
✨ 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: 10
🤖 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 `@docs/gitlab-support.md`:
- Line 123: Update the GitLab prerequisite token scope list to include api,
read_repository, and write_repository, matching the requirements documented
elsewhere. Modify only the GitLab entry in the prerequisites section.
In `@main.go`:
- Around line 536-546: Consolidate extractOwnerRepoFromURL,
hosting.extractOwnerRepo, and extractGitLabRepoInfo into one shared URL parser
with consistent owner/repository fallback behavior. Update each caller,
including the flow using extractOwnerRepoFromURL, to reuse the shared parser and
remove the duplicate implementations, preserving the expected routing keys
across providers.
- Around line 50-71: The application still derives the bot identity from
config.GitHub, which breaks GitLab-only configurations. Resolve BotUsername from
the active hosting provider configuration after RequiredHostingProviders and
reuse that shared value when wiring the container manager, executor, recovery
runner, and scanners; preserve the existing GitHub behavior while selecting
GitLab’s configured username for GitLab-only setups.
In `@services/gitlab_test.go`:
- Around line 299-318: Add coverage for RestoreRemoteAuth with a unit test that
captures the URL supplied to SetRemoteURL and asserts the exact
https://oauth2:<token>@<host>/owner/repo.git format, including the happy path
and relevant error/edge cases required by the path instructions. In
TestGitLabCloneRepository, check os.MkdirAll’s error and call t.Fatalf instead
of ignoring it.
In `@services/gitlab.go`:
- Around line 374-443: Add a status-code check in GetPRComments immediately
after doRequest and before decoding resp.Body; for non-2xx responses, close the
body and return an error consistent with sibling methods, while preserving the
existing JSON decoding and pagination behavior for successful responses.
- Around line 84-94: Correct RestoreRemoteAuth so authURL uses a valid URL
authority: preserve the configured http/https scheme, place oauth2 and the
access token before the GitLab host, and append only the owner/repo.git path
without duplicating baseURL. Remove the no-op strings.Replace logic and ensure
both http:// and https:// BaseURL values are handled.
- Around line 107-147: URL-escape branch names before constructing the GitLab
branch API path. In both RemoteBranchExists and DeleteRemoteBranch, apply
url.PathEscape(branch) when interpolating the branch segment so names containing
slashes remain a single path component; preserve the existing status handling.
In `@services/hosting/router_test.go`:
- Around line 150-259: Add a test alongside TestRouter_RoutesToCorrectProvider
that invokes UpdateIssueComment with an owner/repo mapped to a non-default
provider, then assert that provider receives UpdateIssueComment and the default
provider receives no call. Include the required issue-comment arguments and
verify routing uses the owner/repo mapping rather than fallback.
In `@services/hosting/router.go`:
- Around line 279-302: The parse-failure path in extractOwnerRepo currently
returns a fabricated repository name. Change its final return to empty owner and
repository strings, matching extractOwnerRepoFromURL in main.go, and leave
callers to apply any explicit fallback behavior.
- Around line 232-235: Update Router.UpdateIssueComment to dispatch through
r.forRepo(owner, repo) before invoking UpdateIssueComment, so the repository’s
configured provider handles the request. Remove the stale fallback-only comment
and preserve the existing method arguments and error propagation.
🪄 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: Enterprise
Run ID: 286f1022-26c9-42c3-92cf-3c27b1cf9686
📒 Files selected for processing (12)
AGENTS.mdconfig.example.yamldocs/gitlab-support.mdmain.gomodels/config.goservices/github.goservices/gitlab.goservices/gitlab_test.goservices/gitops.goservices/gitops_test.goservices/hosting/router.goservices/hosting/router_test.go
| ### Prerequisites | ||
|
|
||
| - **Jira**: API token with read/write access to your project(s) | ||
| - **GitLab**: Personal Access Token with `api` + `write_repository` scopes |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Minor: scope list inconsistency. Line 54 requires api, read_repository, write_repository, but the Prerequisites here list only api + write_repository. Align them to avoid users creating under-scoped tokens.
🤖 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 `@docs/gitlab-support.md` at line 123, Update the GitLab prerequisite token
scope list to include api, read_repository, and write_repository, matching the
requirements documented elsewhere. Modify only the GitLab entry in the
prerequisites section.
Source: Linters/SAST tools
Comment on lines
+50
to
+71
|
|
||
| // Build the VCS hosting router from per-project workspace config. | ||
| needsGitHub, needsGitLab := config.RequiredHostingProviders() | ||
|
|
||
| var ghService *services.GitHubServiceImpl | ||
| if needsGitHub { | ||
| ghService = services.NewGitHubService(config, logger) | ||
| } | ||
|
|
||
| var glService *services.GitLabServiceImpl | ||
| if needsGitLab { | ||
| glService = services.NewGitLabService(config, logger) | ||
| } | ||
|
|
||
| repoProviders := buildRepoProviderMap(config, ghService, glService) | ||
| var fallback hosting.Provider | ||
| if ghService != nil { | ||
| fallback = ghService | ||
| } else { | ||
| fallback = glService | ||
| } | ||
| gitService := hosting.NewRouter(repoProviders, fallback) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm branch/prefix/identity all source from GitHub.BotUsername only.
rg -nP 'config\.GitHub\.BotUsername' main.go
rg -nP 'BotUsername' -C2 executor/ recovery/ scanner/ | rg -n 'BotUsername'Repository: flightctl/jira-ai-issue-solver
Length of output: 6665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- main.go relevant slices ---'
sed -n '1,330p' main.go
echo '--- config search ---'
rg -n 'RequiredHostingProviders|BotUsername|GitLab\.BotUsername|GitHub\.BotUsername' -S . --glob '!**/*test.go'Repository: flightctl/jira-ai-issue-solver
Length of output: 15724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '823,1320p' models/config.goRepository: flightctl/jira-ai-issue-solver
Length of output: 16430
Major: route BotUsername from the active hosting config, not config.GitHub. main.go still injects config.GitHub.BotUsername into the container manager, executor, recovery runner, and scanners. GitLab-only configs validate gitlab.bot_username, so this leaves those paths with an empty username and breaks startup/branch naming/comment filtering. Use a shared resolved bot identity before wiring these components.
🤖 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 `@main.go` around lines 50 - 71, The application still derives the bot identity
from config.GitHub, which breaks GitLab-only configurations. Resolve BotUsername
from the active hosting provider configuration after RequiredHostingProviders
and reuse that shared value when wiring the container manager, executor,
recovery runner, and scanners; preserve the existing GitHub behavior while
selecting GitLab’s configured username for GitLab-only setups.
Comment on lines
+536
to
+546
| // extractOwnerRepoFromURL parses owner and repo from a clone URL. | ||
| func extractOwnerRepoFromURL(repoURL string) (string, string) { | ||
| repoURL = strings.TrimSuffix(repoURL, ".git") | ||
| parts := strings.Split(repoURL, "/") | ||
| if len(parts) >= 5 { | ||
| repo := parts[len(parts)-1] | ||
| owner := strings.Join(parts[3:len(parts)-1], "/") | ||
| return owner, repo | ||
| } | ||
| return "", "" | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Recommended: consolidate the three URL parsers. extractOwnerRepoFromURL here duplicates hosting.extractOwnerRepo (services/hosting/router.go) and extractGitLabRepoInfo (services/gitlab.go), with divergent fallbacks (this one returns "",""; the router returns "", "unknown-<url>"). Divergence in owner/repo keys will cause silent routing mismatches. Extract one shared parser and reuse it.
🤖 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 `@main.go` around lines 536 - 546, Consolidate extractOwnerRepoFromURL,
hosting.extractOwnerRepo, and extractGitLabRepoInfo into one shared URL parser
with consistent owner/repository fallback behavior. Update each caller,
including the flow using extractOwnerRepoFromURL, to reuse the shared parser and
remove the duplicate implementations, preserving the expected routing keys
across providers.
Comment on lines
+299
to
+318
| func TestGitLabCloneRepository(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| repoDir := filepath.Join(tmpDir, "repo") | ||
|
|
||
| // Create a fake git repo. | ||
| os.MkdirAll(filepath.Join(repoDir, ".git"), 0750) | ||
|
|
||
| fakeExec := func(name string, args ...string) *exec.Cmd { | ||
| return exec.Command("true") | ||
| } | ||
|
|
||
| config := newTestGitLabConfig("https://gitlab.example.com") | ||
| logger := zap.NewNop() | ||
| svc := NewGitLabService(config, logger, fakeExec) | ||
|
|
||
| err := svc.CloneRepository("https://gitlab.example.com/org/repo.git", repoDir) | ||
| if err != nil { | ||
| t.Fatalf("CloneRepository failed: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for RestoreRemoteAuth. TestGitLabCloneRepository stubs the executor with true, so the malformed auth URL flagged in services/gitlab.go is never asserted. Add a unit test that captures the URL passed to SetRemoteURL and asserts it equals https://oauth2:<token>@<host>/owner/repo.git. Also prefer t.Fatalf on the os.MkdirAll at Line 304 rather than ignoring its error.
As per path instructions: "Every code change must include corresponding unit tests covering new functions' happy paths, errors, and edge cases".
🤖 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 `@services/gitlab_test.go` around lines 299 - 318, Add coverage for
RestoreRemoteAuth with a unit test that captures the URL supplied to
SetRemoteURL and asserts the exact https://oauth2:<token>@<host>/owner/repo.git
format, including the happy path and relevant error/edge cases required by the
path instructions. In TestGitLabCloneRepository, check os.MkdirAll’s error and
call t.Fatalf instead of ignoring it.
Source: Path instructions
Comment on lines
+84
to
+94
| func (s *GitLabServiceImpl) RestoreRemoteAuth(directory, owner, repo string) error { | ||
| token := s.config.GitLab.AccessToken | ||
| baseURL := strings.TrimSuffix(s.config.GitLab.BaseURL, "/") | ||
| authURL := fmt.Sprintf("%s/oauth2:%s@%s/%s/%s.git", | ||
| strings.Replace(baseURL, "https://", "https://", 1), | ||
| token, | ||
| strings.TrimPrefix(strings.TrimPrefix(baseURL, "https://"), "http://"), | ||
| owner, repo) | ||
|
|
||
| return s.gitOps.SetRemoteURL(directory, authURL) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Critical: RestoreRemoteAuth builds a malformed remote URL. With baseURL = "https://gitlab.com", the format string yields https://gitlab.com/oauth2:<token>@gitlab.com/owner/repo.git`` — the base URL is duplicated and credentials land in the path, not the authority. The subsequent git push in `CommitChanges` and the auth restore in `CloneRepository` will fail. Also `strings.Replace(baseURL, "https://", "https://", 1)` is a no-op and `http://` bases are not handled.
🐛 Proposed fix
- token := s.config.GitLab.AccessToken
- baseURL := strings.TrimSuffix(s.config.GitLab.BaseURL, "/")
- authURL := fmt.Sprintf("%s/oauth2:%s@%s/%s/%s.git",
- strings.Replace(baseURL, "https://", "https://", 1),
- token,
- strings.TrimPrefix(strings.TrimPrefix(baseURL, "https://"), "http://"),
- owner, repo)
-
- return s.gitOps.SetRemoteURL(directory, authURL)
+ token := s.config.GitLab.AccessToken
+ baseURL := strings.TrimSuffix(s.config.GitLab.BaseURL, "/")
+ scheme := "https"
+ if strings.HasPrefix(baseURL, "http://") {
+ scheme = "http"
+ }
+ host := strings.TrimPrefix(strings.TrimPrefix(baseURL, "https://"), "http://")
+ authURL := fmt.Sprintf("%s://oauth2:%s@%s/%s/%s.git", scheme, token, host, owner, repo)
+
+ return s.gitOps.SetRemoteURL(directory, authURL)📝 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
| func (s *GitLabServiceImpl) RestoreRemoteAuth(directory, owner, repo string) error { | |
| token := s.config.GitLab.AccessToken | |
| baseURL := strings.TrimSuffix(s.config.GitLab.BaseURL, "/") | |
| authURL := fmt.Sprintf("%s/oauth2:%s@%s/%s/%s.git", | |
| strings.Replace(baseURL, "https://", "https://", 1), | |
| token, | |
| strings.TrimPrefix(strings.TrimPrefix(baseURL, "https://"), "http://"), | |
| owner, repo) | |
| return s.gitOps.SetRemoteURL(directory, authURL) | |
| } | |
| func (s *GitLabServiceImpl) RestoreRemoteAuth(directory, owner, repo string) error { | |
| token := s.config.GitLab.AccessToken | |
| baseURL := strings.TrimSuffix(s.config.GitLab.BaseURL, "/") | |
| scheme := "https" | |
| if strings.HasPrefix(baseURL, "http://") { | |
| scheme = "http" | |
| } | |
| host := strings.TrimPrefix(strings.TrimPrefix(baseURL, "https://"), "http://") | |
| authURL := fmt.Sprintf("%s://oauth2:%s@%s/%s/%s.git", scheme, token, host, owner, repo) | |
| return s.gitOps.SetRemoteURL(directory, authURL) | |
| } |
🤖 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 `@services/gitlab.go` around lines 84 - 94, Correct RestoreRemoteAuth so
authURL uses a valid URL authority: preserve the configured http/https scheme,
place oauth2 and the access token before the GitLab host, and append only the
owner/repo.git path without duplicating baseURL. Remove the no-op
strings.Replace logic and ensure both http:// and https:// BaseURL values are
handled.
Comment on lines
+107
to
+147
| func (s *GitLabServiceImpl) RemoteBranchExists(owner, repo, branch string) (bool, error) { | ||
| projectID := s.projectPath(owner, repo) | ||
| url := fmt.Sprintf("%s/api/v4/projects/%s/repository/branches/%s", | ||
| s.config.GitLab.BaseURL, projectID, branch) | ||
|
|
||
| resp, err := s.doRequest("GET", url, nil) | ||
| if err != nil { | ||
| return false, fmt.Errorf("check branch existence: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| switch resp.StatusCode { | ||
| case http.StatusOK: | ||
| return true, nil | ||
| case http.StatusNotFound: | ||
| return false, nil | ||
| default: | ||
| return false, fmt.Errorf("unexpected status %d checking branch %s", resp.StatusCode, branch) | ||
| } | ||
| } | ||
|
|
||
| func (s *GitLabServiceImpl) DeleteRemoteBranch(owner, repo, branch string) error { | ||
| projectID := s.projectPath(owner, repo) | ||
| url := fmt.Sprintf("%s/api/v4/projects/%s/repository/branches/%s", | ||
| s.config.GitLab.BaseURL, projectID, branch) | ||
|
|
||
| resp, err := s.doRequest("DELETE", url, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("delete branch: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode == http.StatusNotFound { | ||
| return nil | ||
| } | ||
| if resp.StatusCode >= 300 { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| return fmt.Errorf("delete branch %s: status %d, body: %s", branch, resp.StatusCode, string(body)) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
GitLab REST API repository branches endpoint URL encode branch name slash
💡 Result:
Yes, when using the GitLab REST API to interact with repository branches, any branch name that contains a forward slash (/) must be URL-encoded [1][2]. In the URL path, the forward slash character must be replaced with its URL-encoded representation, which is %2F [1][2]. For example, if you are attempting to retrieve information for a branch named "my/branch" in a project, you should encode the slash as follows: GET /api/v4/projects/:id/repository/branches/my%2Fbranch [1][2] Failure to encode the slash will typically result in a 404 Not Found error from the API, as the system will interpret the unencoded slash as a path delimiter rather than part of the branch name [3]. This encoding requirement applies to all endpoints that include the branch name in the URL path, such as GET, DELETE, or other operations targeting a specific branch [4][5].
Citations:
- 1: https://docs.gitlab.com/17.11/api/rest/
- 2: https://docs.gitlab.com/17.9/api/rest/
- 3: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/100464
- 4: https://docs.gitlab.com/18.6/api/branches/
- 5: https://docs.gitlab.com/api/branches/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and surrounding helpers.
git ls-files services/gitlab.go
printf '\n--- outline ---\n'
ast-grep outline services/gitlab.go --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '1,220p' services/gitlab.go | cat -nRepository: flightctl/jira-ai-issue-solver
Length of output: 13225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files services/gitlab.go
printf '\n--- outline ---\n'
ast-grep outline services/gitlab.go --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '1,220p' services/gitlab.go | cat -nRepository: flightctl/jira-ai-issue-solver
Length of output: 13225
URL-escape branch names in GitLab branch API paths. Branches like {botUsername}/{ticketKey} include /, so interpolating branch raw into /repository/branches/%s turns the slash into a path separator. RemoteBranchExists will report existing branches as missing, and DeleteRemoteBranch will accept the resulting 404 as success. Use url.PathEscape(branch) in both calls.
🤖 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 `@services/gitlab.go` around lines 107 - 147, URL-escape branch names before
constructing the GitLab branch API path. In both RemoteBranchExists and
DeleteRemoteBranch, apply url.PathEscape(branch) when interpolating the branch
segment so names containing slashes remain a single path component; preserve the
existing status handling.
Comment on lines
+374
to
+443
| func (s *GitLabServiceImpl) GetPRComments(owner, repo string, number int, since time.Time) ([]models.PRComment, error) { | ||
| projectID := s.projectPath(owner, repo) | ||
| var allComments []models.PRComment | ||
| page := 1 | ||
|
|
||
| for { | ||
| url := fmt.Sprintf("%s/api/v4/projects/%s/merge_requests/%d/notes?per_page=100&page=%d&sort=asc", | ||
| s.config.GitLab.BaseURL, projectID, number, page) | ||
|
|
||
| resp, err := s.doRequest("GET", url, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("get MR notes: %w", err) | ||
| } | ||
|
|
||
| var notes []struct { | ||
| ID int64 `json:"id"` | ||
| Body string `json:"body"` | ||
| Author struct { | ||
| Username string `json:"username"` | ||
| } `json:"author"` | ||
| CreatedAt string `json:"created_at"` | ||
| System bool `json:"system"` | ||
| Resolvable bool `json:"resolvable"` | ||
| Position *struct { | ||
| NewPath string `json:"new_path"` | ||
| NewLine int `json:"new_line"` | ||
| } `json:"position"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(¬es); err != nil { | ||
| resp.Body.Close() | ||
| return nil, fmt.Errorf("decode MR notes: %w", err) | ||
| } | ||
| resp.Body.Close() | ||
|
|
||
| for _, note := range notes { | ||
| if note.System { | ||
| continue | ||
| } | ||
| ts, _ := time.Parse(time.RFC3339, note.CreatedAt) | ||
| if !since.IsZero() && ts.Before(since) { | ||
| continue | ||
| } | ||
|
|
||
| comment := models.PRComment{ | ||
| ID: note.ID, | ||
| Author: models.Author{Name: note.Author.Username}, | ||
| Body: note.Body, | ||
| URL: fmt.Sprintf("%s/api/v4/projects/%s/merge_requests/%d#note_%d", | ||
| s.config.GitLab.BaseURL, projectID, number, note.ID), | ||
| Timestamp: ts, | ||
| IsReviewComment: note.Resolvable, | ||
| } | ||
| if note.Position != nil { | ||
| comment.FilePath = note.Position.NewPath | ||
| comment.Line = note.Position.NewLine | ||
| } | ||
| allComments = append(allComments, comment) | ||
| } | ||
|
|
||
| if len(notes) < 100 { | ||
| break | ||
| } | ||
| page++ | ||
| } | ||
|
|
||
| if allComments == nil { | ||
| allComments = []models.PRComment{} | ||
| } | ||
| return allComments, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Minor: GetPRComments decodes the body without checking resp.StatusCode. Unlike the sibling methods, a non-2xx response (auth failure, rate limit) is decoded as an empty note list and silently returns zero comments, masking errors. Add a status-code guard before decoding.
🤖 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 `@services/gitlab.go` around lines 374 - 443, Add a status-code check in
GetPRComments immediately after doRequest and before decoding resp.Body; for
non-2xx responses, close the body and return an error consistent with sibling
methods, while preserving the existing JSON decoding and pagination behavior for
successful responses.
Comment on lines
+150
to
+259
| func TestRouter_RoutesToCorrectProvider(t *testing.T) { | ||
| github := &stubProvider{name: "github"} | ||
| gitlab := &stubProvider{name: "gitlab"} | ||
|
|
||
| repoProviders := map[string]Provider{ | ||
| "gitlab-org/repo": gitlab, | ||
| "github-org/repo": github, | ||
| } | ||
|
|
||
| router := NewRouter(repoProviders, github) | ||
|
|
||
| // API-based call should route to gitlab for gitlab-org/repo. | ||
| router.GetPRForBranch("gitlab-org", "repo", "feature") | ||
| if len(gitlab.calls) != 1 || gitlab.calls[0] != "GetPRForBranch" { | ||
| t.Errorf("expected gitlab to receive GetPRForBranch, got %v", gitlab.calls) | ||
| } | ||
| if len(github.calls) != 0 { | ||
| t.Errorf("expected github to receive no calls, got %v", github.calls) | ||
| } | ||
|
|
||
| // API-based call should route to github for github-org/repo. | ||
| router.GetPRForBranch("github-org", "repo", "feature") | ||
| if len(github.calls) != 1 || github.calls[0] != "GetPRForBranch" { | ||
| t.Errorf("expected github to receive GetPRForBranch, got %v", github.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestRouter_FallbackToDefault(t *testing.T) { | ||
| github := &stubProvider{name: "github"} | ||
| gitlab := &stubProvider{name: "gitlab"} | ||
|
|
||
| repoProviders := map[string]Provider{ | ||
| "gitlab-org/repo": gitlab, | ||
| } | ||
|
|
||
| router := NewRouter(repoProviders, github) | ||
|
|
||
| // Unknown repo should fall back to the default (github). | ||
| router.GetPRForBranch("unknown-org", "unknown-repo", "feature") | ||
| if len(github.calls) != 1 || github.calls[0] != "GetPRForBranch" { | ||
| t.Errorf("expected github (fallback) to receive call, got %v", github.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestRouter_DirBasedMethodsUseWorkspaceRegistry(t *testing.T) { | ||
| github := &stubProvider{name: "github"} | ||
| gitlab := &stubProvider{name: "gitlab"} | ||
|
|
||
| repoProviders := map[string]Provider{ | ||
| "gitlab-org/repo": gitlab, | ||
| } | ||
|
|
||
| router := NewRouter(repoProviders, github) | ||
|
|
||
| // Register a workspace directory with the gitlab provider. | ||
| router.RegisterWorkspace("/tmp/workspaces/ticket-1/repo", "gitlab-org", "repo") | ||
|
|
||
| // Dir-based call should route to gitlab. | ||
| router.CreateBranch("/tmp/workspaces/ticket-1/repo", "feature", "main") | ||
| if len(gitlab.calls) != 1 || gitlab.calls[0] != "CreateBranch" { | ||
| t.Errorf("expected gitlab to receive CreateBranch, got %v", gitlab.calls) | ||
| } | ||
|
|
||
| // Unknown dir should fall back. | ||
| router.CreateBranch("/tmp/workspaces/ticket-2/repo", "feature", "main") | ||
| if len(github.calls) != 1 || github.calls[0] != "CreateBranch" { | ||
| t.Errorf("expected github (fallback) for unknown dir, got %v", github.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestRouter_CloneRegistersWorkspace(t *testing.T) { | ||
| github := &stubProvider{name: "github"} | ||
| gitlab := &stubProvider{name: "gitlab"} | ||
|
|
||
| repoProviders := map[string]Provider{ | ||
| "gitlab-org/repo": gitlab, | ||
| } | ||
|
|
||
| router := NewRouter(repoProviders, github) | ||
|
|
||
| // Clone registers the workspace directory. | ||
| router.CloneRepository("https://gitlab.com/gitlab-org/repo.git", "/tmp/workspaces/ws1") | ||
|
|
||
| if len(gitlab.calls) != 1 || gitlab.calls[0] != "CloneRepository" { | ||
| t.Errorf("expected gitlab to receive CloneRepository, got %v", gitlab.calls) | ||
| } | ||
|
|
||
| // Subsequent dir-based call should route to gitlab. | ||
| router.HasChanges("/tmp/workspaces/ws1", "main") | ||
| if len(gitlab.calls) != 2 || gitlab.calls[1] != "HasChanges" { | ||
| t.Errorf("expected gitlab to receive HasChanges after clone, got %v", gitlab.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestRouter_CaseInsensitiveRouting(t *testing.T) { | ||
| github := &stubProvider{name: "github"} | ||
| gitlab := &stubProvider{name: "gitlab"} | ||
|
|
||
| repoProviders := map[string]Provider{ | ||
| "GitLab-Org/Repo": gitlab, | ||
| } | ||
|
|
||
| router := NewRouter(repoProviders, github) | ||
|
|
||
| // Lowercase lookup should still find the provider. | ||
| router.GetPRForBranch("gitlab-org", "repo", "feature") | ||
| if len(gitlab.calls) != 1 { | ||
| t.Errorf("expected case-insensitive match to gitlab, got %v", gitlab.calls) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a routing test for UpdateIssueComment. The suite covers GetPRForBranch and dir-based dispatch but not UpdateIssueComment, which is why the fallback-misroute bug in router.go went unnoticed. Add a case asserting it routes by owner/repo.
🤖 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 `@services/hosting/router_test.go` around lines 150 - 259, Add a test alongside
TestRouter_RoutesToCorrectProvider that invokes UpdateIssueComment with an
owner/repo mapped to a non-default provider, then assert that provider receives
UpdateIssueComment and the default provider receives no call. Include the
required issue-comment arguments and verify routing uses the owner/repo mapping
rather than fallback.
Comment on lines
+232
to
+235
| func (r *Router) UpdateIssueComment(owner, repo string, commentID int64, body string) error { | ||
| // commentID-based calls don't have owner/repo; fallback for now. | ||
| return r.fallback.UpdateIssueComment(owner, repo, commentID, body) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Major: UpdateIssueComment ignores its own owner/repo and always hits the fallback provider. The signature carries owner, repo, so in a mixed GitHub+GitLab deployment a GitLab comment update is dispatched to the GitHub fallback. The stale comment ("commentID-based calls don't have owner/repo") contradicts the actual signature. Route via r.forRepo(owner, repo).
🐛 Proposed fix
func (r *Router) UpdateIssueComment(owner, repo string, commentID int64, body string) error {
- // commentID-based calls don't have owner/repo; fallback for now.
- return r.fallback.UpdateIssueComment(owner, repo, commentID, body)
+ return r.forRepo(owner, repo).UpdateIssueComment(owner, repo, commentID, body)
}📝 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
| func (r *Router) UpdateIssueComment(owner, repo string, commentID int64, body string) error { | |
| // commentID-based calls don't have owner/repo; fallback for now. | |
| return r.fallback.UpdateIssueComment(owner, repo, commentID, body) | |
| } | |
| func (r *Router) UpdateIssueComment(owner, repo string, commentID int64, body string) error { | |
| return r.forRepo(owner, repo).UpdateIssueComment(owner, repo, commentID, body) | |
| } |
🤖 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 `@services/hosting/router.go` around lines 232 - 235, Update
Router.UpdateIssueComment to dispatch through r.forRepo(owner, repo) before
invoking UpdateIssueComment, so the repository’s configured provider handles the
request. Remove the stale fallback-only comment and preserve the existing method
arguments and error propagation.
Comment on lines
+279
to
+302
| func extractOwnerRepo(repoURL string) (string, string) { | ||
| repoURL = strings.TrimSuffix(repoURL, ".git") | ||
|
|
||
| // SSH format: git@host:owner/repo | ||
| if strings.Contains(repoURL, ":") && !strings.Contains(repoURL, "://") { | ||
| parts := strings.SplitN(repoURL, ":", 2) | ||
| if len(parts) == 2 { | ||
| pathParts := strings.Split(parts[1], "/") | ||
| if len(pathParts) >= 2 { | ||
| return strings.Join(pathParts[:len(pathParts)-1], "/"), pathParts[len(pathParts)-1] | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // HTTPS format: https://host/owner/[subgroups/]repo | ||
| parts := strings.Split(repoURL, "/") | ||
| if len(parts) >= 5 { | ||
| repo := parts[len(parts)-1] | ||
| owner := strings.Join(parts[3:len(parts)-1], "/") | ||
| return owner, repo | ||
| } | ||
|
|
||
| return "", fmt.Sprintf("unknown-%s", repoURL) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Minor: odd sentinel return on parse failure. On an unparseable URL, extractOwnerRepo returns ("", "unknown-<url>"), which then becomes the workspaceDirs/repoProviders key. This silently maps to a bogus repo instead of failing loudly, and diverges from main.go's extractOwnerRepoFromURL (which returns "",""). Return "","" and let callers fall back explicitly.
🤖 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 `@services/hosting/router.go` around lines 279 - 302, The parse-failure path in
extractOwnerRepo currently returns a fabricated repository name. Change its
final return to empty owner and repository strings, matching
extractOwnerRepoFromURL in main.go, and leave callers to apply any explicit
fallback behavior.
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.
Enable GitLab as a VCS backend alongside GitHub, selectable per workspace via the
hosting: gitlabconfiguration field. A hosting router dispatches operations to the correct provider based on owner/repo mapping.Key changes:
Design:
Affected packages
Execution pipeline
Updates hosting-dependent pipeline operations, including repository synchronization, local commits and pushes, PR/MR creation, comment and review handling, labels, CI status, and mergeability. GitHub retains its existing Git Data API workflow, while GitLab uses local
git pushand GitLab APIs.Infrastructure
No changes to container management or workspace lifecycle orchestration. Shared Git workspace operations are centralized in
GitOps, with routing tracked by repository and cloned workspace directory.Configuration and deployment
Adds optional GitLab configuration with environment variable support and workspace-level
hosting: gitlabselection, while preserving GitHub fallback behavior. Adds GitLab setup, deployment, usage, workflow, and behavior-difference documentation, plus example configuration and tests for GitLab services, routing, and Git operations.