diff --git a/README.md b/README.md index bda58e89..6968955c 100644 --- a/README.md +++ b/README.md @@ -101,8 +101,9 @@ retention conventions, see [docs/review-guidance.md](docs/review-guidance.md). ## Authentication And Setup -`cr` v1 supports GitHub personal access token authentication for Git-host -operations. The credential key for GitHub PATs is `git_token`. +`cr` supports GitHub (personal access token or GitHub App) and GitLab +(personal access token) authentication for Git-host operations. The credential +key for Git-host PATs is `git_token`. Quick setup with adapter-managed LLM credentials: @@ -256,6 +257,52 @@ read runtime tokens directly from arbitrary environment variables. Reviewer credentials use a separate credential name that also stores key `git_token`; keep it distinct from the user Git credential in the same store. +### GitLab + +GitLab merge requests (gitlab.com and self-managed instances) are reviewed +with the same commands as GitHub pull requests. A GitLab profile sets +`git.provider: gitlab` and authenticates with a personal access token holding +the `api` scope, stored under the same `git_token` credential key: + +```yaml +profiles: + gitlab-work: + git: + provider: gitlab + host: gitlab.example.com + auth_mode: pat + credential: + store: local-os + name: codereview/gitlab-work +``` + +```bash +printf '%s' "$GITLAB_TOKEN" | cr set-credential \ + --store local-os \ + --name codereview/gitlab-work \ + --key git_token \ + --stdin + +cr review https://gitlab.example.com/group/subgroup/project/-/merge_requests/42 +``` + +`git.provider` defaults to `github` when omitted, so existing configs are +unaffected. GitLab specifics worth knowing: + +- Inline findings post as diff discussions and thread resolution uses the + discussions API, so `resolve_threads` works as on GitHub. +- GitLab has no review-object equivalent of a GitHub review: an `approve` + verdict calls the approvals API and the review summary posts as a + merge-request note; `request_changes` revokes any active approval by the + posting identity before posting the summary. +- `pat` is the only supported `auth_mode` for GitLab; there is no GitHub App + equivalent. +- The `cr init` wizard does not offer GitLab yet — add the profile to + `config.yml` directly (validated on load) and store the token with + `cr set-credential`. +- Reading merge-request diffs requires GitLab 15.7 or newer for the raw diff + endpoint; older instances fall back to per-file diff reconstruction. + ### Org Deployment / MDM For managed rollouts, ship non-secret config and pre-stage secrets in the @@ -1043,8 +1090,9 @@ provenance, and trust note when repo-local agents are considered. cr review [flags] ``` -Runs the automated review pipeline for a GitHub pull request URL. The PR host -must match the active profile's `git.host`. +Runs the automated review pipeline for a GitHub pull request URL or a GitLab +merge request URL. The PR host must match the active profile's `git.host`, and +the URL's provider family must match the profile's `git.provider`. Modes: diff --git a/internal/app/runtime.go b/internal/app/runtime.go index c0697959..50020064 100644 --- a/internal/app/runtime.go +++ b/internal/app/runtime.go @@ -21,6 +21,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/gitexec" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" "github.com/open-cli-collective/codereview-cli/internal/hooks" "github.com/open-cli-collective/codereview-cli/internal/ledger" "github.com/open-cli-collective/codereview-cli/internal/llm" @@ -91,7 +92,7 @@ type SelectionOpenRequest struct { // GitProviderFactory builds a provider and the credential used to authenticate // it. -type GitProviderFactory func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) +type GitProviderFactory func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) // PostingIdentityResolver resolves the identity used for live review writes. type PostingIdentityResolver func(context.Context, gitprovider.GitProvider, gitprovider.Credential, credentials.Reader, config.Profile) (gitprovider.Identity, error) @@ -103,7 +104,7 @@ type AdapterFactory func(config.LLMConfig, credentials.Reader) (llm.Adapter, err // defaults. type Dependencies struct { NewGitProvider GitProviderFactory - NewGitCommand func(string, gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) + NewGitCommand func(string, gitexec.TokenSource, string) (func(context.Context, string, ...string) ([]byte, error), error) ResolveRepoRoot func(context.Context) (string, error) ResolvePostingIdentity PostingIdentityResolver NewAdapter AdapterFactory @@ -114,16 +115,14 @@ type Dependencies struct { func (d Dependencies) withDefaults() Dependencies { if d.NewGitProvider == nil { - d.NewGitProvider = func(git config.GitConfig, store credentials.Reader, opts githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { - return githubprovider.NewFromGitConfig(git, store, opts) - } + d.NewGitProvider = gitproviders.New } if d.ResolvePostingIdentity == nil { d.ResolvePostingIdentity = resolvePostingIdentity } if d.NewGitCommand == nil { - d.NewGitCommand = func(host string, tokens gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) { - client, err := gitexec.New(host, tokens) + d.NewGitCommand = func(host string, tokens gitexec.TokenSource, username string) (func(context.Context, string, ...string) ([]byte, error), error) { + client, err := gitexec.NewWithUsername(host, tokens, username) if err != nil { return nil, err } @@ -169,7 +168,7 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) { cleanup() return Runtime{}, err } - gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(repoProvider, repoCredential)) + gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(repoProvider, repoCredential), gitproviders.GitBasicAuthUsername(profile.Git.ProviderKind())) if err != nil { cleanup() return Runtime{}, err @@ -295,6 +294,7 @@ func gitConfigForReviewerAuth(profile config.Profile) config.GitConfig { githubApp = &app } return config.GitConfig{ + Provider: profile.Git.Provider, Host: profile.Git.Host, AuthMode: profile.ReviewerCredentials.AuthMode, Credential: profile.ReviewerCredentials.Credential, @@ -362,14 +362,17 @@ func normalizeRuntimeProfile(profile config.Profile) config.Profile { }).Profiles["runtime"] } -func gitProviderOptions(profile config.Profile, git config.GitConfig, ref gitprovider.PRRef) githubprovider.Options { - opts := githubprovider.Options{} +func gitProviderOptions(profile config.Profile, git config.GitConfig, ref gitprovider.PRRef) gitproviders.Options { + opts := gitproviders.Options{} + if git.ProviderKind() != config.GitProviderGitHub { + return opts + } if installationID := config.PinnedGitHubAppInstallationIDForGit(profile, git); installationID != "" { - opts.InstallationID = installationID + opts.GitHub.InstallationID = installationID return opts } if strings.TrimSpace(ref.Owner) != "" && strings.TrimSpace(ref.Repo) != "" { - opts.InstallationLookup = &githubprovider.InstallationLookup{ + opts.GitHub.InstallationLookup = &githubprovider.InstallationLookup{ Owner: ref.Owner, Repo: ref.Repo, } diff --git a/internal/app/runtime_selection.go b/internal/app/runtime_selection.go index a4a02727..ac828d27 100644 --- a/internal/app/runtime_selection.go +++ b/internal/app/runtime_selection.go @@ -4,6 +4,7 @@ import ( "context" "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" "github.com/open-cli-collective/codereview-cli/internal/pipeline" ) @@ -38,7 +39,7 @@ func OpenSelection(ctx context.Context, req SelectionOpenRequest) (SelectionRunt cleanup() return SelectionRuntime{}, err } - gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(provider, credential)) + gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(provider, credential), gitproviders.GitBasicAuthUsername(profile.Git.ProviderKind())) if err != nil { cleanup() return SelectionRuntime{}, err diff --git a/internal/app/runtime_test.go b/internal/app/runtime_test.go index c3a2ca89..89ddaa4e 100644 --- a/internal/app/runtime_test.go +++ b/internal/app/runtime_test.go @@ -21,6 +21,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/gitexec" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" "github.com/open-cli-collective/codereview-cli/internal/ledger" "github.com/open-cli-collective/codereview-cli/internal/llm" "github.com/open-cli-collective/codereview-cli/internal/outbox" @@ -43,7 +44,7 @@ func TestOpenCanInstantiateWithoutCobra(t *testing.T) { Profile: cfg.Profiles["home"], PRRef: testPRRef(), Dependencies: testDependencies(t, - func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { return &gitprovider.Fake{}, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, func(context.Context, gitprovider.GitProvider, gitprovider.Credential, credentials.Reader, config.Profile) (gitprovider.Identity, error) { @@ -82,7 +83,7 @@ func TestOpenUsesReviewerCredentialsAsRuntimeProvider(t *testing.T) { identity := gitprovider.Identity{Login: "review-bot", ID: "bot-id"} deps := testDependencies(t, - func(git config.GitConfig, _ credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(git config.GitConfig, _ credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { providerCalls = append(providerCalls, git) if git.Credential.Name == "codereview/home-reviewer" { return reviewerProvider, gitprovider.Credential{Type: "pat", Token: "reviewer-token"}, nil @@ -99,7 +100,7 @@ func TestOpenUsesReviewerCredentialsAsRuntimeProvider(t *testing.T) { return &llm.FakeAdapter{NameValue: "fake-llm"}, nil }, ) - deps.NewGitCommand = func(host string, tokens gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) { + deps.NewGitCommand = func(host string, tokens gitexec.TokenSource, _ string) (func(context.Context, string, ...string) ([]byte, error), error) { if host != "github.com" { t.Fatalf("Git host = %q, want github.com", host) } @@ -179,7 +180,7 @@ func TestOpenWarnsAndContinuesWhenOpinionatedReviewAuthorityIsIneligible(t *test RequireOpinionatedReviewAuthority: true, Warnings: &stderr, Dependencies: testDependencies(t, - func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { return provider, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, func(context.Context, gitprovider.GitProvider, gitprovider.Credential, credentials.Reader, config.Profile) (gitprovider.Identity, error) { @@ -211,7 +212,7 @@ func TestOpenAbortsWhenOpinionatedReviewAuthorityProbeIsCanceled(t *testing.T) { PRRef: testPRRef(), RequireOpinionatedReviewAuthority: true, Dependencies: testDependencies(t, - func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { return provider, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, func(context.Context, gitprovider.GitProvider, gitprovider.Credential, credentials.Reader, config.Profile) (gitprovider.Identity, error) { @@ -232,7 +233,7 @@ func TestOpenClosesCredentialStoresWhenRuntimeLayoutFails(t *testing.T) { layoutErr := errors.New("layout failed") var readers []credentials.Reader deps := testDependencies(t, - func(_ config.GitConfig, tokenStore credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(_ config.GitConfig, tokenStore credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { readers = append(readers, tokenStore) return &gitprovider.Fake{}, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, @@ -260,7 +261,7 @@ func TestOpenClosesCredentialStoresWhenLedgerOpenFails(t *testing.T) { ledgerErr := errors.New("ledger open failed") var readers []credentials.Reader deps := testDependencies(t, - func(_ config.GitConfig, tokenStore credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(_ config.GitConfig, tokenStore credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { readers = append(readers, tokenStore) return &gitprovider.Fake{}, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, @@ -289,7 +290,7 @@ func TestOpenClosesCredentialStoresAndLedgerWhenLimiterCreationFails(t *testing. var readers []credentials.Reader var ledgerStore *ledger.Store deps := testDependencies(t, - func(_ config.GitConfig, tokenStore credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(_ config.GitConfig, tokenStore credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { readers = append(readers, tokenStore) return &gitprovider.Fake{}, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, @@ -376,9 +377,9 @@ func TestOpenPassesGitHubAppInstallationLookupAndPinnedID(t *testing.T) { Profile: cfg.Profiles["home"], PRRef: testPRRef(), Dependencies: testDependencies(t, - func(_ config.GitConfig, _ credentials.Reader, opts githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { - gotLookup = opts.InstallationLookup - gotInstallationID = opts.InstallationID + func(_ config.GitConfig, _ credentials.Reader, opts gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + gotLookup = opts.GitHub.InstallationLookup + gotInstallationID = opts.GitHub.InstallationID return &gitprovider.Fake{}, gitprovider.Credential{Type: "github_app", Token: "installation-token"}, nil }, func(context.Context, gitprovider.GitProvider, gitprovider.Credential, credentials.Reader, config.Profile) (gitprovider.Identity, error) { @@ -460,9 +461,9 @@ func TestOpenSelectionPassesGitHubAppInstallationLookupAndPinnedID(t *testing.T) Profile: cfg.Profiles["home"], PRRef: testPRRef(), Dependencies: Dependencies{ - NewGitProvider: func(_ config.GitConfig, _ credentials.Reader, opts githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { - gotLookup = opts.InstallationLookup - gotInstallationID = opts.InstallationID + NewGitProvider: func(_ config.GitConfig, _ credentials.Reader, opts gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + gotLookup = opts.GitHub.InstallationLookup + gotInstallationID = opts.GitHub.InstallationID return &gitprovider.Fake{}, gitprovider.Credential{Type: "github_app", Token: "installation-token"}, nil }, NewAdapter: func(config.LLMConfig, credentials.Reader) (llm.Adapter, error) { @@ -528,10 +529,10 @@ func TestOpenSelectionReturnsExecutableSelectionRuntime(t *testing.T) { runtime, err := OpenSelection(ctx, SelectionOpenRequest{ Config: cfg, Profile: profile, PRRef: ref, Dependencies: Dependencies{ - NewGitProvider: func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + NewGitProvider: func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { return provider, gitprovider.Credential{Type: "pat", Token: "repo-token"}, nil }, - NewGitCommand: func(_ string, tokens gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) { + NewGitCommand: func(_ string, tokens gitexec.TokenSource, _ string) (func(context.Context, string, ...string) ([]byte, error), error) { return func(ctx context.Context, dir string, args ...string) ([]byte, error) { cmdArgs := append([]string(nil), args...) if len(cmdArgs) >= 3 && cmdArgs[0] == "fetch" { @@ -634,7 +635,7 @@ func TestOpenSelectionUsesNamedSecretsStoreWithoutBackendOverride(t *testing.T) Config: cfg, Profile: cfg.Profiles["home"], Dependencies: Dependencies{ - NewGitProvider: func(_ config.GitConfig, tokenStore credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + NewGitProvider: func(_ config.GitConfig, tokenStore credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { token, err := tokenStore.Get("home", credentials.GitTokenKey) if err != nil { t.Fatalf("tokenStore.Get(home, git_token): %v", err) @@ -666,7 +667,7 @@ func TestOpenSelectionClosesCredentialStoresWhenProviderCreationFails(t *testing Config: cfg, Profile: cfg.Profiles["home"], Dependencies: Dependencies{ - NewGitProvider: func(_ config.GitConfig, tokenStore credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + NewGitProvider: func(_ config.GitConfig, tokenStore credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { readers = append(readers, tokenStore) return nil, gitprovider.Credential{}, providerErr }, @@ -691,7 +692,7 @@ func TestOpenSelectionClosesCredentialStoresWhenAdapterCreationFails(t *testing. Config: cfg, Profile: profile, Dependencies: Dependencies{ - NewGitProvider: func(_ config.GitConfig, tokenStore credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + NewGitProvider: func(_ config.GitConfig, tokenStore credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { readers = append(readers, tokenStore) return &gitprovider.Fake{}, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, @@ -725,7 +726,7 @@ func TestOpenInjectsCachedReaderIntoProviderAndAdapter(t *testing.T) { Profile: profile, PRRef: testPRRef(), Dependencies: testDependencies(t, - func(git config.GitConfig, reader credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(git config.GitConfig, reader credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { if git.Credential.Name == "codereview/home" && providerReader == nil { providerReader = reader } else { @@ -782,7 +783,7 @@ func TestOpenSelectionInjectsCachedReaderIntoProviderAndAdapter(t *testing.T) { Profile: profile, PRRef: testPRRef(), Dependencies: Dependencies{ - NewGitProvider: func(_ config.GitConfig, reader credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + NewGitProvider: func(_ config.GitConfig, reader credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { providerReader = reader return &gitprovider.Fake{}, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, @@ -835,7 +836,7 @@ func TestOpenSelectionCachedReaderReturnsStoreClosedAfterCleanupForCachedKey(t * Profile: profile, PRRef: testPRRef(), Dependencies: Dependencies{ - NewGitProvider: func(_ config.GitConfig, reader credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + NewGitProvider: func(_ config.GitConfig, reader credentials.Reader, _ gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { providerReader = reader token, err := reader.Get("home", credentials.GitTokenKey) if err != nil { @@ -887,7 +888,7 @@ func TestOpenLiveApprovedFastPathDoesNotInitializeAdapter(t *testing.T) { Profile: profile, PRRef: ref, Dependencies: testDependencies(t, - func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { return provider, gitprovider.Credential{Type: "pat", Token: "token"}, nil }, func(context.Context, gitprovider.GitProvider, gitprovider.Credential, credentials.Reader, config.Profile) (gitprovider.Identity, error) { @@ -976,7 +977,7 @@ func TestAdapterConstructorsCoverLLMRuntimeSpecs(t *testing.T) { func testDependencies(t *testing.T, provider GitProviderFactory, identity PostingIdentityResolver, adapter AdapterFactory) Dependencies { t.Helper() if provider == nil { - provider = func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + provider = func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { return &gitprovider.Fake{}, gitprovider.Credential{Type: "pat", Token: "token"}, nil } } diff --git a/internal/benchmark/suite.go b/internal/benchmark/suite.go index 73b43058..301991b7 100644 --- a/internal/benchmark/suite.go +++ b/internal/benchmark/suite.go @@ -584,7 +584,7 @@ func validateCases(cases []Case) error { return fmt.Errorf("%w: duplicate case id %q", ErrInvalid, benchCase.ID) } seen[benchCase.ID] = true - if _, err := prref.ParseGitHubPullURL(benchCase.PR); err != nil { + if _, _, err := prref.ParsePullURL(benchCase.PR); err != nil { return fmt.Errorf("%w: case %q invalid PR URL: %w", ErrInvalid, benchCase.ID, err) } if err := validateOptionalSHA("review_base_sha", benchCase.ID, benchCase.ReviewBaseSHA, benchCase.reviewBaseSHASet); err != nil { @@ -642,7 +642,7 @@ func validateCandidateCaseHosts(candidates []Candidate, cases []Case, cfg config profile := cfg.Profiles[candidate.Profile] for i, benchCase := range cases { if !parsed[i] { - ref, err := prref.ParseGitHubPullURL(benchCase.PR) + ref, _, err := prref.ParsePullURL(benchCase.PR) if err != nil { return fmt.Errorf("%w: case %q invalid PR URL: %w", ErrInvalid, benchCase.ID, err) } diff --git a/internal/cmd/agentscmd/agentscmd.go b/internal/cmd/agentscmd/agentscmd.go index b4d74686..e0148c6c 100644 --- a/internal/cmd/agentscmd/agentscmd.go +++ b/internal/cmd/agentscmd/agentscmd.go @@ -18,7 +18,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/config" "github.com/open-cli-collective/codereview-cli/internal/credentials" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" - githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" "github.com/open-cli-collective/codereview-cli/internal/prref" "github.com/open-cli-collective/codereview-cli/internal/reporoot" "github.com/open-cli-collective/codereview-cli/internal/view" @@ -34,7 +34,7 @@ type commandFlags struct { // Register attaches the agents command tree to rootCmd. func Register(rootCmd *cobra.Command, opts *root.Options) { - RegisterWithFactory(rootCmd, opts, newGitHubProvider) + RegisterWithFactory(rootCmd, opts, newConfiguredProvider) } // RegisterWithFactory attaches the agents command tree with an injected provider factory. @@ -118,11 +118,12 @@ func buildCatalog(ctx context.Context, cmd *cobra.Command, opts *root.Options, f return agents.Catalog{}, cmderr.Config(err) } var ref gitprovider.PRRef + var urlProvider prref.Provider profile := config.Profile{} hasPR := strings.TrimSpace(prArg) != "" if hasPR { var err error - ref, err = prref.ParseGitHubPullURL(prArg) + ref, urlProvider, err = prref.ParsePullURL(prArg) if err != nil { return agents.Catalog{}, exitcode.Usage(err) } @@ -157,6 +158,9 @@ func buildCatalog(ctx context.Context, cmd *cobra.Command, opts *root.Options, f if !prref.SameHost(ref.Host, profile.Git.Host) { return agents.Catalog{}, exitcode.Usage(fmt.Errorf("PR host %q must match configured git host %q", ref.Host, profile.Git.Host)) } + if err := prref.MatchProvider(urlProvider, string(profile.Git.ProviderKind())); err != nil { + return agents.Catalog{}, exitcode.Usage(err) + } provider, cleanup, err := factory(cmd, opts, cfg, profile) if err != nil { return agents.Catalog{}, err @@ -194,12 +198,12 @@ func mapLoadError(err error) error { } } -func newGitHubProvider(cmd *cobra.Command, opts *root.Options, cfg config.File, profile config.Profile) (gitprovider.GitProvider, func(), error) { +func newConfiguredProvider(cmd *cobra.Command, opts *root.Options, cfg config.File, profile config.Profile) (gitprovider.GitProvider, func(), error) { store, err := credentials.OpenStore(opts.Backend, cmderr.BackendFlagChanged(cmd), cfg) if err != nil { return nil, nil, cmderr.Credential(err) } - client, _, err := githubprovider.NewFromGitConfig(profile.Git, store, githubprovider.Options{}) + client, _, err := gitproviders.New(profile.Git, store, gitproviders.Options{}) if err != nil { _ = store.Close() if errors.Is(err, config.ErrUnsupported) { diff --git a/internal/cmd/benchmarkcmd/executor.go b/internal/cmd/benchmarkcmd/executor.go index 5a6d5453..febc8014 100644 --- a/internal/cmd/benchmarkcmd/executor.go +++ b/internal/cmd/benchmarkcmd/executor.go @@ -118,7 +118,7 @@ func (e inProcessExecutor) Execute(ctx context.Context, req reviewExecutionReque result.Stderr = append([]byte(nil), stderr.Bytes()...) }() - ref, err := prref.ParseGitHubPullURL(req.Case.PR) + ref, urlProvider, err := prref.ParsePullURL(req.Case.PR) if err != nil { return inProcessReviewFailure(exitcode.Usage(err), &stderr) } @@ -129,6 +129,9 @@ func (e inProcessExecutor) Execute(ctx context.Context, req reviewExecutionReque if !prref.SameHost(ref.Host, profile.Git.Host) { return inProcessReviewFailure(exitcode.Usage(fmt.Errorf("PR host %q must match configured git host %q", ref.Host, profile.Git.Host)), &stderr) } + if err := prref.MatchProvider(urlProvider, string(profile.Git.ProviderKind())); err != nil { + return inProcessReviewFailure(exitcode.Usage(err), &stderr) + } selectionPrompt, err := loadSelectionPromptInstructions(req.SuiteDir, req.Candidate.Stages.Selection.Prompt) if err != nil { return inProcessReviewFailure(exitcode.Usage(err), &stderr) diff --git a/internal/cmd/benchmarkcmd/select.go b/internal/cmd/benchmarkcmd/select.go index ce54c7fd..3c72f266 100644 --- a/internal/cmd/benchmarkcmd/select.go +++ b/internal/cmd/benchmarkcmd/select.go @@ -258,7 +258,7 @@ func executeBenchmarkSelectRun(ctx context.Context, logger *progress.Logger, sui return finalized, runSpan.End(finalizeErr) } parseSpan := logger.Start("benchmark.select", "parse_pr", runID) - ref, err := prref.ParseGitHubPullURL(benchCase.PR) + ref, _, err := prref.ParsePullURL(benchCase.PR) if err != nil { _ = parseSpan.End(err) return fail(err) diff --git a/internal/cmd/configcmd/configcmd.go b/internal/cmd/configcmd/configcmd.go index 4d25bba4..2f223170 100644 --- a/internal/cmd/configcmd/configcmd.go +++ b/internal/cmd/configcmd/configcmd.go @@ -249,7 +249,7 @@ func Register(rootCmd *cobra.Command, opts *root.Options) { Short: "Preview which profile a PR URL would resolve to", Args: exitcode.ExactArgs(1, "config resolve-profile requires "), RunE: func(cmd *cobra.Command, args []string) error { - ref, err := prref.ParseGitHubPullURL(args[0]) + ref, urlProvider, err := prref.ParsePullURL(args[0]) if err != nil { return exitcode.Usage(err) } @@ -268,6 +268,9 @@ func Register(rootCmd *cobra.Command, opts *root.Options) { if !prref.SameHost(ref.Host, resolution.Profile.Git.Host) { return exitcode.Usage(fmt.Errorf("PR host %q must match configured git host %q", ref.Host, resolution.Profile.Git.Host)) } + if err := prref.MatchProvider(urlProvider, string(resolution.Profile.Git.ProviderKind())); err != nil { + return exitcode.Usage(err) + } result := configResolveProfileView(args[0], resolution) return view.Render(opts.Stdout, resolveProfileJSON, result, func(w io.Writer) error { return view.RenderConfigResolveProfileText(w, result) diff --git a/internal/cmd/initcmd/initcmd.go b/internal/cmd/initcmd/initcmd.go index a08a50e5..a82f1b99 100644 --- a/internal/cmd/initcmd/initcmd.go +++ b/internal/cmd/initcmd/initcmd.go @@ -5249,7 +5249,7 @@ func parseInitRouteSpec(raw string) (configedit.RepositoryRouteSpec, error) { case 3: repos = append(repos, parts[2]) default: - return configedit.RepositoryRouteSpec{}, fmt.Errorf("route %q must be host/namespace, host/namespace/repo, host/namespace [repo1, repo2], or a GitHub PR URL", raw) + return configedit.RepositoryRouteSpec{}, fmt.Errorf("route %q must be host/namespace, host/namespace/repo, host/namespace [repo1, repo2], or a PR/MR URL", raw) } return configedit.NormalizeRepositoryRouteSpec(configedit.RepositoryRouteSpec{ Host: parts[0], @@ -5260,14 +5260,15 @@ func parseInitRouteSpec(raw string) (configedit.RepositoryRouteSpec, error) { func parseInitRoutePRURL(raw string) (gitprovider.PRRef, error) { trimmed := strings.TrimSpace(raw) - ref, err := prref.ParseGitHubPullURL(trimmed) + ref, _, err := prref.ParsePullURL(trimmed) if err == nil { return ref, nil } if strings.Contains(trimmed, "://") { return gitprovider.PRRef{}, err } - return prref.ParseGitHubPullURL("https://" + strings.TrimPrefix(trimmed, "//")) + ref, _, err = prref.ParsePullURL("https://" + strings.TrimPrefix(trimmed, "//")) + return ref, err } func collectInteractiveInitRetentionConfig(opts *root.Options, deps initDeps, cfg config.File) (config.File, error) { diff --git a/internal/cmd/mecmd/mecmd.go b/internal/cmd/mecmd/mecmd.go index adb078bf..09ee9970 100644 --- a/internal/cmd/mecmd/mecmd.go +++ b/internal/cmd/mecmd/mecmd.go @@ -19,7 +19,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/config" "github.com/open-cli-collective/codereview-cli/internal/credentials" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" - githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" "github.com/open-cli-collective/codereview-cli/internal/identity" "github.com/open-cli-collective/codereview-cli/internal/progress" "github.com/open-cli-collective/codereview-cli/internal/view" @@ -30,7 +30,7 @@ type IdentityResolverFactory func(cmd *cobra.Command, opts *root.Options, cfg co // Register attaches the me command to rootCmd. func Register(rootCmd *cobra.Command, opts *root.Options) { - RegisterWithFactory(rootCmd, opts, newGitHubResolver) + RegisterWithFactory(rootCmd, opts, newProviderResolver) } // RegisterWithFactory attaches the me command with an injected resolver factory. @@ -191,8 +191,8 @@ func mapRunError(err error) error { return err } -func newGitHubResolver(cmd *cobra.Command, opts *root.Options, cfg config.File) (identity.Resolver, func(), error) { - return &githubResolver{ +func newProviderResolver(cmd *cobra.Command, opts *root.Options, cfg config.File) (identity.Resolver, func(), error) { + return &providerResolver{ cfg: cfg, backend: opts.Backend, backendFlagChanged: cmderr.BackendFlagChanged(cmd), @@ -201,21 +201,21 @@ func newGitHubResolver(cmd *cobra.Command, opts *root.Options, cfg config.File) }, nil, nil } -type githubResolver struct { +type providerResolver struct { cfg config.File backend string backendFlagChanged bool - options githubprovider.Options + options gitproviders.Options warnings io.Writer logger *progress.Logger - NewClient func(config.GitConfig, credentials.Reader, githubprovider.Options) (*githubprovider.Client, gitprovider.Credential, error) + NewClient func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) } -// ResolveIdentity resolves one configured GitHub identity. -func (r *githubResolver) ResolveIdentity(ctx context.Context, profileName string, git config.GitConfig) (gitprovider.Identity, error) { +// ResolveIdentity resolves one configured git-host identity. +func (r *providerResolver) ResolveIdentity(ctx context.Context, profileName string, git config.GitConfig) (gitprovider.Identity, error) { newClient := r.NewClient if newClient == nil { - newClient = githubprovider.NewFromGitConfig + newClient = gitproviders.New } resolvedSecretsStore, err := credentials.ResolveSecretsStoreForRef(r.cfg, git.Credential.Name, profileName) if err != nil { @@ -228,7 +228,7 @@ func (r *githubResolver) ResolveIdentity(ctx context.Context, profileName string defer store.Close() options := r.options if installationID := config.PinnedGitHubAppInstallationIDForGit(r.cfg.Profiles[profileName], git); installationID != "" { - options.InstallationID = installationID + options.GitHub.InstallationID = installationID } else if git.AuthMode == config.GitAuthModeGitHubApp { cached := strings.TrimSpace(git.IdentityCache) if cached == "" { @@ -247,4 +247,4 @@ func (r *githubResolver) ResolveIdentity(ctx context.Context, profileName string return client.WhoAmI(ctx, credential) } -var _ identity.Resolver = (*githubResolver)(nil) +var _ identity.Resolver = (*providerResolver)(nil) diff --git a/internal/cmd/mecmd/mecmd_test.go b/internal/cmd/mecmd/mecmd_test.go index c1875bfa..3e62e4a0 100644 --- a/internal/cmd/mecmd/mecmd_test.go +++ b/internal/cmd/mecmd/mecmd_test.go @@ -29,6 +29,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/credentials" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" "github.com/open-cli-collective/codereview-cli/internal/identity" "github.com/open-cli-collective/codereview-cli/internal/view" ) @@ -67,12 +68,12 @@ func TestMeJSONDoesNotLeakTokenMaterial(t *testing.T) { })) defer server.Close() cmd, out := newTestCommandWithFactory(path, func(_ *cobra.Command, _ *root.Options, cfg config.File) (identity.Resolver, func(), error) { - return &githubResolver{ + return &providerResolver{ cfg: cfg, - options: githubprovider.Options{ + options: gitproviders.Options{GitHub: githubprovider.Options{ BaseURL: server.URL, GraphQLURL: server.URL + "/graphql", - }, + }}, }, nil, nil }) @@ -115,12 +116,12 @@ func TestMeUsesNamedSecretsStoreWithoutBackendOverride(t *testing.T) { })) defer server.Close() cmd, out := newTestCommandWithFactory(path, func(_ *cobra.Command, _ *root.Options, cfg config.File) (identity.Resolver, func(), error) { - return &githubResolver{ + return &providerResolver{ cfg: cfg, - options: githubprovider.Options{ + options: gitproviders.Options{GitHub: githubprovider.Options{ BaseURL: server.URL, GraphQLURL: server.URL + "/graphql", - }, + }}, }, nil, nil }) @@ -198,12 +199,12 @@ func TestMeReviewerCredentialsTakePrecedenceOverGitHubAppRepositoryAccess(t *tes })) defer server.Close() cmd, out := newTestCommandWithFactory(path, func(_ *cobra.Command, _ *root.Options, cfg config.File) (identity.Resolver, func(), error) { - return &githubResolver{ + return &providerResolver{ cfg: cfg, - options: githubprovider.Options{ + options: gitproviders.Options{GitHub: githubprovider.Options{ BaseURL: server.URL, GraphQLURL: server.URL + "/graphql", - }, + }}, }, nil, nil }) @@ -517,10 +518,10 @@ profiles: llm_runtime: claude-cli `) cmd, out := newTestCommandWithFactory(path, func(_ *cobra.Command, opts *root.Options, cfg config.File) (identity.Resolver, func(), error) { - return &githubResolver{ + return &providerResolver{ cfg: cfg, warnings: opts.Stderr, - NewClient: func(config.GitConfig, credentials.Reader, githubprovider.Options) (*githubprovider.Client, gitprovider.Credential, error) { + NewClient: func(config.GitConfig, credentials.Reader, gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { t.Fatal("GitHub client should not be opened for discovery-mode app identity without PR context") return nil, gitprovider.Credential{}, nil }, @@ -804,12 +805,12 @@ func TestGitHubResolverUsesCR08FactoryAndCredentialStore(t *testing.T) { resolverCfg := fileBackendConfig(t) resolverCfg.Profiles = map[string]config.Profile{"work": resolverCfg.Profiles["work"]} - resolver := &githubResolver{ + resolver := &providerResolver{ cfg: resolverCfg, - options: githubprovider.Options{ + options: gitproviders.Options{GitHub: githubprovider.Options{ BaseURL: server.URL, GraphQLURL: server.URL + "/graphql", - }, + }}, } gitIdentity, err := resolver.ResolveIdentity(context.Background(), "work", config.GitConfig{ Host: "github.com", @@ -1011,13 +1012,13 @@ func runOrgDeploymentCommand(t *testing.T, path string, stdin *strings.Reader, f func orgDeploymentFactory(serverURL string) IdentityResolverFactory { return func(_ *cobra.Command, opts *root.Options, cfg config.File) (identity.Resolver, func(), error) { - return &githubResolver{ + return &providerResolver{ cfg: cfg, backend: opts.Backend, - options: githubprovider.Options{ + options: gitproviders.Options{GitHub: githubprovider.Options{ BaseURL: serverURL, GraphQLURL: serverURL + "/graphql", - }, + }}, }, nil, nil } } diff --git a/internal/cmd/respondcmd/respondcmd.go b/internal/cmd/respondcmd/respondcmd.go index bfd7bc37..c5990481 100644 --- a/internal/cmd/respondcmd/respondcmd.go +++ b/internal/cmd/respondcmd/respondcmd.go @@ -84,7 +84,7 @@ func run(ctx context.Context, cmd *cobra.Command, opts *root.Options, factory Ru if err != nil { return cmderr.Config(err) } - ref, err := prref.ParseGitHubPullURL(prArg) + ref, urlProvider, err := prref.ParsePullURL(prArg) if err != nil { return exitcode.Usage(err) } @@ -99,6 +99,9 @@ func run(ctx context.Context, cmd *cobra.Command, opts *root.Options, factory Ru if !prref.SameHost(ref.Host, profile.Git.Host) { return exitcode.Usage(fmt.Errorf("PR host %q must match configured git host %q", ref.Host, profile.Git.Host)) } + if err := prref.MatchProvider(urlProvider, string(profile.Git.ProviderKind())); err != nil { + return exitcode.Usage(err) + } runtime, err := factory(ctx, app.OpenRequest{ Config: cfg, Profile: profile, diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index ef87bfc8..fb72b80e 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -245,7 +245,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact } _ = configSpan.End(nil) parseSpan := logger.Start("review", "parse_pr", "pr") - ref, err := prref.ParseGitHubPullURL(prArg) + ref, urlProvider, err := prref.ParsePullURL(prArg) if err != nil { return exitcode.Usage(parseSpan.End(err)) } @@ -262,6 +262,9 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact if !prref.SameHost(ref.Host, profile.Git.Host) { return exitcode.Usage(profileSpan.End(fmt.Errorf("PR host %q must match configured git host %q", ref.Host, profile.Git.Host))) } + if err := prref.MatchProvider(urlProvider, string(profile.Git.ProviderKind())); err != nil { + return exitcode.Usage(profileSpan.End(err)) + } _ = profileSpan.End(nil) runtimeReq := app.OpenRequest{ diff --git a/internal/cmd/reviewcmd/reviewcmd_gitlab_test.go b/internal/cmd/reviewcmd/reviewcmd_gitlab_test.go new file mode 100644 index 00000000..ed4a9246 --- /dev/null +++ b/internal/cmd/reviewcmd/reviewcmd_gitlab_test.go @@ -0,0 +1,77 @@ +package reviewcmd + +import ( + "context" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/app" + "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +func gitLabTestConfig() config.File { + cfg := testConfig() + home := cfg.Profiles["home"] + home.Git.Provider = config.GitProviderGitLab + home.Git.Host = "gitlab.example.com" + cfg.Profiles["home"] = home + cfg.RepositoryProfiles = nil + return cfg +} + +func TestReviewAcceptsGitLabMergeRequestURL(t *testing.T) { + runner := &fakeRunner{result: testPipelineResult(false)} + cmd, _ := newTestCommand(t, gitLabTestConfig(), fakeFactory(runner)) + + err := root.Execute(cmd, []string{"--profile", "home", "review", "https://gitlab.example.com/group/subgroup/project/-/merge_requests/29", "--dry-run"}) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if len(runner.requests) != 1 { + t.Fatalf("runner calls = %d, want 1", len(runner.requests)) + } + want := gitprovider.PRRef{Host: "gitlab.example.com", Owner: "group/subgroup", Repo: "project", Number: 29} + if runner.requests[0].PRRef != want { + t.Fatalf("PRRef = %#v, want %#v", runner.requests[0].PRRef, want) + } +} + +func TestReviewRejectsGitLabURLForGitHubProviderProfile(t *testing.T) { + cfg := gitLabTestConfig() + home := cfg.Profiles["home"] + home.Git.Provider = config.GitProviderGitHub + cfg.Profiles["home"] = home + cmd, _ := newTestCommand(t, cfg, func(context.Context, app.OpenRequest) (app.Runtime, error) { + t.Fatal("runtime factory should not be called on provider mismatch") + return app.Runtime{}, nil + }) + + err := root.Execute(cmd, []string{"--profile", "home", "review", "https://gitlab.example.com/group/project/-/merge_requests/29", "--dry-run"}) + if err == nil { + t.Fatal("Execute error = nil, want provider mismatch") + } + if got := exitcode.FromError(err); got != exitcode.UsageError { + t.Fatalf("exit code = %d, want usage", got) + } + if !strings.Contains(err.Error(), `PR URL is a gitlab URL but the configured git provider is "github"`) { + t.Fatalf("error = %v, want provider mismatch detail", err) + } +} + +func TestReviewRejectsGitHubURLForGitLabProviderProfile(t *testing.T) { + cmd, _ := newTestCommand(t, gitLabTestConfig(), func(context.Context, app.OpenRequest) (app.Runtime, error) { + t.Fatal("runtime factory should not be called on provider mismatch") + return app.Runtime{}, nil + }) + + err := root.Execute(cmd, []string{"--profile", "home", "review", "https://gitlab.example.com/group/project/pull/29", "--dry-run"}) + if err == nil { + t.Fatal("Execute error = nil, want provider mismatch") + } + if !strings.Contains(err.Error(), `PR URL is a github URL but the configured git provider is "gitlab"`) { + t.Fatalf("error = %v, want provider mismatch detail", err) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index a213820c..8f691892 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -232,6 +232,7 @@ func (p Profile) MarshalYAML() (any, error) { // GitConfig identifies the user's git-host credentials. type GitConfig struct { + Provider GitProviderKind `yaml:"provider,omitempty" json:"provider,omitempty"` Host string `yaml:"host" json:"host"` AuthMode GitAuthMode `yaml:"auth_mode" json:"auth_mode"` Credential CredentialLocation `yaml:"credential" json:"credential"` @@ -239,6 +240,15 @@ type GitConfig struct { IdentityCache string `yaml:"identity_cache,omitempty" json:"identity_cache,omitempty"` } +// ProviderKind returns the effective git provider, defaulting to GitHub when +// the config omits the field. +func (g GitConfig) ProviderKind() GitProviderKind { + if g.Provider == "" { + return GitProviderGitHub + } + return g.Provider +} + // GitHubAppConfig stores non-secret GitHub App identifiers. type GitHubAppConfig struct { AppID string `yaml:"app_id" json:"app_id"` @@ -407,6 +417,27 @@ type RetentionConfig struct { Enforcement RetentionEnforcement `yaml:"enforcement,omitempty" json:"enforcement"` } +// GitProviderKind identifies which git-host API family a git config targets. +type GitProviderKind string + +// Git provider kinds. An empty value means GitHub for backward compatibility +// with configs written before the field existed. +const ( + GitProviderGitHub GitProviderKind = "github" + GitProviderGitLab GitProviderKind = "gitlab" +) + +// Valid reports whether k is a known git provider kind. The empty value is +// valid and means GitHub. +func (k GitProviderKind) Valid() bool { + switch k { + case "", GitProviderGitHub, GitProviderGitLab: + return true + default: + return false + } +} + // GitAuthMode identifies how git-host credentials are obtained. type GitAuthMode string @@ -1418,6 +1449,9 @@ func validateRepositoryAccess(secrets SecretsConfig, name string, access Reposit } func validateGitConfig(path string, git GitConfig) error { + if !git.Provider.Valid() { + return invalid("%s.provider %q is invalid: valid values are github, gitlab", path, git.Provider) + } if strings.TrimSpace(git.Host) == "" { return invalid("%s.host is required", path) } @@ -1427,6 +1461,9 @@ func validateGitConfig(path string, git GitConfig) error { if !git.AuthMode.Supported() { return fmt.Errorf("%w: %s.auth_mode %q", ErrUnsupported, path, git.AuthMode) } + if git.ProviderKind() == GitProviderGitLab && git.AuthMode != GitAuthModePAT { + return fmt.Errorf("%w: %s.auth_mode %q is not supported for provider gitlab; use pat", ErrUnsupported, path, git.AuthMode) + } if err := validateCredentialLocation(path+".credential", git.Credential); err != nil { return err } @@ -2126,6 +2163,7 @@ func (i ProfileReviewerGitHubAppInstallation) normalized() ProfileReviewerGitHub } func (g GitConfig) normalized() GitConfig { + g.Provider = GitProviderKind(strings.ToLower(strings.TrimSpace(string(g.Provider)))) g.Host = normalizeConfigHost(g.Host) g.AuthMode = GitAuthMode(strings.TrimSpace(string(g.AuthMode))) g.Credential = g.Credential.normalized() @@ -2135,7 +2173,8 @@ func (g GitConfig) normalized() GitConfig { func (g GitConfig) empty() bool { g = g.normalized() - return g.Host == "" && + return g.Provider == "" && + g.Host == "" && g.AuthMode == "" && g.Credential.empty() && g.GitHubApp == nil && diff --git a/internal/config/config_git_provider_test.go b/internal/config/config_git_provider_test.go new file mode 100644 index 00000000..7baf9752 --- /dev/null +++ b/internal/config/config_git_provider_test.go @@ -0,0 +1,82 @@ +package config + +import ( + "errors" + "strings" + "testing" +) + +func TestGitProviderKindValid(t *testing.T) { + tests := []struct { + kind GitProviderKind + want bool + }{ + {kind: "", want: true}, + {kind: GitProviderGitHub, want: true}, + {kind: GitProviderGitLab, want: true}, + {kind: "bitbucket", want: false}, + {kind: "GitHub", want: false}, + } + for _, tt := range tests { + if got := tt.kind.Valid(); got != tt.want { + t.Errorf("GitProviderKind(%q).Valid() = %v, want %v", tt.kind, got, tt.want) + } + } +} + +func TestGitConfigProviderKindDefaultsToGitHub(t *testing.T) { + if got := (GitConfig{}).ProviderKind(); got != GitProviderGitHub { + t.Fatalf("ProviderKind() = %q, want %q", got, GitProviderGitHub) + } + if got := (GitConfig{Provider: GitProviderGitLab}).ProviderKind(); got != GitProviderGitLab { + t.Fatalf("ProviderKind() = %q, want %q", got, GitProviderGitLab) + } +} + +func TestValidateAcceptsGitLabProviderWithPAT(t *testing.T) { + cfg := validFile() + profile := cfg.Profiles["home"] + profile.Git.Provider = GitProviderGitLab + profile.Git.Host = "gitlab.example.com" + cfg.Profiles["home"] = profile + if err := Validate(cfg); err != nil { + t.Fatalf("Validate error = %v, want nil", err) + } +} + +func TestValidateRejectsUnknownGitProvider(t *testing.T) { + cfg := validFile() + profile := cfg.Profiles["home"] + profile.Git.Provider = "bitbucket" + cfg.Profiles["home"] = profile + err := Validate(cfg) + if !errors.Is(err, ErrInvalid) { + t.Fatalf("Validate error = %v, want ErrInvalid", err) + } + if !strings.Contains(err.Error(), "provider") { + t.Fatalf("Validate error = %v, want provider mention", err) + } +} + +func TestValidateRejectsGitLabProviderWithGitHubAppAuth(t *testing.T) { + cfg := validFile() + profile := cfg.Profiles["home"] + profile.Git.Provider = GitProviderGitLab + profile.Git.AuthMode = GitAuthModeGitHubApp + profile.Git.GitHubApp = &GitHubAppConfig{AppID: "12345"} + cfg.Profiles["home"] = profile + if err := Validate(cfg); !errors.Is(err, ErrUnsupported) { + t.Fatalf("Validate error = %v, want ErrUnsupported", err) + } +} + +func TestNormalizeLowercasesGitProvider(t *testing.T) { + cfg := validFile() + profile := cfg.Profiles["home"] + profile.Git.Provider = " GitLab " + cfg.Profiles["home"] = profile + normalized := Normalize(cfg) + if got := normalized.Profiles["home"].Git.Provider; got != GitProviderGitLab { + t.Fatalf("normalized provider = %q, want %q", got, GitProviderGitLab) + } +} diff --git a/internal/gitexec/client.go b/internal/gitexec/client.go index 289d9775..673d6d55 100644 --- a/internal/gitexec/client.go +++ b/internal/gitexec/client.go @@ -21,14 +21,26 @@ type TokenSourceFunc func(context.Context) (string, error) // AccessToken returns the current token. func (f TokenSourceFunc) AccessToken(ctx context.Context) (string, error) { return f(ctx) } +// defaultBasicAuthUsername is the GitHub HTTPS transport convention. +const defaultBasicAuthUsername = "x-access-token" + // Client runs Git with host-scoped HTTPS authentication. type Client struct { - host string - tokens TokenSource + host string + tokens TokenSource + username string } -// New constructs an authenticated Git client. +// New constructs an authenticated Git client using the GitHub HTTPS username +// convention. func New(host string, tokens TokenSource) (*Client, error) { + return NewWithUsername(host, tokens, defaultBasicAuthUsername) +} + +// NewWithUsername constructs an authenticated Git client whose HTTPS basic +// auth pairs the given username with the token (for example GitLab expects +// "oauth2"). +func NewWithUsername(host string, tokens TokenSource, username string) (*Client, error) { host = strings.TrimSpace(strings.TrimSuffix(host, "/")) if host == "" || strings.Contains(host, "://") || strings.Contains(host, "/") { return nil, fmt.Errorf("gitexec: valid host is required") @@ -36,7 +48,11 @@ func New(host string, tokens TokenSource) (*Client, error) { if tokens == nil { return nil, fmt.Errorf("gitexec: token source is required") } - return &Client{host: host, tokens: tokens}, nil + username = strings.TrimSpace(username) + if username == "" || strings.Contains(username, ":") { + return nil, fmt.Errorf("gitexec: valid basic auth username is required") + } + return &Client{host: host, tokens: tokens, username: username}, nil } // Run executes Git in dir, or the current directory when dir is empty. @@ -53,7 +69,7 @@ func (c *Client) Run(ctx context.Context, dir string, args ...string) ([]byte, e if token == "" { return nil, fmt.Errorf("gitexec: access token is empty") } - header = "Authorization: Basic " + base64.StdEncoding.EncodeToString([]byte("x-access-token:"+token)) + header = "Authorization: Basic " + base64.StdEncoding.EncodeToString([]byte(c.username+":"+token)) } cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- fixed git binary with structured arguments. if strings.TrimSpace(dir) != "" { diff --git a/internal/gitexec/client_username_test.go b/internal/gitexec/client_username_test.go new file mode 100644 index 00000000..6fb96698 --- /dev/null +++ b/internal/gitexec/client_username_test.go @@ -0,0 +1,52 @@ +package gitexec + +import ( + "context" + "encoding/base64" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestNewWithUsernamePairsUsernameWithToken(t *testing.T) { + dir := t.TempDir() + record := filepath.Join(dir, "record") + script := filepath.Join(dir, "git") + body := "#!/bin/sh\nenv > \"$GITEXEC_RECORD.env\"\n" + if err := os.WriteFile(script, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(script, 0o700); err != nil { // #nosec G302 -- executable test fixture. + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GITEXEC_RECORD", record) + + client, err := NewWithUsername("gitlab.example.com", TokenSourceFunc(func(context.Context) (string, error) { + return "glpat-secret", nil + }), "oauth2") + if err != nil { + t.Fatalf("NewWithUsername: %v", err) + } + if _, err := client.Run(context.Background(), "", "fetch", "https://gitlab.example.com/group/subgroup/project.git"); err != nil { + t.Fatalf("Run: %v", err) + } + env, err := os.ReadFile(record + ".env") // #nosec G304 -- test-controlled temp path. + if err != nil { + t.Fatal(err) + } + wantAuth := base64.StdEncoding.EncodeToString([]byte("oauth2:glpat-secret")) + if !strings.Contains(string(env), wantAuth) { + t.Fatalf("git environment = %s, want oauth2 basic auth", env) + } +} + +func TestNewWithUsernameRejectsInvalidUsername(t *testing.T) { + tokens := TokenSourceFunc(func(context.Context) (string, error) { return "token", nil }) + for _, username := range []string{"", " ", "user:name"} { + if _, err := NewWithUsername("gitlab.example.com", tokens, username); err == nil { + t.Errorf("NewWithUsername(%q) error = nil, want error", username) + } + } +} diff --git a/internal/gitprovider/gitlab/client.go b/internal/gitprovider/gitlab/client.go new file mode 100644 index 00000000..01e6baab --- /dev/null +++ b/internal/gitprovider/gitlab/client.go @@ -0,0 +1,210 @@ +// Package gitlab adapts the GitLab REST API to gitprovider read and write models. +package gitlab + +import ( + "errors" + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +const credentialTypePAT = "pat" + +// ErrValidation identifies non-retryable adapter input or GitLab validation failures. +var ErrValidation = errors.New("gitlab: validation error") + +// Options configures a GitLab client. +type Options struct { + Host string + Token string + HTTPClient *http.Client + BaseURL string +} + +// Client is a GitLab merge-request adapter implementing gitprovider.GitProvider. +type Client struct { + host string + token string + httpClient *http.Client + baseURL *url.URL +} + +// NewFromGitConfig builds a GitLab client and credential from config plus a token store. +func NewFromGitConfig(git config.GitConfig, store credentials.Reader, opts Options) (*Client, gitprovider.Credential, error) { + host, err := normalizeHost(git.Host) + if err != nil { + return nil, gitprovider.Credential{}, err + } + if opts.Host != "" { + optHost, err := normalizeHost(opts.Host) + if err != nil { + return nil, gitprovider.Credential{}, err + } + if optHost != host { + return nil, gitprovider.Credential{}, fmt.Errorf("%w: options host %q conflicts with config host %q", ErrValidation, optHost, host) + } + } + if store == nil { + return nil, gitprovider.Credential{}, fmt.Errorf("%w: token store is required", gitprovider.ErrAuth) + } + parsed, err := credentials.ParseRef(git.Credential.Name) + if err != nil { + return nil, gitprovider.Credential{}, err + } + if git.AuthMode != config.GitAuthModePAT { + return nil, gitprovider.Credential{}, fmt.Errorf("%w: git auth_mode %q for provider gitlab", config.ErrUnsupported, git.AuthMode) + } + key, err := credentials.KeyForPurpose(config.CredentialRef{ + Purpose: "git", + Ref: git.Credential.Name, + Mode: string(git.AuthMode), + }) + if err != nil { + return nil, gitprovider.Credential{}, err + } + token, err := store.Get(parsed.Profile, key) + if err != nil { + return nil, gitprovider.Credential{}, gitprovider.WrapError(gitprovider.ErrAuth, "", fmt.Errorf("read git credential: %w", err)) + } + credential := gitprovider.Credential{Type: credentialTypePAT, Token: token} + if err := validateCredential("", credential); err != nil { + return nil, gitprovider.Credential{}, err + } + opts.Host = host + opts.Token = token + client, err := New(opts) + if err != nil { + return nil, gitprovider.Credential{}, err + } + return client, credential, nil +} + +// New builds a GitLab client from explicit options. +func New(opts Options) (*Client, error) { + normalizedHost, err := normalizeHost(opts.Host) + if err != nil { + return nil, err + } + credential := gitprovider.Credential{Type: credentialTypePAT, Token: opts.Token} + if err := validateCredential("", credential); err != nil { + return nil, err + } + baseURL, err := resolveBaseURL(normalizedHost, opts.BaseURL) + if err != nil { + return nil, err + } + httpClient := opts.HTTPClient + if httpClient == nil { + httpClient = http.DefaultClient + } + return &Client{ + host: normalizedHost, + token: opts.Token, + httpClient: httpClient, + baseURL: baseURL, + }, nil +} + +// Host returns the normalized host this client is bound to. +func (c *Client) Host() string { + if c == nil { + return "" + } + return c.host +} + +// Capabilities returns GitLab feature support. +func (c *Client) Capabilities() gitprovider.ProviderCaps { + return gitprovider.ProviderCaps{ + NativeFileLevelComments: true, + ThreadResolution: true, + BundleInlineOnSubmit: false, + ReviewSummaryAsComment: true, + HeadRefNamespace: "merge-requests", + } +} + +func validateCredential(op gitprovider.Operation, creds gitprovider.Credential) error { + if creds.Type != credentialTypePAT { + return gitprovider.WrapError(gitprovider.ErrAuth, op, fmt.Errorf("unsupported credential type %q", creds.Type)) + } + if strings.TrimSpace(creds.Token) == "" { + return gitprovider.WrapError(gitprovider.ErrAuth, op, fmt.Errorf("credential token is required")) + } + return nil +} + +func (c *Client) validatePRRef(ref gitprovider.PRRef) error { + if c == nil { + return fmt.Errorf("%w: nil client", ErrValidation) + } + if err := ref.Validate(); err != nil { + return err + } + if !strings.EqualFold(ref.Host, c.host) { + return fmt.Errorf("%w: PR host %q does not match client host %q", ErrValidation, ref.Host, c.host) + } + // GitLab namespaces may nest, so owners can contain slashes; every + // segment must still be a plain path element. + if err := validatePathSegments("owner", ref.Owner); err != nil { + return err + } + if strings.Contains(ref.Repo, "/") { + return fmt.Errorf("%w: repo must not contain slash", ErrValidation) + } + if err := validatePathSegments("repo", ref.Repo); err != nil { + return err + } + return nil +} + +func normalizeHost(host string) (string, error) { + host = strings.TrimSpace(host) + if host == "" { + return "", fmt.Errorf("%w: host is required", ErrValidation) + } + if strings.Contains(host, "://") { + parsed, err := url.Parse(host) + if err != nil || parsed.Host == "" { + return "", fmt.Errorf("%w: invalid host %q", ErrValidation, host) + } + host = parsed.Host + } + return strings.ToLower(strings.TrimSuffix(host, "/")), nil +} + +// resolveBaseURL maps a host to its REST v4 base. gitlab.com and self-managed +// instances share the same layout. +func resolveBaseURL(host, baseOverride string) (*url.URL, error) { + base := baseOverride + if base == "" { + base = "https://" + host + "/api/v4/" + } + baseURL, err := url.Parse(base) + if err != nil || baseURL.Scheme == "" || baseURL.Host == "" { + return nil, fmt.Errorf("%w: invalid REST base URL %q", ErrValidation, base) + } + return ensureTrailingSlash(baseURL), nil +} + +func ensureTrailingSlash(u *url.URL) *url.URL { + copied := *u + if !strings.HasSuffix(copied.Path, "/") { + copied.Path += "/" + } + return &copied +} + +func validatePathSegments(name, value string) error { + for _, segment := range strings.Split(value, "/") { + if segment == "" || segment == "." || segment == ".." { + return fmt.Errorf("%w: %s must not contain empty or dot segments", ErrValidation, name) + } + } + return nil +} diff --git a/internal/gitprovider/gitlab/client_test.go b/internal/gitprovider/gitlab/client_test.go new file mode 100644 index 00000000..b9755e78 --- /dev/null +++ b/internal/gitprovider/gitlab/client_test.go @@ -0,0 +1,223 @@ +package gitlab + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/outbox" +) + +var errTokenNotFound = errors.New("token not found") + +func TestClientImplementsGitProvider(_ *testing.T) { + var _ gitprovider.GitProvider = (*Client)(nil) + var _ outbox.Provider = (*Client)(nil) +} + +func TestCapabilities(t *testing.T) { + client := mustClient(t, Options{Token: "token"}) + caps := client.Capabilities() + if !caps.NativeFileLevelComments || !caps.ThreadResolution || caps.BundleInlineOnSubmit { + t.Fatalf("Capabilities() = %#v, want native file comments and thread resolution without bundling", caps) + } + if !caps.ReviewSummaryAsComment { + t.Fatalf("Capabilities() = %#v, want review summaries as comments", caps) + } + if caps.HeadRefNamespace != "merge-requests" { + t.Fatalf("HeadRefNamespace = %q, want merge-requests", caps.HeadRefNamespace) + } +} + +func TestNewFromGitConfigBuildsPATClientAndCredential(t *testing.T) { + store := tokenStore{"work": {credentials.GitTokenKey: "token"}} + client, credential, err := NewFromGitConfig(config.GitConfig{ + Provider: config.GitProviderGitLab, + Host: "gitlab.example.com", + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work"}, + }, store, Options{}) + if err != nil { + t.Fatalf("NewFromGitConfig: %v", err) + } + if client.Host() != "gitlab.example.com" { + t.Fatalf("Host() = %q, want gitlab.example.com", client.Host()) + } + if credential.Type != credentialTypePAT || credential.Token != "token" { + t.Fatalf("credential = %#v, want PAT token", credential) + } + if got := client.baseURL.String(); got != "https://gitlab.example.com/api/v4/" { + t.Fatalf("baseURL = %q, want REST v4 mapping", got) + } +} + +func TestNewFromGitConfigRejectsNonPATAuthModes(t *testing.T) { + store := tokenStore{"work": {credentials.GitTokenKey: "token"}} + for _, mode := range []config.GitAuthMode{config.GitAuthModeGitHubApp, config.GitAuthModeOAuthDevice} { + _, _, err := NewFromGitConfig(config.GitConfig{ + Provider: config.GitProviderGitLab, + Host: "gitlab.example.com", + AuthMode: mode, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work"}, + }, store, Options{}) + if !errors.Is(err, config.ErrUnsupported) { + t.Fatalf("NewFromGitConfig(%q) error = %v, want ErrUnsupported", mode, err) + } + } +} + +func TestNewFromGitConfigRejectsConflictingOptionsHost(t *testing.T) { + store := tokenStore{"work": {credentials.GitTokenKey: "token"}} + _, _, err := NewFromGitConfig(config.GitConfig{ + Host: "gitlab.example.com", + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work"}, + }, store, Options{Host: "gitlab.other.com"}) + if !errors.Is(err, ErrValidation) { + t.Fatalf("NewFromGitConfig error = %v, want ErrValidation", err) + } +} + +func TestHTTPErrorTaxonomy(t *testing.T) { + tests := []struct { + status int + want error + }{ + {status: http.StatusUnauthorized, want: gitprovider.ErrAuth}, + {status: http.StatusForbidden, want: gitprovider.ErrPermission}, + {status: http.StatusNotFound, want: gitprovider.ErrNotFound}, + {status: http.StatusConflict, want: gitprovider.ErrConflict}, + {status: http.StatusBadRequest, want: ErrValidation}, + {status: http.StatusUnprocessableEntity, want: ErrValidation}, + {status: http.StatusTooManyRequests, want: gitprovider.ErrRetryable}, + {status: http.StatusBadGateway, want: gitprovider.ErrRetryable}, + } + for _, tt := range tests { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(`{"message":"secret detail"}`)) + })) + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.GetPR(context.Background(), testPRRef()) + server.Close() + if !errors.Is(err, tt.want) { + t.Errorf("status %d error = %v, want %v", tt.status, err, tt.want) + } + } +} + +func TestRESTHTTPErrorDoesNotLeakSecretBearingBody(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"message":"token glpat-secret is invalid"}`)) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.GetPR(context.Background(), testPRRef()) + if err == nil || strings.Contains(err.Error(), "glpat-secret") { + t.Fatalf("error = %v, want redacted body", err) + } +} + +func TestRESTPaginationRejectsOffHostNextLink(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Link", `; rel="next"`) + writeJSON(t, w, []noteResponse{}) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.ListIssueComments(context.Background(), testPRRef()) + if !errors.Is(err, ErrValidation) { + t.Fatalf("ListIssueComments error = %v, want ErrValidation for off-host pagination", err) + } +} + +func TestValidatePRRefRejectsHostMismatchAndDotSegments(t *testing.T) { + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + requests++ + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + + _, err := client.GetPR(context.Background(), gitprovider.PRRef{Host: "gitlab.other.com", Owner: "group", Repo: "repo", Number: 1}) + if !errors.Is(err, ErrValidation) { + t.Fatalf("GetPR host mismatch error = %v, want ErrValidation", err) + } + _, err = client.GetPR(context.Background(), gitprovider.PRRef{Host: "gitlab.example.com", Owner: "group/../evil", Repo: "repo", Number: 1}) + if !errors.Is(err, ErrValidation) { + t.Fatalf("GetPR dot owner error = %v, want ErrValidation", err) + } + _, err = client.GetPR(context.Background(), gitprovider.PRRef{Host: "gitlab.example.com", Owner: "group", Repo: "re/po", Number: 1}) + if !errors.Is(err, ErrValidation) { + t.Fatalf("GetPR slashed repo error = %v, want ErrValidation", err) + } + if requests != 0 { + t.Fatalf("requests = %d, want no requests for invalid refs", requests) + } +} + +type tokenStore map[string]map[string]string + +func (s tokenStore) Exists(profile, key string) (bool, error) { + keys, ok := s[profile] + if !ok { + return false, nil + } + _, ok = keys[key] + return ok, nil +} + +func (s tokenStore) Get(profile, key string) (string, error) { + keys, ok := s[profile] + if !ok { + return "", errTokenNotFound + } + value, ok := keys[key] + if !ok { + return "", errTokenNotFound + } + return value, nil +} + +func mustClient(t *testing.T, opts Options) *Client { + t.Helper() + if opts.Token == "" { + opts.Token = "token" + } + if opts.Host == "" { + opts.Host = "gitlab.com" + } + client, err := New(opts) + if err != nil { + t.Fatalf("New: %v", err) + } + return client +} + +// testPRRef exercises URL-hostile characters and a nested namespace so path +// escaping stays covered by every wire test. +func testPRRef() gitprovider.PRRef { + return gitprovider.PRRef{Host: "gitlab.example.com", Owner: "open cli/sub group", Repo: "repo+name", Number: 42} +} + +// testProjectPath is testPRRef's project path as GitLab expects it encoded. +func testProjectPath() string { + return url.PathEscape("open cli/sub group/repo+name") +} + +func writeJSON(t *testing.T, w http.ResponseWriter, value any) { + t.Helper() + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(value); err != nil { + t.Fatalf("Encode response: %v", err) + } +} diff --git a/internal/gitprovider/gitlab/discussions.go b/internal/gitprovider/gitlab/discussions.go new file mode 100644 index 00000000..35f96979 --- /dev/null +++ b/internal/gitprovider/gitlab/discussions.go @@ -0,0 +1,310 @@ +package gitlab + +import ( + "context" + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +const ( + positionTypeText = "text" + positionTypeFile = "file" +) + +type positionResponse struct { + BaseSHA string `json:"base_sha"` + StartSHA string `json:"start_sha"` + HeadSHA string `json:"head_sha"` + OldPath string `json:"old_path"` + NewPath string `json:"new_path"` + PositionType string `json:"position_type"` + OldLine *int `json:"old_line"` + NewLine *int `json:"new_line"` +} + +type discussionResponse struct { + ID string `json:"id"` + Notes []noteResponse `json:"notes"` +} + +type positionRequest struct { + BaseSHA string `json:"base_sha"` + StartSHA string `json:"start_sha"` + HeadSHA string `json:"head_sha"` + OldPath string `json:"old_path"` + NewPath string `json:"new_path"` + PositionType string `json:"position_type"` + OldLine int `json:"old_line,omitempty"` + NewLine int `json:"new_line,omitempty"` +} + +type discussionRequest struct { + Body string `json:"body"` + Position *positionRequest `json:"position,omitempty"` +} + +type noteRequest struct { + Body string `json:"body"` +} + +// ListInlineThreads returns merge-request discussions anchored to the diff. +func (c *Client) ListInlineThreads(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.InlineThread, error) { + if err := c.validatePRRef(ref); err != nil { + return nil, err + } + endpoint := withQuery(c.discussionsURL(ref), url.Values{"per_page": {"100"}}) + payloads, err := doRESTPages[discussionResponse](ctx, c, gitprovider.OperationListInlineThreads, endpoint) + if err != nil { + return nil, err + } + threads := make([]gitprovider.InlineThread, 0, len(payloads)) + for _, payload := range payloads { + position := firstPosition(payload.Notes) + if position == nil { + continue + } + thread := gitprovider.InlineThread{ + ID: gitprovider.ThreadID(payload.ID), + Resolved: discussionResolved(payload.Notes), + Path: positionPath(*position), + Side: positionSide(*position), + Line: positionLine(*position), + SubjectType: positionSubjectType(*position), + CommitSHA: position.HeadSHA, + } + for _, note := range payload.Notes { + if note.System { + continue + } + thread.Comments = append(thread.Comments, gitprovider.ThreadComment{ + ID: gitprovider.CommentID(stringIDFromInt(note.ID)), + ThreadID: thread.ID, + Body: note.Body, + Author: identityFromUser(note.Author), + CommitSHA: position.HeadSHA, + Path: thread.Path, + Side: thread.Side, + Line: thread.Line, + SubjectType: thread.SubjectType, + CreatedAt: note.CreatedAt, + UpdatedAt: note.UpdatedAt, + }) + } + threads = append(threads, thread) + } + return threads, nil +} + +// PostInlineComment starts a new diff discussion on the merge request. +func (c *Client) PostInlineComment(ctx context.Context, ref gitprovider.PRRef, comment gitprovider.InlineComment) (gitprovider.CommentID, error) { + op := gitprovider.OperationPostInlineComment + if err := c.validatePRRef(ref); err != nil { + return "", err + } + if err := comment.Validate(); err != nil { + return "", err + } + mr, err := c.getMergeRequest(ctx, op, ref) + if err != nil { + return "", err + } + if err := requireCurrentHead(op, mr, comment.CommitSHA); err != nil { + return "", err + } + position, err := c.buildPosition(ctx, op, ref, mr, comment) + if err != nil { + return "", err + } + var response discussionResponse + if err := c.doRESTJSON(ctx, op, http.MethodPost, c.discussionsURL(ref), discussionRequest{Body: comment.Body, Position: position}, &response); err != nil { + return "", err + } + if len(response.Notes) == 0 || response.Notes[0].ID <= 0 { + return "", fmt.Errorf("%w: discussion note ID missing from successful GitLab response", ErrValidation) + } + return gitprovider.CommentID(stringIDFromInt(response.Notes[0].ID)), nil +} + +// ReplyToThread adds a note to an existing merge-request discussion. +func (c *Client) ReplyToThread(ctx context.Context, ref gitprovider.PRRef, threadID gitprovider.ThreadID, body string) (gitprovider.CommentID, error) { + op := gitprovider.OperationReplyToThread + if err := c.validatePRRef(ref); err != nil { + return "", err + } + if strings.TrimSpace(string(threadID)) == "" { + return "", fmt.Errorf("%w: thread ID is required", ErrValidation) + } + if strings.TrimSpace(body) == "" { + return "", fmt.Errorf("%w: thread reply body is required", ErrValidation) + } + var response noteResponse + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "discussions", url.PathEscape(string(threadID)), "notes") + if err := c.doRESTJSON(ctx, op, http.MethodPost, endpoint, noteRequest{Body: body}, &response); err != nil { + return "", err + } + if response.ID <= 0 { + return "", fmt.Errorf("%w: note ID missing from successful GitLab response", ErrValidation) + } + return gitprovider.CommentID(stringIDFromInt(response.ID)), nil +} + +// ResolveThread marks a merge-request discussion resolved. +func (c *Client) ResolveThread(ctx context.Context, ref gitprovider.PRRef, threadID gitprovider.ThreadID) error { + op := gitprovider.OperationResolveThread + if err := c.validatePRRef(ref); err != nil { + return err + } + if strings.TrimSpace(string(threadID)) == "" { + return fmt.Errorf("%w: thread ID is required", ErrValidation) + } + endpoint := withQuery(restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "discussions", url.PathEscape(string(threadID))), url.Values{"resolved": {"true"}}) + return c.doRESTJSON(ctx, op, http.MethodPut, endpoint, nil, nil) +} + +func (c *Client) discussionsURL(ref gitprovider.PRRef) string { + return restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "discussions") +} + +// buildPosition maps a provider-neutral inline comment onto a GitLab diff-note +// position. Unchanged context lines require both old and new line numbers, so +// the counterpart is computed from the merge request's per-file diff. +func (c *Client) buildPosition(ctx context.Context, op gitprovider.Operation, ref gitprovider.PRRef, mr mergeRequestResponse, comment gitprovider.InlineComment) (*positionRequest, error) { + position := &positionRequest{ + BaseSHA: mr.DiffRefs.BaseSHA, + StartSHA: mr.DiffRefs.StartSHA, + HeadSHA: mr.DiffRefs.HeadSHA, + OldPath: comment.Path, + NewPath: comment.Path, + } + files, err := c.listMergeRequestDiffs(ctx, op, ref) + if err != nil { + return nil, err + } + file, ok := diffFileForPath(files, comment.Path, comment.Side) + if ok { + position.OldPath = file.OldPath + position.NewPath = file.NewPath + if position.OldPath == "" { + position.OldPath = position.NewPath + } + if position.NewPath == "" { + position.NewPath = position.OldPath + } + } + if comment.SubjectType == review.AnchorKindFile { + position.PositionType = positionTypeFile + return position, nil + } + position.PositionType = positionTypeText + switch comment.Side { + case review.DiffSideRight: + position.NewLine = comment.Line + if ok { + if anchor := anchorForNewLine(file.Diff, comment.Line); anchor.found && !anchor.changed { + position.OldLine = anchor.counterpart + } + } + case review.DiffSideLeft: + position.OldLine = comment.Line + if ok { + if anchor := anchorForOldLine(file.Diff, comment.Line); anchor.found && !anchor.changed { + position.NewLine = anchor.counterpart + } + } + default: + return nil, fmt.Errorf("%w: unsupported diff side %q", ErrValidation, comment.Side) + } + return position, nil +} + +func diffFileForPath(files []diffFileResponse, path string, side review.DiffSide) (diffFileResponse, bool) { + for _, file := range files { + if side == review.DiffSideLeft && file.OldPath == path { + return file, true + } + if file.NewPath == path || file.OldPath == path { + return file, true + } + } + return diffFileResponse{}, false +} + +func requireCurrentHead(op gitprovider.Operation, mr mergeRequestResponse, commitSHA string) error { + head := mr.DiffRefs.HeadSHA + if head == "" { + head = mr.SHA + } + if head == "" || strings.EqualFold(head, commitSHA) { + return nil + } + return gitprovider.WrapError(gitprovider.ErrStaleSHA, op, + fmt.Errorf("gitlab: commit %q is not the current merge request head %q", commitSHA, head)) +} + +func firstPosition(notes []noteResponse) *positionResponse { + for _, note := range notes { + if note.Position != nil { + return note.Position + } + } + return nil +} + +func discussionResolved(notes []noteResponse) bool { + resolvable := false + for _, note := range notes { + if !note.Resolvable { + continue + } + resolvable = true + if !note.Resolved { + return false + } + } + return resolvable +} + +func positionPath(position positionResponse) string { + if position.NewLine == nil && position.OldLine != nil && position.OldPath != "" { + return position.OldPath + } + if position.NewPath != "" { + return position.NewPath + } + return position.OldPath +} + +func positionSide(position positionResponse) review.DiffSide { + if position.PositionType == positionTypeFile { + return "" + } + if position.NewLine == nil && position.OldLine != nil { + return review.DiffSideLeft + } + return review.DiffSideRight +} + +func positionLine(position positionResponse) int { + if position.PositionType == positionTypeFile { + return 0 + } + if position.NewLine != nil { + return *position.NewLine + } + if position.OldLine != nil { + return *position.OldLine + } + return 0 +} + +func positionSubjectType(position positionResponse) review.AnchorKind { + if position.PositionType == positionTypeFile { + return review.AnchorKindFile + } + return review.AnchorKindLine +} diff --git a/internal/gitprovider/gitlab/discussions_test.go b/internal/gitprovider/gitlab/discussions_test.go new file mode 100644 index 00000000..6ccfe18a --- /dev/null +++ b/internal/gitprovider/gitlab/discussions_test.go @@ -0,0 +1,279 @@ +package gitlab + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +func TestListInlineThreadsMapsDiffDiscussionsAndPaginates(t *testing.T) { + newLine := 12 + oldLine := 4 + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/merge_requests/42/discussions" { + t.Fatalf("path = %q, want discussions", r.URL.EscapedPath()) + } + if r.URL.Query().Get("page") == "2" { + writeJSON(t, w, []discussionResponse{ + { + ID: "disc-left", + Notes: []noteResponse{{ + ID: 30, Body: "old side", Author: userResponse{Username: "human", ID: 8}, + Resolvable: true, Resolved: false, + Position: &positionResponse{HeadSHA: "headsha", OldPath: "legacy.go", NewPath: "legacy.go", PositionType: "text", OldLine: &oldLine}, + }}, + }, + { + ID: "disc-plain", + Notes: []noteResponse{{ID: 40, Body: "no position note"}}, + }, + }) + return + } + w.Header().Set("Link", "<"+server.URL+r.URL.EscapedPath()+"?page=2&per_page=100>; rel=\"next\"") + writeJSON(t, w, []discussionResponse{ + { + ID: "disc-right", + Notes: []noteResponse{ + { + ID: 10, Body: "finding", Author: userResponse{Username: "review-bot", ID: 7}, + Resolvable: true, Resolved: true, + Position: &positionResponse{HeadSHA: "headsha", OldPath: "main.go", NewPath: "main.go", PositionType: "text", NewLine: &newLine}, + }, + { + ID: 11, Body: "reply", Author: userResponse{Username: "human", ID: 8}, + Resolvable: true, Resolved: true, + }, + {ID: 12, Body: "changed this line", System: true}, + }, + }, + }) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + threads, err := client.ListInlineThreads(context.Background(), testPRRef()) + if err != nil { + t.Fatalf("ListInlineThreads: %v", err) + } + if len(threads) != 2 { + t.Fatalf("threads = %#v, want two diff discussions", threads) + } + first := threads[0] + if first.ID != gitprovider.ThreadID("disc-right") || !first.Resolved || first.Path != "main.go" || + first.Side != review.DiffSideRight || first.Line != 12 || first.SubjectType != review.AnchorKindLine || first.CommitSHA != "headsha" { + t.Fatalf("thread = %#v, want mapped right-side thread", first) + } + if len(first.Comments) != 2 { + t.Fatalf("comments = %#v, want system notes excluded", first.Comments) + } + if first.Comments[0].ID != gitprovider.CommentID("10") || first.Comments[0].ThreadID != first.ID || + first.Comments[0].Author.Login != "review-bot" || first.Comments[0].Line != 12 { + t.Fatalf("comment = %#v, want mapped first note", first.Comments[0]) + } + second := threads[1] + if second.ID != gitprovider.ThreadID("disc-left") || second.Resolved || second.Side != review.DiffSideLeft || + second.Line != 4 || second.Path != "legacy.go" { + t.Fatalf("thread = %#v, want mapped left-side thread", second) + } +} + +func TestPostInlineCommentBuildsTextPosition(t *testing.T) { + tests := []struct { + name string + comment gitprovider.InlineComment + diff string + wantOldLine int + wantNewLine int + }{ + { + name: "added line posts new line only", + comment: gitprovider.InlineComment{ + CommitSHA: "headsha", Body: "finding", Path: "main.go", + Side: review.DiffSideRight, Line: 2, SubjectType: review.AnchorKindLine, + }, + diff: "@@ -1,2 +1,3 @@\n context\n+added\n context2\n", + wantNewLine: 2, + }, + { + name: "context line posts both sides", + comment: gitprovider.InlineComment{ + CommitSHA: "headsha", Body: "finding", Path: "main.go", + Side: review.DiffSideRight, Line: 3, SubjectType: review.AnchorKindLine, + }, + diff: "@@ -1,2 +1,3 @@\n context\n+added\n context2\n", + wantOldLine: 2, + wantNewLine: 3, + }, + { + name: "deleted line posts old line only", + comment: gitprovider.InlineComment{ + CommitSHA: "headsha", Body: "finding", Path: "main.go", + Side: review.DiffSideLeft, Line: 2, SubjectType: review.AnchorKindLine, + }, + diff: "@@ -1,2 +1,1 @@\n context\n-removed\n", + wantOldLine: 2, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var posted positionRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42" && r.Method == http.MethodGet: + writeJSON(t, w, mergeRequestResponse{ + State: "opened", SourceBranch: "feature", TargetBranch: "main", + DiffRefs: diffRefsResponse{BaseSHA: "basesha", StartSHA: "startsha", HeadSHA: "headsha"}, + }) + case r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/diffs": + writeJSON(t, w, []diffFileResponse{{OldPath: "main.go", NewPath: "main.go", Diff: tt.diff}}) + case r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/discussions" && r.Method == http.MethodPost: + var request discussionRequest + decodeJSON(t, r.Body, &request) + if request.Position == nil { + t.Fatal("position missing from discussion request") + } + posted = *request.Position + writeJSON(t, w, discussionResponse{ID: "disc-1", Notes: []noteResponse{{ID: 99}}}) + default: + t.Fatalf("unexpected request %s %q", r.Method, r.URL.EscapedPath()) + } + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + id, err := client.PostInlineComment(context.Background(), testPRRef(), tt.comment) + if err != nil { + t.Fatalf("PostInlineComment: %v", err) + } + if id != gitprovider.CommentID("99") { + t.Fatalf("id = %q, want 99", id) + } + if posted.BaseSHA != "basesha" || posted.StartSHA != "startsha" || posted.HeadSHA != "headsha" { + t.Fatalf("position = %#v, want diff refs from merge request", posted) + } + if posted.PositionType != "text" || posted.OldPath != "main.go" || posted.NewPath != "main.go" { + t.Fatalf("position = %#v, want text position on main.go", posted) + } + if posted.OldLine != tt.wantOldLine || posted.NewLine != tt.wantNewLine { + t.Fatalf("position lines = old %d new %d, want old %d new %d", posted.OldLine, posted.NewLine, tt.wantOldLine, tt.wantNewLine) + } + }) + } +} + +func TestPostInlineCommentBuildsFilePositionWithRenamePaths(t *testing.T) { + var posted positionRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42" && r.Method == http.MethodGet: + writeJSON(t, w, mergeRequestResponse{ + State: "opened", SourceBranch: "feature", TargetBranch: "main", + DiffRefs: diffRefsResponse{BaseSHA: "basesha", StartSHA: "startsha", HeadSHA: "headsha"}, + }) + case r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/diffs": + writeJSON(t, w, []diffFileResponse{{OldPath: "old-name.go", NewPath: "new-name.go", RenamedFile: true}}) + case r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/discussions" && r.Method == http.MethodPost: + var request discussionRequest + decodeJSON(t, r.Body, &request) + posted = *request.Position + writeJSON(t, w, discussionResponse{ID: "disc-1", Notes: []noteResponse{{ID: 99}}}) + default: + t.Fatalf("unexpected request %s %q", r.Method, r.URL.EscapedPath()) + } + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.PostInlineComment(context.Background(), testPRRef(), gitprovider.InlineComment{ + CommitSHA: "headsha", Body: "file finding", Path: "new-name.go", SubjectType: review.AnchorKindFile, + }) + if err != nil { + t.Fatalf("PostInlineComment: %v", err) + } + if posted.PositionType != "file" || posted.OldPath != "old-name.go" || posted.NewPath != "new-name.go" { + t.Fatalf("position = %#v, want file position with rename paths", posted) + } + if posted.OldLine != 0 || posted.NewLine != 0 { + t.Fatalf("position = %#v, want no lines on file position", posted) + } +} + +func TestPostInlineCommentRejectsStaleCommit(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + t.Fatalf("unexpected write %s %q", r.Method, r.URL.EscapedPath()) + } + writeJSON(t, w, mergeRequestResponse{ + State: "opened", SourceBranch: "feature", TargetBranch: "main", + DiffRefs: diffRefsResponse{BaseSHA: "basesha", StartSHA: "startsha", HeadSHA: "newer-head"}, + }) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.PostInlineComment(context.Background(), testPRRef(), gitprovider.InlineComment{ + CommitSHA: "stale-head", Body: "finding", Path: "main.go", + Side: review.DiffSideRight, Line: 2, SubjectType: review.AnchorKindLine, + }) + if !errors.Is(err, gitprovider.ErrStaleSHA) { + t.Fatalf("PostInlineComment error = %v, want ErrStaleSHA", err) + } +} + +func TestReplyToThreadPostsDiscussionNote(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/merge_requests/42/discussions/disc-1/notes" { + t.Fatalf("unexpected request %s %q", r.Method, r.URL.EscapedPath()) + } + var request noteRequest + decodeJSON(t, r.Body, &request) + if request.Body != "reply body" { + t.Fatalf("body = %q, want reply body", request.Body) + } + writeJSON(t, w, noteResponse{ID: 55}) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + id, err := client.ReplyToThread(context.Background(), testPRRef(), gitprovider.ThreadID("disc-1"), "reply body") + if err != nil { + t.Fatalf("ReplyToThread: %v", err) + } + if id != gitprovider.CommentID("55") { + t.Fatalf("id = %q, want 55", id) + } +} + +func TestResolveThreadPutsResolved(t *testing.T) { + resolved := "" + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPut || r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/merge_requests/42/discussions/disc-1" { + t.Fatalf("unexpected request %s %q", r.Method, r.URL.EscapedPath()) + } + resolved = r.URL.Query().Get("resolved") + writeJSON(t, w, discussionResponse{ID: "disc-1"}) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + if err := client.ResolveThread(context.Background(), testPRRef(), gitprovider.ThreadID("disc-1")); err != nil { + t.Fatalf("ResolveThread: %v", err) + } + if resolved != "true" { + t.Fatalf("resolved = %q, want true", resolved) + } +} + +func decodeJSON(t *testing.T, body io.Reader, out any) { + t.Helper() + payload, err := io.ReadAll(body) + if err != nil { + t.Fatalf("ReadAll: %v", err) + } + if err := json.Unmarshal(payload, out); err != nil { + t.Fatalf("Unmarshal %s: %v", payload, err) + } +} diff --git a/internal/gitprovider/gitlab/http.go b/internal/gitprovider/gitlab/http.go new file mode 100644 index 00000000..283636d6 --- /dev/null +++ b/internal/gitprovider/gitlab/http.go @@ -0,0 +1,221 @@ +package gitlab + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +const ( + acceptJSON = "application/json" + acceptAny = "*/*" +) + +func (c *Client) doREST(ctx context.Context, op gitprovider.Operation, method, endpoint, accept string, out any) ([]byte, http.Header, error) { + return c.doRESTWithToken(ctx, op, method, endpoint, c.token, accept, out) +} + +func (c *Client) doRESTWithToken(ctx context.Context, op gitprovider.Operation, method, endpoint, token, accept string, out any) ([]byte, http.Header, error) { + req, err := http.NewRequestWithContext(ctx, method, endpoint, nil) + if err != nil { + return nil, nil, err + } + setHeaders(req, token, accept) + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, nil, mapTransportError(op, err) + } + defer resp.Body.Close() + body, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return nil, nil, gitprovider.WrapError(gitprovider.ErrRetryable, op, readErr) + } + if resp.StatusCode < 200 || resp.StatusCode > 299 { + return nil, resp.Header, mapHTTPStatus(op, resp.StatusCode, body) + } + if out != nil { + if err := json.Unmarshal(body, out); err != nil { + return nil, resp.Header, fmt.Errorf("%w: decode GitLab response: %w", ErrValidation, err) + } + } + return body, resp.Header, nil +} + +func (c *Client) doRESTJSON(ctx context.Context, op gitprovider.Operation, method, endpoint string, in any, out any) error { + var reader io.Reader + if in != nil { + payload, err := json.Marshal(in) + if err != nil { + return err + } + reader = bytes.NewReader(payload) + } + req, err := http.NewRequestWithContext(ctx, method, endpoint, reader) + if err != nil { + return err + } + setHeaders(req, c.token, acceptJSON) + if in != nil { + req.Header.Set("Content-Type", "application/json") + } + resp, err := c.httpClient.Do(req) + if err != nil { + return mapTransportError(op, err) + } + defer resp.Body.Close() + body, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return gitprovider.WrapError(gitprovider.ErrRetryable, op, readErr) + } + if resp.StatusCode < 200 || resp.StatusCode > 299 { + return mapHTTPStatus(op, resp.StatusCode, body) + } + if out != nil { + if err := json.Unmarshal(body, out); err != nil { + return fmt.Errorf("%w: decode GitLab response: %w", ErrValidation, err) + } + } + return nil +} + +func doRESTPages[T any](ctx context.Context, c *Client, op gitprovider.Operation, endpoint string) ([]T, error) { + var all []T + next := endpoint + for next != "" { + var page []T + _, header, err := c.doREST(ctx, op, http.MethodGet, next, acceptJSON, &page) + if err != nil { + return nil, err + } + all = append(all, page...) + next, err = c.nextPageURL(header.Get("Link")) + if err != nil { + return nil, err + } + } + return all, nil +} + +func setHeaders(req *http.Request, token, accept string) { + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Accept", accept) + req.Header.Set("User-Agent", "codereview-cli") +} + +func mapTransportError(op gitprovider.Operation, err error) error { + if errors.Is(err, context.Canceled) { + return context.Canceled + } + return gitprovider.WrapError(gitprovider.ErrRetryable, op, err) +} + +func mapHTTPStatus(op gitprovider.Operation, status int, body []byte) error { + err := httpStatusError(status, body) + switch status { + case http.StatusUnauthorized: + return gitprovider.WrapError(gitprovider.ErrAuth, op, err) + case http.StatusForbidden: + return gitprovider.WrapError(gitprovider.ErrPermission, op, err) + case http.StatusNotFound: + return gitprovider.WrapError(gitprovider.ErrNotFound, op, err) + case http.StatusConflict: + return gitprovider.WrapError(gitprovider.ErrConflict, op, err) + case http.StatusBadRequest, http.StatusUnprocessableEntity: + return fmt.Errorf("%w: %w", ErrValidation, err) + case http.StatusTooManyRequests: + return gitprovider.WrapError(gitprovider.ErrRetryable, op, err) + default: + if status >= 500 && status <= 599 { + return gitprovider.WrapError(gitprovider.ErrRetryable, op, err) + } + return fmt.Errorf("%w: %w", ErrValidation, err) + } +} + +func httpStatusError(status int, body []byte) error { + body = bytes.TrimSpace(body) + if len(body) == 0 { + return fmt.Errorf("gitlab: status %d", status) + } + return fmt.Errorf("gitlab: status %d (response body redacted)", status) +} + +// restURL joins pre-escaped path parts onto the REST base. Parts must already +// be percent-encoded (for example via url.PathEscape) because GitLab encodes +// whole project paths and file paths as single path segments. +func restURL(base *url.URL, parts ...string) string { + joined := strings.TrimSuffix(base.String(), "/") + for _, part := range parts { + joined += "/" + part + } + return joined +} + +func projectSegment(ref gitprovider.PRRef) string { + return url.PathEscape(ref.Owner + "/" + ref.Repo) +} + +func withQuery(endpoint string, values url.Values) string { + if len(values) == 0 { + return endpoint + } + return endpoint + "?" + values.Encode() +} + +func (c *Client) nextPageURL(header string) (string, error) { + next := nextLink(header) + if next == "" { + return "", nil + } + parsed, err := url.Parse(next) + if err != nil { + return "", fmt.Errorf("%w: invalid pagination URL", ErrValidation) + } + if !parsed.IsAbs() { + parsed = c.baseURL.ResolveReference(parsed) + } + if parsed.Scheme != c.baseURL.Scheme || !strings.EqualFold(parsed.Host, c.baseURL.Host) { + return "", fmt.Errorf("%w: pagination URL host does not match GitLab REST host", ErrValidation) + } + basePath := c.baseURL.EscapedPath() + if basePath == "" { + basePath = "/" + } + if basePath != "/" && !strings.HasPrefix(parsed.EscapedPath(), basePath) { + return "", fmt.Errorf("%w: pagination URL path escapes GitLab REST base", ErrValidation) + } + return parsed.String(), nil +} + +func nextLink(header string) string { + for _, part := range strings.Split(header, ",") { + pieces := strings.Split(part, ";") + if len(pieces) < 2 { + continue + } + if !strings.Contains(pieces[1], `rel="next"`) { + continue + } + link := strings.TrimSpace(pieces[0]) + link = strings.TrimPrefix(link, "<") + link = strings.TrimSuffix(link, ">") + return link + } + return "" +} + +func stringIDFromInt(id int64) string { + if id == 0 { + return "" + } + return strconv.FormatInt(id, 10) +} diff --git a/internal/gitprovider/gitlab/imports_test.go b/internal/gitprovider/gitlab/imports_test.go new file mode 100644 index 00000000..ba503ca7 --- /dev/null +++ b/internal/gitprovider/gitlab/imports_test.go @@ -0,0 +1,92 @@ +package gitlab + +import ( + "bytes" + "go/parser" + "go/token" + "io/fs" + "os/exec" + "path/filepath" + "runtime" + "strconv" + "strings" + "testing" +) + +func TestProductionImportsStayInAdapterLayer(t *testing.T) { + _, testFile, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + dir := filepath.Dir(testFile) + repoRoot, modulePath := repoRootAndModule(t, dir) + allowedInternal := map[string]struct{}{ + modulePath + "/internal/config": {}, + modulePath + "/internal/credentials": {}, + modulePath + "/internal/gitprovider": {}, + modulePath + "/internal/review": {}, + } + stdlib := stdlibImports(t, repoRoot) + fset := token.NewFileSet() + err := filepath.WalkDir(dir, func(file string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.IsDir() || filepath.Ext(file) != ".go" || strings.HasSuffix(file, "_test.go") { + return nil + } + parsed, err := parser.ParseFile(fset, file, nil, parser.ImportsOnly) + if err != nil { + return err + } + for _, imported := range parsed.Imports { + path, err := strconv.Unquote(imported.Path.Value) + if err != nil { + return err + } + if _, ok := allowedInternal[path]; ok { + continue + } + if strings.HasPrefix(path, modulePath+"/") || path == modulePath { + t.Fatalf("production import %q is outside allowed adapter-layer imports", path) + } + if _, ok := stdlib[path]; !ok { + t.Fatalf("production import %q is not in the standard library", path) + } + } + return nil + }) + if err != nil { + t.Fatalf("WalkDir(%s): %v", dir, err) + } +} + +func repoRootAndModule(t *testing.T, dir string) (string, string) { + t.Helper() + cmd := exec.Command("go", "list", "-m", "-f", "{{.Dir}} {{.Path}}") + cmd.Dir = dir + output, err := cmd.Output() + if err != nil { + t.Fatalf("go list module: %v", err) + } + parts := strings.Fields(string(output)) + if len(parts) != 2 { + t.Fatalf("go list module output = %q, want dir and path", output) + } + return parts[0], parts[1] +} + +func stdlibImports(t *testing.T, repoRoot string) map[string]struct{} { + t.Helper() + cmd := exec.Command("go", "list", "std") + cmd.Dir = repoRoot + output, err := cmd.Output() + if err != nil { + t.Fatalf("go list std: %v", err) + } + imports := make(map[string]struct{}) + for _, path := range bytes.Fields(output) { + imports[string(path)] = struct{}{} + } + return imports +} diff --git a/internal/gitprovider/gitlab/linemap.go b/internal/gitprovider/gitlab/linemap.go new file mode 100644 index 00000000..8c833d0a --- /dev/null +++ b/internal/gitprovider/gitlab/linemap.go @@ -0,0 +1,100 @@ +package gitlab + +import ( + "strconv" + "strings" +) + +// lineAnchor classifies one target line of a per-file diff and carries the +// counterpart line number on the other diff side when the line is unchanged +// context. GitLab diff-note positions require both sides for context lines. +type lineAnchor struct { + found bool + changed bool + counterpart int +} + +// anchorForNewLine locates 1-based line number on the new side of a per-file +// unified diff fragment (hunks only, as served by the GitLab diffs API). +func anchorForNewLine(diff string, line int) lineAnchor { + return anchorForLine(diff, line, false) +} + +// anchorForOldLine locates a 1-based line number on the old side. +func anchorForOldLine(diff string, line int) lineAnchor { + return anchorForLine(diff, line, true) +} + +func anchorForLine(diff string, target int, oldSide bool) lineAnchor { + lines := strings.Split(diff, "\n") + if len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + oldLine, newLine := 0, 0 + inHunk := false + for _, raw := range lines { + if strings.HasPrefix(raw, "@@") { + oldStart, newStart, ok := parseHunkHeader(raw) + if !ok { + return lineAnchor{} + } + oldLine, newLine = oldStart, newStart + inHunk = true + continue + } + if !inHunk { + continue + } + switch { + case strings.HasPrefix(raw, "+"): + if !oldSide && newLine == target { + return lineAnchor{found: true, changed: true} + } + newLine++ + case strings.HasPrefix(raw, "-"): + if oldSide && oldLine == target { + return lineAnchor{found: true, changed: true} + } + oldLine++ + case strings.HasPrefix(raw, "\\"): + // "\ No newline at end of file" advances neither side. + default: + if oldSide && oldLine == target { + return lineAnchor{found: true, counterpart: newLine} + } + if !oldSide && newLine == target { + return lineAnchor{found: true, counterpart: oldLine} + } + oldLine++ + newLine++ + } + } + return lineAnchor{} +} + +func parseHunkHeader(header string) (oldStart, newStart int, ok bool) { + fields := strings.Fields(header) + if len(fields) < 3 || !strings.HasPrefix(fields[1], "-") || !strings.HasPrefix(fields[2], "+") { + return 0, 0, false + } + oldStart, ok = parseHunkStart(strings.TrimPrefix(fields[1], "-")) + if !ok { + return 0, 0, false + } + newStart, ok = parseHunkStart(strings.TrimPrefix(fields[2], "+")) + if !ok { + return 0, 0, false + } + return oldStart, newStart, true +} + +func parseHunkStart(spec string) (int, bool) { + if comma := strings.Index(spec, ","); comma >= 0 { + spec = spec[:comma] + } + start, err := strconv.Atoi(spec) + if err != nil || start < 0 { + return 0, false + } + return start, true +} diff --git a/internal/gitprovider/gitlab/linemap_test.go b/internal/gitprovider/gitlab/linemap_test.go new file mode 100644 index 00000000..328ba4b1 --- /dev/null +++ b/internal/gitprovider/gitlab/linemap_test.go @@ -0,0 +1,61 @@ +package gitlab + +import "testing" + +const sampleDiff = "@@ -1,4 +1,5 @@\n context1\n-removed\n+added1\n+added2\n context2\n@@ -10,2 +11,2 @@\n context3\n-old-tail\n+new-tail\n" + +func TestAnchorForNewLine(t *testing.T) { + tests := []struct { + name string + line int + found bool + changed bool + old int + }{ + {name: "leading context", line: 1, found: true, old: 1}, + {name: "first added", line: 2, found: true, changed: true}, + {name: "second added", line: 3, found: true, changed: true}, + {name: "trailing context", line: 4, found: true, old: 3}, + {name: "second hunk context", line: 11, found: true, old: 10}, + {name: "second hunk added", line: 12, found: true, changed: true}, + {name: "outside hunks", line: 8, found: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + anchor := anchorForNewLine(sampleDiff, tt.line) + if anchor.found != tt.found || anchor.changed != tt.changed || anchor.counterpart != tt.old { + t.Fatalf("anchorForNewLine(%d) = %#v, want found=%v changed=%v counterpart=%d", tt.line, anchor, tt.found, tt.changed, tt.old) + } + }) + } +} + +func TestAnchorForOldLine(t *testing.T) { + tests := []struct { + name string + line int + found bool + changed bool + new int + }{ + {name: "leading context", line: 1, found: true, new: 1}, + {name: "removed", line: 2, found: true, changed: true}, + {name: "trailing context", line: 3, found: true, new: 4}, + {name: "second hunk removed", line: 11, found: true, changed: true}, + {name: "outside hunks", line: 7, found: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + anchor := anchorForOldLine(sampleDiff, tt.line) + if anchor.found != tt.found || anchor.changed != tt.changed || anchor.counterpart != tt.new { + t.Fatalf("anchorForOldLine(%d) = %#v, want found=%v changed=%v counterpart=%d", tt.line, anchor, tt.found, tt.changed, tt.new) + } + }) + } +} + +func TestAnchorHandlesMalformedHunkHeader(t *testing.T) { + if anchor := anchorForNewLine("@@ garbage @@\n+x\n", 1); anchor.found { + t.Fatalf("anchor = %#v, want not found for malformed header", anchor) + } +} diff --git a/internal/gitprovider/gitlab/rest_reads.go b/internal/gitprovider/gitlab/rest_reads.go new file mode 100644 index 00000000..b8a2a046 --- /dev/null +++ b/internal/gitprovider/gitlab/rest_reads.go @@ -0,0 +1,421 @@ +package gitlab + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/url" + "strings" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +type userResponse struct { + Username string `json:"username"` + ID int64 `json:"id"` + Name string `json:"name"` +} + +type diffRefsResponse struct { + BaseSHA string `json:"base_sha"` + StartSHA string `json:"start_sha"` + HeadSHA string `json:"head_sha"` +} + +type mergeRequestResponse struct { + IID int64 `json:"iid"` + Title string `json:"title"` + Description string `json:"description"` + State string `json:"state"` + WebURL string `json:"web_url"` + Author userResponse `json:"author"` + SourceBranch string `json:"source_branch"` + TargetBranch string `json:"target_branch"` + SHA string `json:"sha"` + DiffRefs diffRefsResponse `json:"diff_refs"` + SourceProjectID int64 `json:"source_project_id"` + TargetProjectID int64 `json:"target_project_id"` +} + +type projectResponse struct { + PathWithNamespace string `json:"path_with_namespace"` +} + +type diffFileResponse struct { + OldPath string `json:"old_path"` + NewPath string `json:"new_path"` + NewFile bool `json:"new_file"` + RenamedFile bool `json:"renamed_file"` + DeletedFile bool `json:"deleted_file"` + Diff string `json:"diff"` +} + +type approvalsResponse struct { + ApprovedBy []struct { + User userResponse `json:"user"` + } `json:"approved_by"` +} + +type noteResponse struct { + ID int64 `json:"id"` + Type string `json:"type"` + Body string `json:"body"` + Author userResponse `json:"author"` + System bool `json:"system"` + Resolvable bool `json:"resolvable"` + Resolved bool `json:"resolved"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + Position *positionResponse `json:"position"` +} + +type treeEntryResponse struct { + ID string `json:"id"` + Type string `json:"type"` + Path string `json:"path"` +} + +// WhoAmI returns the identity for the supplied credential. +func (c *Client) WhoAmI(ctx context.Context, creds gitprovider.Credential) (gitprovider.Identity, error) { + if err := validateCredential(gitprovider.OperationWhoAmI, creds); err != nil { + return gitprovider.Identity{}, err + } + var user userResponse + _, _, err := c.doRESTWithToken(ctx, gitprovider.OperationWhoAmI, http.MethodGet, restURL(c.baseURL, "user"), creds.Token, acceptJSON, &user) + if err != nil { + return gitprovider.Identity{}, err + } + return identityFromUser(user), nil +} + +// GetPR returns one merge request snapshot. +func (c *Client) GetPR(ctx context.Context, ref gitprovider.PRRef) (gitprovider.PR, error) { + if err := c.validatePRRef(ref); err != nil { + return gitprovider.PR{}, err + } + payload, err := c.getMergeRequest(ctx, gitprovider.OperationGetPR, ref) + if err != nil { + return gitprovider.PR{}, err + } + state, err := prState(payload.State) + if err != nil { + return gitprovider.PR{}, err + } + headOwner, headRepo := ref.Owner, ref.Repo + if payload.SourceProjectID != 0 && payload.SourceProjectID != payload.TargetProjectID { + owner, repo, err := c.projectPath(ctx, gitprovider.OperationGetPR, payload.SourceProjectID) + if err != nil { + return gitprovider.PR{}, err + } + headOwner, headRepo = owner, repo + } + headSHA := payload.DiffRefs.HeadSHA + if headSHA == "" { + headSHA = payload.SHA + } + return gitprovider.PR{ + Ref: ref, + Title: payload.Title, + Body: payload.Description, + URL: payload.WebURL, + State: state, + Author: identityFromUser(payload.Author), + Head: gitprovider.PRBranchRef{ + Host: c.host, + Owner: headOwner, + Repo: headRepo, + Name: payload.SourceBranch, + Ref: "refs/heads/" + payload.SourceBranch, + SHA: headSHA, + }, + Base: gitprovider.PRBranchRef{ + Host: c.host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: payload.TargetBranch, + Ref: "refs/heads/" + payload.TargetBranch, + SHA: payload.DiffRefs.BaseSHA, + }, + }, nil +} + +func (c *Client) getMergeRequest(ctx context.Context, op gitprovider.Operation, ref gitprovider.PRRef) (mergeRequestResponse, error) { + var payload mergeRequestResponse + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number)) + if _, _, err := c.doREST(ctx, op, http.MethodGet, endpoint, acceptJSON, &payload); err != nil { + return mergeRequestResponse{}, err + } + return payload, nil +} + +func (c *Client) projectPath(ctx context.Context, op gitprovider.Operation, projectID int64) (owner, repo string, err error) { + var payload projectResponse + endpoint := restURL(c.baseURL, "projects", fmt.Sprint(projectID)) + if _, _, err := c.doREST(ctx, op, http.MethodGet, endpoint, acceptJSON, &payload); err != nil { + return "", "", err + } + full := strings.Trim(payload.PathWithNamespace, "/") + slash := strings.LastIndex(full, "/") + if slash <= 0 || slash == len(full)-1 { + return "", "", fmt.Errorf("%w: unexpected project path %q", ErrValidation, payload.PathWithNamespace) + } + return full[:slash], full[slash+1:], nil +} + +// GetDiff returns the raw unified diff for a merge request. Newer GitLab +// versions serve it directly; older ones fall back to reconstruction from the +// per-file diffs listing. +func (c *Client) GetDiff(ctx context.Context, ref gitprovider.PRRef) (gitprovider.UnifiedDiff, error) { + if err := c.validatePRRef(ref); err != nil { + return gitprovider.UnifiedDiff{}, err + } + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "raw_diffs") + body, _, err := c.doREST(ctx, gitprovider.OperationGetDiff, http.MethodGet, endpoint, acceptAny, nil) + if err != nil { + // GitLab instances older than 15.7 do not serve raw_diffs; the + // per-file diffs listing still covers them. + if errors.Is(err, gitprovider.ErrNotFound) { + return c.reconstructMergeRequestDiff(ctx, ref) + } + return gitprovider.UnifiedDiff{}, err + } + return gitprovider.UnifiedDiff{Raw: string(body)}, nil +} + +func (c *Client) reconstructMergeRequestDiff(ctx context.Context, ref gitprovider.PRRef) (gitprovider.UnifiedDiff, error) { + files, err := c.listMergeRequestDiffs(ctx, gitprovider.OperationGetDiff, ref) + if err != nil { + return gitprovider.UnifiedDiff{}, err + } + raw := reconstructUnifiedDiff(files) + if raw == "" { + return gitprovider.UnifiedDiff{}, gitprovider.WrapError(gitprovider.ErrNotFound, gitprovider.OperationGetDiff, + errors.New("gitlab: merge request diff unavailable")) + } + return gitprovider.UnifiedDiff{Raw: raw}, nil +} + +func (c *Client) listMergeRequestDiffs(ctx context.Context, op gitprovider.Operation, ref gitprovider.PRRef) ([]diffFileResponse, error) { + endpoint := withQuery(restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "diffs"), url.Values{"per_page": {"100"}}) + return doRESTPages[diffFileResponse](ctx, c, op, endpoint) +} + +// GetDiffBetweenRefs returns the raw unified diff between two git refs in the +// merge request's target repository. +func (c *Client) GetDiffBetweenRefs(ctx context.Context, ref gitprovider.PRRef, baseSHA, headSHA string) (gitprovider.UnifiedDiff, error) { + if err := c.validatePRRef(ref); err != nil { + return gitprovider.UnifiedDiff{}, err + } + baseSHA = strings.TrimSpace(baseSHA) + headSHA = strings.TrimSpace(headSHA) + if baseSHA == "" { + return gitprovider.UnifiedDiff{}, fmt.Errorf("%w: base SHA is required", ErrValidation) + } + if headSHA == "" { + return gitprovider.UnifiedDiff{}, fmt.Errorf("%w: head SHA is required", ErrValidation) + } + var payload struct { + Diffs []diffFileResponse `json:"diffs"` + } + endpoint := withQuery(restURL(c.baseURL, "projects", projectSegment(ref), "repository", "compare"), url.Values{ + "from": {baseSHA}, + "to": {headSHA}, + }) + if _, _, err := c.doREST(ctx, gitprovider.OperationGetDiffBetweenRefs, http.MethodGet, endpoint, acceptJSON, &payload); err != nil { + return gitprovider.UnifiedDiff{}, err + } + return gitprovider.UnifiedDiff{Raw: reconstructUnifiedDiff(payload.Diffs)}, nil +} + +// reconstructUnifiedDiff synthesizes standard git-format unified diff text +// from GitLab per-file diffs, which carry hunks without the "diff --git" +// header lines. +func reconstructUnifiedDiff(files []diffFileResponse) string { + var b strings.Builder + for _, f := range files { + newPath := f.NewPath + oldPath := f.OldPath + if newPath == "" && oldPath == "" { + continue + } + if newPath == "" { + newPath = oldPath + } + if oldPath == "" { + oldPath = newPath + } + fmt.Fprintf(&b, "diff --git a/%s b/%s\n", oldPath, newPath) + if f.RenamedFile { + fmt.Fprintf(&b, "rename from %s\n", oldPath) + fmt.Fprintf(&b, "rename to %s\n", newPath) + } + diff := f.Diff + if strings.TrimSpace(diff) == "" { + // A pure rename carries no hunks and is already fully described. + // Anything else without hunks is binary or collapsed as too + // large; keep the change visible with a header-only entry. + if !f.RenamedFile { + fmt.Fprintf(&b, "Binary files a/%s and b/%s differ\n", oldPath, newPath) + } + continue + } + switch { + case f.NewFile: + b.WriteString("--- /dev/null\n") + fmt.Fprintf(&b, "+++ b/%s\n", newPath) + case f.DeletedFile: + fmt.Fprintf(&b, "--- a/%s\n", oldPath) + b.WriteString("+++ /dev/null\n") + default: + fmt.Fprintf(&b, "--- a/%s\n", oldPath) + fmt.Fprintf(&b, "+++ b/%s\n", newPath) + } + b.WriteString(diff) + if !strings.HasSuffix(diff, "\n") { + b.WriteByte('\n') + } + } + return b.String() +} + +// GetFileAtRef returns raw file contents at a git ref. +func (c *Client) GetFileAtRef(ctx context.Context, ref gitprovider.PRRef, gitRef string, filePath string) ([]byte, error) { + if err := c.validatePRRef(ref); err != nil { + return nil, err + } + if strings.TrimSpace(gitRef) == "" { + return nil, fmt.Errorf("%w: git ref is required", ErrValidation) + } + normalized, err := validateFilePath(filePath) + if err != nil { + return nil, err + } + endpoint := withQuery(restURL(c.baseURL, "projects", projectSegment(ref), "repository", "files", url.PathEscape(normalized), "raw"), url.Values{"ref": {gitRef}}) + body, _, err := c.doREST(ctx, gitprovider.OperationGetFileAtRef, http.MethodGet, endpoint, acceptAny, nil) + if err != nil { + return nil, err + } + return body, nil +} + +// ListTreeAtRef returns the git tree entries at a ref and path. +func (c *Client) ListTreeAtRef(ctx context.Context, ref gitprovider.PRRef, gitRef string, treePath string) ([]gitprovider.TreeEntry, error) { + if err := c.validatePRRef(ref); err != nil { + return nil, err + } + if strings.TrimSpace(gitRef) == "" { + return nil, fmt.Errorf("%w: git ref is required", ErrValidation) + } + values := url.Values{"ref": {gitRef}, "per_page": {"100"}} + if trimmed := strings.Trim(strings.TrimSpace(treePath), "/"); trimmed != "" { + values.Set("path", trimmed) + } + endpoint := withQuery(restURL(c.baseURL, "projects", projectSegment(ref), "repository", "tree"), values) + payloads, err := doRESTPages[treeEntryResponse](ctx, c, gitprovider.OperationListTreeAtRef, endpoint) + if err != nil { + return nil, err + } + entries := make([]gitprovider.TreeEntry, 0, len(payloads)) + for _, payload := range payloads { + entries = append(entries, gitprovider.TreeEntry{ + Path: payload.Path, + Type: payload.Type, + SHA: payload.ID, + }) + } + return entries, nil +} + +// ListReviews maps current merge-request approvals to provider-neutral +// reviews. GitLab has no first-class review object: approvals are current +// state (revoking removes them), and review summaries are posted as notes. +func (c *Client) ListReviews(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.Review, error) { + if err := c.validatePRRef(ref); err != nil { + return nil, err + } + var payload approvalsResponse + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "approvals") + if _, _, err := c.doREST(ctx, gitprovider.OperationListReviews, http.MethodGet, endpoint, acceptJSON, &payload); err != nil { + return nil, err + } + reviews := make([]gitprovider.Review, 0, len(payload.ApprovedBy)) + for _, approval := range payload.ApprovedBy { + reviews = append(reviews, gitprovider.Review{ + ID: gitprovider.ReviewID("approval-" + stringIDFromInt(approval.User.ID)), + Author: identityFromUser(approval.User), + State: gitprovider.ReviewStateApproved, + Event: review.ReviewEventApprove, + }) + } + return reviews, nil +} + +// ListIssueComments returns non-system merge-request notes without a diff +// position, GitLab's equivalent of pull-request issue comments. +func (c *Client) ListIssueComments(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.IssueComment, error) { + if err := c.validatePRRef(ref); err != nil { + return nil, err + } + endpoint := withQuery(restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "notes"), url.Values{"per_page": {"100"}}) + payloads, err := doRESTPages[noteResponse](ctx, c, gitprovider.OperationListIssueComments, endpoint) + if err != nil { + return nil, err + } + comments := make([]gitprovider.IssueComment, 0, len(payloads)) + for _, payload := range payloads { + if payload.System || payload.Position != nil { + continue + } + comments = append(comments, gitprovider.IssueComment{ + ID: gitprovider.CommentID(stringIDFromInt(payload.ID)), + Body: payload.Body, + Author: identityFromUser(payload.Author), + CreatedAt: payload.CreatedAt, + UpdatedAt: payload.UpdatedAt, + }) + } + return comments, nil +} + +func identityFromUser(user userResponse) gitprovider.Identity { + return gitprovider.Identity{ + Login: user.Username, + ID: stringIDFromInt(user.ID), + DisplayName: user.Name, + } +} + +func prState(state string) (gitprovider.PRState, error) { + switch strings.ToLower(strings.TrimSpace(state)) { + case "opened", "locked": + return gitprovider.PRStateOpen, nil + case "closed": + return gitprovider.PRStateClosed, nil + case "merged": + return gitprovider.PRStateMerged, nil + default: + return "", fmt.Errorf("%w: unknown merge request state %q", ErrValidation, state) + } +} + +func validateFilePath(filePath string) (string, error) { + if strings.TrimSpace(filePath) == "" { + return "", fmt.Errorf("%w: path is required", ErrValidation) + } + normalized := strings.Trim(filePath, "/") + if normalized == "" { + return "", fmt.Errorf("%w: path is required", ErrValidation) + } + for _, segment := range strings.Split(normalized, "/") { + if segment == "" { + return "", fmt.Errorf("%w: path must not contain empty segments", ErrValidation) + } + if segment == "." || segment == ".." { + return "", fmt.Errorf("%w: path must not contain dot segments", ErrValidation) + } + } + return normalized, nil +} diff --git a/internal/gitprovider/gitlab/rest_reads_test.go b/internal/gitprovider/gitlab/rest_reads_test.go new file mode 100644 index 00000000..044ffa5d --- /dev/null +++ b/internal/gitprovider/gitlab/rest_reads_test.go @@ -0,0 +1,314 @@ +package gitlab + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +func TestWhoAmIMapsUserAndUsesSuppliedToken(t *testing.T) { + var authorization string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/user" { + t.Fatalf("path = %q, want /user", r.URL.EscapedPath()) + } + authorization = r.Header.Get("Authorization") + writeJSON(t, w, userResponse{Username: "review-bot", ID: 7, Name: "Review Bot"}) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + identity, err := client.WhoAmI(context.Background(), gitprovider.Credential{Type: "pat", Token: "other-token"}) + if err != nil { + t.Fatalf("WhoAmI: %v", err) + } + if identity.Login != "review-bot" || identity.ID != "7" || identity.DisplayName != "Review Bot" { + t.Fatalf("identity = %#v, want mapped user", identity) + } + if authorization != "Bearer other-token" { + t.Fatalf("Authorization = %q, want supplied credential token", authorization) + } +} + +func TestGetPRMapsMergeRequestAndResolvesForkSource(t *testing.T) { + ref := testPRRef() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.EscapedPath() { + case "/projects/" + testProjectPath() + "/merge_requests/42": + writeJSON(t, w, mergeRequestResponse{ + IID: 42, + Title: "Add feature", + Description: "Body text", + State: "opened", + WebURL: "https://gitlab.example.com/open cli/sub group/repo+name/-/merge_requests/42", + Author: userResponse{Username: "author", ID: 3}, + SourceBranch: "feature", + TargetBranch: "main", + SHA: "headsha", + DiffRefs: diffRefsResponse{ + BaseSHA: "basesha", + StartSHA: "startsha", + HeadSHA: "headsha", + }, + SourceProjectID: 777, + TargetProjectID: 778, + }) + case "/projects/777": + writeJSON(t, w, projectResponse{PathWithNamespace: "fork-group/fork-repo"}) + default: + t.Fatalf("unexpected path %q", r.URL.EscapedPath()) + } + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + pr, err := client.GetPR(context.Background(), ref) + if err != nil { + t.Fatalf("GetPR: %v", err) + } + if pr.Title != "Add feature" || pr.Body != "Body text" || pr.State != gitprovider.PRStateOpen { + t.Fatalf("pr = %#v, want mapped merge request", pr) + } + if pr.Author.Login != "author" || pr.Author.ID != "3" { + t.Fatalf("author = %#v, want mapped author", pr.Author) + } + wantHead := gitprovider.PRBranchRef{Host: "gitlab.example.com", Owner: "fork-group", Repo: "fork-repo", Name: "feature", Ref: "refs/heads/feature", SHA: "headsha"} + if pr.Head != wantHead { + t.Fatalf("head = %#v, want %#v", pr.Head, wantHead) + } + wantBase := gitprovider.PRBranchRef{Host: "gitlab.example.com", Owner: ref.Owner, Repo: ref.Repo, Name: "main", Ref: "refs/heads/main", SHA: "basesha"} + if pr.Base != wantBase { + t.Fatalf("base = %#v, want %#v", pr.Base, wantBase) + } +} + +func TestGetPRMapsStates(t *testing.T) { + tests := []struct { + state string + want gitprovider.PRState + }{ + {state: "opened", want: gitprovider.PRStateOpen}, + {state: "locked", want: gitprovider.PRStateOpen}, + {state: "closed", want: gitprovider.PRStateClosed}, + {state: "merged", want: gitprovider.PRStateMerged}, + } + for _, tt := range tests { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + writeJSON(t, w, mergeRequestResponse{State: tt.state, SourceBranch: "feature", TargetBranch: "main"}) + })) + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + pr, err := client.GetPR(context.Background(), testPRRef()) + server.Close() + if err != nil { + t.Fatalf("GetPR(%s): %v", tt.state, err) + } + if pr.State != tt.want { + t.Errorf("state %q = %q, want %q", tt.state, pr.State, tt.want) + } + } +} + +func TestGetDiffReturnsRawDiff(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/merge_requests/42/raw_diffs" { + t.Fatalf("path = %q, want raw_diffs", r.URL.EscapedPath()) + } + _, _ = w.Write([]byte("diff --git a/main.go b/main.go\n")) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + diff, err := client.GetDiff(context.Background(), testPRRef()) + if err != nil { + t.Fatalf("GetDiff: %v", err) + } + if diff.Raw != "diff --git a/main.go b/main.go\n" { + t.Fatalf("diff = %q, want raw payload", diff.Raw) + } +} + +func TestGetDiffFallsBackToDiffsListingWhenRawDiffsMissing(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.EscapedPath() { + case "/projects/" + testProjectPath() + "/merge_requests/42/raw_diffs": + w.WriteHeader(http.StatusNotFound) + case "/projects/" + testProjectPath() + "/merge_requests/42/diffs": + writeJSON(t, w, []diffFileResponse{ + {OldPath: "main.go", NewPath: "main.go", Diff: "@@ -1,2 +1,2 @@\n-old\n+new\n context\n"}, + {OldPath: "gone.go", NewPath: "gone.go", DeletedFile: true, Diff: "@@ -1 +0,0 @@\n-bye\n"}, + {OldPath: "old-name.go", NewPath: "new-name.go", RenamedFile: true}, + {OldPath: "logo.png", NewPath: "logo.png"}, + }) + default: + t.Fatalf("unexpected path %q", r.URL.EscapedPath()) + } + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + diff, err := client.GetDiff(context.Background(), testPRRef()) + if err != nil { + t.Fatalf("GetDiff: %v", err) + } + want := strings.Join([]string{ + "diff --git a/main.go b/main.go", + "--- a/main.go", + "+++ b/main.go", + "@@ -1,2 +1,2 @@", + "-old", + "+new", + " context", + "diff --git a/gone.go b/gone.go", + "--- a/gone.go", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-bye", + "diff --git a/old-name.go b/new-name.go", + "rename from old-name.go", + "rename to new-name.go", + "diff --git a/logo.png b/logo.png", + "Binary files a/logo.png and b/logo.png differ", + "", + }, "\n") + if diff.Raw != want { + t.Fatalf("diff = %q, want %q", diff.Raw, want) + } +} + +func TestGetDiffBetweenRefsReconstructsCompare(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/repository/compare" { + t.Fatalf("path = %q, want compare", r.URL.EscapedPath()) + } + if r.URL.Query().Get("from") != "base" || r.URL.Query().Get("to") != "head" { + t.Fatalf("query = %q, want from=base to=head", r.URL.RawQuery) + } + writeJSON(t, w, map[string]any{ + "diffs": []diffFileResponse{{OldPath: "a.go", NewPath: "a.go", Diff: "@@ -1 +1 @@\n-x\n+y\n"}}, + }) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + diff, err := client.GetDiffBetweenRefs(context.Background(), testPRRef(), "base", "head") + if err != nil { + t.Fatalf("GetDiffBetweenRefs: %v", err) + } + if !strings.Contains(diff.Raw, "diff --git a/a.go b/a.go") || !strings.Contains(diff.Raw, "+y") { + t.Fatalf("diff = %q, want reconstructed compare diff", diff.Raw) + } +} + +func TestGetFileAtRefFetchesRawContents(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/repository/files/docs%2Fguide.md/raw" { + t.Fatalf("path = %q, want escaped file path", r.URL.EscapedPath()) + } + if r.URL.Query().Get("ref") != "headsha" { + t.Fatalf("ref = %q, want headsha", r.URL.Query().Get("ref")) + } + _, _ = w.Write([]byte("# Guide\n")) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + body, err := client.GetFileAtRef(context.Background(), testPRRef(), "headsha", "docs/guide.md") + if err != nil { + t.Fatalf("GetFileAtRef: %v", err) + } + if string(body) != "# Guide\n" { + t.Fatalf("body = %q, want raw file", body) + } + if _, err := client.GetFileAtRef(context.Background(), testPRRef(), "headsha", "../escape"); !errors.Is(err, ErrValidation) { + t.Fatalf("GetFileAtRef dot path error = %v, want ErrValidation", err) + } +} + +func TestListTreeAtRefPaginates(t *testing.T) { + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/repository/tree" { + t.Fatalf("path = %q, want repository tree", r.URL.EscapedPath()) + } + if r.URL.Query().Get("page") == "2" { + writeJSON(t, w, []treeEntryResponse{{ID: "sha-2", Type: "blob", Path: ".codereview/agents/security/prompt.md"}}) + return + } + if got := r.URL.Query().Get("path"); got != ".codereview/agents" { + t.Fatalf("path query = %q, want .codereview/agents", got) + } + w.Header().Set("Link", "<"+server.URL+r.URL.EscapedPath()+"?page=2&per_page=100>; rel=\"next\"") + writeJSON(t, w, []treeEntryResponse{{ID: "sha-1", Type: "tree", Path: ".codereview/agents/security"}}) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + entries, err := client.ListTreeAtRef(context.Background(), testPRRef(), "headsha", ".codereview/agents") + if err != nil { + t.Fatalf("ListTreeAtRef: %v", err) + } + want := []gitprovider.TreeEntry{ + {Path: ".codereview/agents/security", Type: "tree", SHA: "sha-1"}, + {Path: ".codereview/agents/security/prompt.md", Type: "blob", SHA: "sha-2"}, + } + if !reflect.DeepEqual(entries, want) { + t.Fatalf("entries = %#v, want %#v", entries, want) + } +} + +func TestListReviewsMapsApprovals(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/merge_requests/42/approvals" { + t.Fatalf("path = %q, want approvals", r.URL.EscapedPath()) + } + writeJSON(t, w, map[string]any{ + "approved_by": []map[string]any{ + {"user": userResponse{Username: "review-bot", ID: 7}}, + {"user": userResponse{Username: "human", ID: 8}}, + }, + }) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + reviews, err := client.ListReviews(context.Background(), testPRRef()) + if err != nil { + t.Fatalf("ListReviews: %v", err) + } + if len(reviews) != 2 { + t.Fatalf("reviews = %#v, want two approvals", reviews) + } + if reviews[0].State != gitprovider.ReviewStateApproved || reviews[0].Event != review.ReviewEventApprove { + t.Fatalf("review = %#v, want approved state", reviews[0]) + } + if reviews[0].Author.Login != "review-bot" || reviews[0].ID != gitprovider.ReviewID("approval-7") { + t.Fatalf("review = %#v, want mapped approver", reviews[0]) + } +} + +func TestListIssueCommentsFiltersSystemAndDiffNotes(t *testing.T) { + created := time.Date(2026, 7, 1, 12, 0, 0, 0, time.UTC) + line := 3 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/merge_requests/42/notes" { + t.Fatalf("path = %q, want notes", r.URL.EscapedPath()) + } + writeJSON(t, w, []noteResponse{ + {ID: 1, Body: "plain comment", Author: userResponse{Username: "human", ID: 8}, CreatedAt: created, UpdatedAt: created}, + {ID: 2, Body: "approved this merge request", System: true}, + {ID: 3, Body: "diff comment", Position: &positionResponse{NewPath: "main.go", NewLine: &line}}, + }) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + comments, err := client.ListIssueComments(context.Background(), testPRRef()) + if err != nil { + t.Fatalf("ListIssueComments: %v", err) + } + if len(comments) != 1 { + t.Fatalf("comments = %#v, want only the plain note", comments) + } + if comments[0].ID != gitprovider.CommentID("1") || comments[0].Body != "plain comment" || !comments[0].CreatedAt.Equal(created) { + t.Fatalf("comment = %#v, want mapped plain note", comments[0]) + } +} diff --git a/internal/gitprovider/gitlab/rest_writes.go b/internal/gitprovider/gitlab/rest_writes.go new file mode 100644 index 00000000..c43c00ad --- /dev/null +++ b/internal/gitprovider/gitlab/rest_writes.go @@ -0,0 +1,115 @@ +package gitlab + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +type approveRequest struct { + SHA string `json:"sha"` +} + +// PostIssueComment posts a merge-request note. +func (c *Client) PostIssueComment(ctx context.Context, ref gitprovider.PRRef, body string) (gitprovider.CommentID, error) { + if err := c.validatePRRef(ref); err != nil { + return "", err + } + if strings.TrimSpace(body) == "" { + return "", fmt.Errorf("%w: issue comment body is required", ErrValidation) + } + var response noteResponse + if err := c.doRESTJSON(ctx, gitprovider.OperationPostIssueComment, http.MethodPost, c.notesURL(ref), noteRequest{Body: body}, &response); err != nil { + return "", err + } + if response.ID <= 0 { + return "", fmt.Errorf("%w: note ID missing from successful GitLab response", ErrValidation) + } + return gitprovider.CommentID(stringIDFromInt(response.ID)), nil +} + +// SubmitReview posts the final review verdict. GitLab has no single review +// submission primitive, so the approval state changes first (approve for +// approve events, revoke for request-changes events) and the summary body is +// then posted as a merge-request note. Ordering matters for retries: the +// summary note carries the idempotency marker, so it must be the last write. +func (c *Client) SubmitReview(ctx context.Context, ref gitprovider.PRRef, request gitprovider.ReviewRequest) (gitprovider.ReviewID, error) { + op := gitprovider.OperationSubmitReview + if err := c.validatePRRef(ref); err != nil { + return "", err + } + if err := request.Validate(); err != nil { + return "", err + } + if len(request.Comments) > 0 { + return "", fmt.Errorf("%w: GitLab does not support bundling inline comments into a review submission", ErrValidation) + } + mr, err := c.getMergeRequest(ctx, op, ref) + if err != nil { + return "", err + } + if err := requireCurrentHead(op, mr, request.CommitSHA); err != nil { + return "", err + } + switch request.Event { + case review.ReviewEventApprove: + if err := c.approve(ctx, ref, request.CommitSHA); err != nil { + return "", err + } + case review.ReviewEventRequestChanges: + if err := c.unapprove(ctx, ref); err != nil { + return "", err + } + case review.ReviewEventComment: + default: + return "", fmt.Errorf("%w: invalid review event %q", ErrValidation, request.Event) + } + var response noteResponse + if err := c.doRESTJSON(ctx, op, http.MethodPost, c.notesURL(ref), noteRequest{Body: request.Body}, &response); err != nil { + return "", err + } + if response.ID <= 0 { + return "", fmt.Errorf("%w: note ID missing from successful GitLab response", ErrValidation) + } + return gitprovider.ReviewID(stringIDFromInt(response.ID)), nil +} + +func (c *Client) approve(ctx context.Context, ref gitprovider.PRRef, commitSHA string) error { + op := gitprovider.OperationSubmitReview + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "approve") + err := c.doRESTJSON(ctx, op, http.MethodPost, endpoint, approveRequest{SHA: commitSHA}, nil) + switch { + case err == nil: + return nil + case errors.Is(err, gitprovider.ErrConflict): + // GitLab responds 409 when the supplied SHA no longer matches the + // merge request head. + return gitprovider.WrapError(gitprovider.ErrStaleSHA, op, err) + case errors.Is(err, gitprovider.ErrAuth): + // The approve endpoint responds 401 when the authenticated user is + // not allowed to approve (for example self-approval restrictions), + // which is a permission problem rather than a credential one. + return gitprovider.WrapError(gitprovider.ErrPermission, op, err) + default: + return err + } +} + +func (c *Client) unapprove(ctx context.Context, ref gitprovider.PRRef) error { + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "unapprove") + err := c.doRESTJSON(ctx, gitprovider.OperationSubmitReview, http.MethodPost, endpoint, nil, nil) + // 404 means the posting identity had no active approval to revoke. + if err != nil && !errors.Is(err, gitprovider.ErrNotFound) { + return err + } + return nil +} + +func (c *Client) notesURL(ref gitprovider.PRRef) string { + return restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "notes") +} diff --git a/internal/gitprovider/gitlab/rest_writes_test.go b/internal/gitprovider/gitlab/rest_writes_test.go new file mode 100644 index 00000000..59d74953 --- /dev/null +++ b/internal/gitprovider/gitlab/rest_writes_test.go @@ -0,0 +1,238 @@ +package gitlab + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "reflect" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +func TestPostIssueCommentPostsNote(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/merge_requests/42/notes" { + t.Fatalf("unexpected request %s %q", r.Method, r.URL.EscapedPath()) + } + var request noteRequest + decodeJSON(t, r.Body, &request) + if request.Body != "rollup body" { + t.Fatalf("body = %q, want rollup body", request.Body) + } + writeJSON(t, w, noteResponse{ID: 61}) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + id, err := client.PostIssueComment(context.Background(), testPRRef(), "rollup body") + if err != nil { + t.Fatalf("PostIssueComment: %v", err) + } + if id != gitprovider.CommentID("61") { + t.Fatalf("id = %q, want 61", id) + } +} + +func submitReviewServer(t *testing.T, headSHA string, approveStatus, unapproveStatus int, calls *[]string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42": + *calls = append(*calls, "get") + writeJSON(t, w, mergeRequestResponse{ + State: "opened", SourceBranch: "feature", TargetBranch: "main", + DiffRefs: diffRefsResponse{BaseSHA: "basesha", StartSHA: "startsha", HeadSHA: headSHA}, + }) + case r.Method == http.MethodPost && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/approve": + var request approveRequest + decodeJSON(t, r.Body, &request) + *calls = append(*calls, "approve:"+request.SHA) + if approveStatus != http.StatusCreated { + w.WriteHeader(approveStatus) + return + } + writeJSON(t, w, map[string]any{"state": "approved"}) + case r.Method == http.MethodPost && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/unapprove": + *calls = append(*calls, "unapprove") + if unapproveStatus != http.StatusCreated { + w.WriteHeader(unapproveStatus) + return + } + writeJSON(t, w, map[string]any{"state": "unapproved"}) + case r.Method == http.MethodPost && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/notes": + var request noteRequest + decodeJSON(t, r.Body, &request) + *calls = append(*calls, "note:"+request.Body) + writeJSON(t, w, noteResponse{ID: 88}) + default: + t.Fatalf("unexpected request %s %q", r.Method, r.URL.EscapedPath()) + } + })) +} + +func TestSubmitReviewApproveAppliesApprovalThenPostsSummaryNote(t *testing.T) { + var calls []string + server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusCreated, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + id, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventApprove, + Body: "review summary", + }) + if err != nil { + t.Fatalf("SubmitReview: %v", err) + } + if id != gitprovider.ReviewID("88") { + t.Fatalf("id = %q, want 88", id) + } + if !reflect.DeepEqual(calls, []string{"get", "approve:headsha", "note:review summary"}) { + t.Fatalf("calls = %#v, want approval before summary note", calls) + } +} + +func TestSubmitReviewRequestChangesRevokesApprovalAndIgnoresMissingApproval(t *testing.T) { + var calls []string + server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusNotFound, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + if _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventRequestChanges, + Body: "needs work", + }); err != nil { + t.Fatalf("SubmitReview: %v", err) + } + if !reflect.DeepEqual(calls, []string{"get", "unapprove", "note:needs work"}) { + t.Fatalf("calls = %#v, want unapprove then note", calls) + } +} + +func TestSubmitReviewCommentOnlyPostsNote(t *testing.T) { + var calls []string + server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusCreated, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + if _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventComment, + Body: "just a comment", + }); err != nil { + t.Fatalf("SubmitReview: %v", err) + } + if !reflect.DeepEqual(calls, []string{"get", "note:just a comment"}) { + t.Fatalf("calls = %#v, want note only", calls) + } +} + +func TestSubmitReviewRejectsStaleCommitBeforeWriting(t *testing.T) { + var calls []string + server := submitReviewServer(t, "newer-head", http.StatusCreated, http.StatusCreated, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "stale-head", + Event: review.ReviewEventApprove, + Body: "review summary", + }) + if !errors.Is(err, gitprovider.ErrStaleSHA) { + t.Fatalf("SubmitReview error = %v, want ErrStaleSHA", err) + } + if !reflect.DeepEqual(calls, []string{"get"}) { + t.Fatalf("calls = %#v, want no writes after stale detection", calls) + } +} + +func TestSubmitReviewMapsApproveConflictToStaleSHA(t *testing.T) { + var calls []string + server := submitReviewServer(t, "headsha", http.StatusConflict, http.StatusCreated, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventApprove, + Body: "review summary", + }) + if !errors.Is(err, gitprovider.ErrStaleSHA) { + t.Fatalf("SubmitReview error = %v, want ErrStaleSHA", err) + } +} + +func TestSubmitReviewMapsApproveUnauthorizedToPermission(t *testing.T) { + var calls []string + server := submitReviewServer(t, "headsha", http.StatusUnauthorized, http.StatusCreated, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventApprove, + Body: "review summary", + }) + if !errors.Is(err, gitprovider.ErrPermission) { + t.Fatalf("SubmitReview error = %v, want ErrPermission", err) + } +} + +func TestSubmitReviewRejectsBundledComments(t *testing.T) { + client := mustClient(t, Options{Host: "gitlab.example.com"}) + _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventComment, + Body: "summary", + Comments: []gitprovider.InlineComment{{ + CommitSHA: "headsha", Body: "inline", Path: "main.go", + Side: review.DiffSideRight, Line: 2, SubjectType: review.AnchorKindLine, + }}, + }) + if !errors.Is(err, ErrValidation) { + t.Fatalf("SubmitReview error = %v, want ErrValidation for bundled comments", err) + } +} + +func TestReviewAuthorityMapsAccessLevels(t *testing.T) { + tests := []struct { + name string + status int + accessLevel int + wantEligible bool + wantName string + }{ + {name: "developer eligible", status: http.StatusOK, accessLevel: 30, wantEligible: true, wantName: "developer"}, + {name: "maintainer eligible", status: http.StatusOK, accessLevel: 40, wantEligible: true, wantName: "maintainer"}, + {name: "reporter ineligible", status: http.StatusOK, accessLevel: 20, wantEligible: false, wantName: "reporter"}, + {name: "non member", status: http.StatusNotFound, wantEligible: false, wantName: ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() != "/projects/"+testProjectPath()+"/members/all/7" { + t.Fatalf("path = %q, want members lookup", r.URL.EscapedPath()) + } + if tt.status != http.StatusOK { + w.WriteHeader(tt.status) + return + } + writeJSON(t, w, memberResponse{AccessLevel: tt.accessLevel}) + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + authority, err := client.ReviewAuthority(context.Background(), testPRRef(), gitprovider.Identity{Login: "review-bot", ID: "7"}) + if err != nil { + t.Fatalf("ReviewAuthority: %v", err) + } + if authority.Eligible != tt.wantEligible || authority.Permission != tt.wantName { + t.Fatalf("authority = %#v, want eligible=%v permission=%q", authority, tt.wantEligible, tt.wantName) + } + }) + } +} + +func TestReviewAuthorityRequiresIdentityID(t *testing.T) { + client := mustClient(t, Options{Host: "gitlab.example.com"}) + _, err := client.ReviewAuthority(context.Background(), testPRRef(), gitprovider.Identity{Login: "review-bot"}) + if !errors.Is(err, ErrValidation) { + t.Fatalf("ReviewAuthority error = %v, want ErrValidation", err) + } +} diff --git a/internal/gitprovider/gitlab/review_authority.go b/internal/gitprovider/gitlab/review_authority.go new file mode 100644 index 00000000..4d64ab6c --- /dev/null +++ b/internal/gitprovider/gitlab/review_authority.go @@ -0,0 +1,66 @@ +package gitlab + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +// GitLab access levels relevant to merge-request approval eligibility. +const accessLevelDeveloper = 30 + +type memberResponse struct { + AccessLevel int `json:"access_level"` +} + +// ReviewAuthority reports whether the resolved posting identity can approve +// merge requests on the target project. GitLab requires at least Developer +// access to approve. +func (c *Client) ReviewAuthority(ctx context.Context, ref gitprovider.PRRef, identity gitprovider.Identity) (gitprovider.ReviewAuthority, error) { + op := gitprovider.OperationReviewAuthority + if err := c.validatePRRef(ref); err != nil { + return gitprovider.ReviewAuthority{}, err + } + userID := strings.TrimSpace(identity.ID) + if userID == "" { + return gitprovider.ReviewAuthority{}, fmt.Errorf("%w: identity ID is required", ErrValidation) + } + var payload memberResponse + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "members", "all", url.PathEscape(userID)) + if _, _, err := c.doREST(ctx, op, http.MethodGet, endpoint, acceptJSON, &payload); err != nil { + if errors.Is(err, gitprovider.ErrNotFound) { + return gitprovider.ReviewAuthority{}, nil + } + return gitprovider.ReviewAuthority{}, err + } + return gitprovider.ReviewAuthority{ + Eligible: payload.AccessLevel >= accessLevelDeveloper, + Permission: accessLevelName(payload.AccessLevel), + }, nil +} + +func accessLevelName(level int) string { + switch level { + case 5: + return "minimal" + case 10: + return "guest" + case 15: + return "planner" + case 20: + return "reporter" + case 30: + return "developer" + case 40: + return "maintainer" + case 50: + return "owner" + default: + return fmt.Sprintf("access_level_%d", level) + } +} diff --git a/internal/gitprovider/types.go b/internal/gitprovider/types.go index b527dcc3..cd4fad6a 100644 --- a/internal/gitprovider/types.go +++ b/internal/gitprovider/types.go @@ -122,8 +122,20 @@ type ProviderCaps struct { NativeFileLevelComments bool ThreadResolution bool BundleInlineOnSubmit bool + // ReviewSummaryAsComment indicates SubmitReview posts its summary as a + // plain pull-request comment instead of a first-class review object, so + // posted-review reconciliation must scan issue comments. + ReviewSummaryAsComment bool + // HeadRefNamespace is the host's virtual ref namespace that serves + // pull-request head commits (refs///head). Empty + // means the GitHub "pull" namespace. + HeadRefNamespace string } +// PullHeadRefNamespace is the default virtual ref namespace for pull-request +// heads when a provider does not advertise one. +const PullHeadRefNamespace = "pull" + type ( // CommentID identifies a provider comment. CommentID string diff --git a/internal/gitproviders/gitproviders.go b/internal/gitproviders/gitproviders.go new file mode 100644 index 00000000..493949d6 --- /dev/null +++ b/internal/gitproviders/gitproviders.go @@ -0,0 +1,41 @@ +// Package gitproviders selects the concrete git-provider adapter for a git +// config. It is the single construction seam commands and the app runtime use +// so provider dispatch stays out of call sites. +package gitproviders + +import ( + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + gitlabprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/gitlab" +) + +// Options carries provider-specific construction options; only the block +// matching the config's provider kind is consulted. +type Options struct { + GitHub githubprovider.Options + GitLab gitlabprovider.Options +} + +// New builds the provider adapter selected by the git config along with the +// credential used to authenticate it. +func New(git config.GitConfig, store credentials.Reader, opts Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + switch git.ProviderKind() { + case config.GitProviderGitLab: + return gitlabprovider.NewFromGitConfig(git, store, opts.GitLab) + case config.GitProviderGitHub: + return githubprovider.NewFromGitConfig(git, store, opts.GitHub) + default: + return githubprovider.NewFromGitConfig(git, store, opts.GitHub) + } +} + +// GitBasicAuthUsername returns the username the git HTTP transport pairs with +// the provider token when fetching over HTTPS. +func GitBasicAuthUsername(kind config.GitProviderKind) string { + if kind == config.GitProviderGitLab { + return "oauth2" + } + return "x-access-token" +} diff --git a/internal/gitproviders/gitproviders_test.go b/internal/gitproviders/gitproviders_test.go new file mode 100644 index 00000000..5049452e --- /dev/null +++ b/internal/gitproviders/gitproviders_test.go @@ -0,0 +1,71 @@ +package gitproviders + +import ( + "errors" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + gitlabprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/gitlab" +) + +var errTokenNotFound = errors.New("token not found") + +type tokenStore map[string]map[string]string + +func (s tokenStore) Get(profile, key string) (string, error) { + keys, ok := s[profile] + if !ok { + return "", errTokenNotFound + } + value, ok := keys[key] + if !ok { + return "", errTokenNotFound + } + return value, nil +} + +func testGitConfig(provider config.GitProviderKind, host string) config.GitConfig { + return config.GitConfig{ + Provider: provider, + Host: host, + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work"}, + } +} + +func TestNewDispatchesOnProviderKind(t *testing.T) { + store := tokenStore{"work": {credentials.GitTokenKey: "token"}} + + provider, credential, err := New(testGitConfig("", "github.example.com"), store, Options{}) + if err != nil { + t.Fatalf("New github: %v", err) + } + if _, ok := provider.(*githubprovider.Client); !ok { + t.Fatalf("provider = %T, want github client for empty provider kind", provider) + } + if credential.Type != "pat" || credential.Token != "token" { + t.Fatalf("credential = %#v, want PAT", credential) + } + + provider, _, err = New(testGitConfig(config.GitProviderGitLab, "gitlab.example.com"), store, Options{}) + if err != nil { + t.Fatalf("New gitlab: %v", err) + } + if _, ok := provider.(*gitlabprovider.Client); !ok { + t.Fatalf("provider = %T, want gitlab client", provider) + } +} + +func TestGitBasicAuthUsername(t *testing.T) { + if got := GitBasicAuthUsername(config.GitProviderGitHub); got != "x-access-token" { + t.Fatalf("github username = %q, want x-access-token", got) + } + if got := GitBasicAuthUsername(""); got != "x-access-token" { + t.Fatalf("default username = %q, want x-access-token", got) + } + if got := GitBasicAuthUsername(config.GitProviderGitLab); got != "oauth2" { + t.Fatalf("gitlab username = %q, want oauth2", got) + } +} diff --git a/internal/outbox/outbox.go b/internal/outbox/outbox.go index 732ba740..d6fb5f29 100644 --- a/internal/outbox/outbox.go +++ b/internal/outbox/outbox.go @@ -148,7 +148,7 @@ func Post(ctx context.Context, opts Options, req Request) (Result, error) { if irrelevant(actions[i]) { continue } - match, err := reconcileAction(req, actions, actions[i], state) + match, err := reconcileAction(req, opts.Provider.Capabilities(), actions, actions[i], state) if err != nil { // Reconciliation is best-effort; dispatch planning below performs the // authoritative local validation and terminal state update. @@ -251,7 +251,7 @@ func Post(ctx context.Context, opts Options, req Request) (Result, error) { } return summarize(actions, ledger.OutcomeIncomplete, exitUpstream, false), readErr } - match, matchErr := reconcileAction(req, actions, actions[i], refetched) + match, matchErr := reconcileAction(req, opts.Provider.Capabilities(), actions, actions[i], refetched) if matchErr == nil && match.ok { now := opts.Now().UTC() actions[i].Status = ledger.PlannedActionPosted @@ -364,7 +364,7 @@ type reconcileMatch struct { upstreamID string } -func reconcileAction(req Request, actions []ledger.PlannedAction, action ledger.PlannedAction, state hostState) (reconcileMatch, error) { +func reconcileAction(req Request, caps gitprovider.ProviderCaps, actions []ledger.PlannedAction, action ledger.PlannedAction, state hostState) (reconcileMatch, error) { switch action.Kind { case ledger.PlannedActionInlineComment: return reconcileInline(req, action, state), nil @@ -377,7 +377,7 @@ func reconcileAction(req Request, actions []ledger.PlannedAction, action ledger. case ledger.PlannedActionRollupComment: return reconcileRollup(req, action, state), nil case ledger.PlannedActionSubmitReview: - return reconcileSubmitReview(req, action, state), nil + return reconcileSubmitReview(req, caps, action, state), nil case ledger.PlannedActionResolveThread: if _, err := typedPayload(action, action.ResolveThread); err != nil { return reconcileMatch{}, err @@ -442,7 +442,20 @@ func reconcileRollup(req Request, action ledger.PlannedAction, state hostState) return reconcileMatch{} } -func reconcileSubmitReview(req Request, action ledger.PlannedAction, state hostState) reconcileMatch { +func reconcileSubmitReview(req Request, caps gitprovider.ProviderCaps, action ledger.PlannedAction, state hostState) reconcileMatch { + if caps.ReviewSummaryAsComment { + // Hosts without first-class review objects post the review summary as + // a plain comment, so that is where an already-posted marker lives. + for _, comment := range state.issueComments { + if !comment.Author.Same(req.PostingIdentity) { + continue + } + if actionMarkerMatches(req, action, marker.ActionKindSubmitReview, "", comment.Body) { + return reconcileMatch{ok: true, upstreamID: string(comment.ID)} + } + } + return reconcileMatch{} + } for _, review := range state.reviews { if !review.Author.Same(req.PostingIdentity) { continue diff --git a/internal/outbox/outbox_review_summary_comment_test.go b/internal/outbox/outbox_review_summary_comment_test.go new file mode 100644 index 00000000..43eff92b --- /dev/null +++ b/internal/outbox/outbox_review_summary_comment_test.go @@ -0,0 +1,93 @@ +package outbox + +import ( + "context" + "reflect" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/ledger" + "github.com/open-cli-collective/codereview-cli/internal/marker" + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +func TestPostReconcilesSubmitReviewFromIssueCommentsWhenReviewSummaryAsComment(t *testing.T) { + store := openStore(t) + run := allocateRun(t, store, ledger.PostModeLive) + provider := newRecordingProvider() + provider.SetCapabilities(gitprovider.ProviderCaps{ReviewSummaryAsComment: true}) + ref := testPRRef() + limiter := &spyLimiter{} + + insertAction(t, store, plannedAction(run.RunID, "submit-1", ledger.PlannedActionSubmitReview, true, "", SubmitReviewPayload{ + Body: "review body", + Event: review.ReviewEventComment, + })) + + if err := provider.SetIssueComments(ref, []gitprovider.IssueComment{{ + ID: gitprovider.CommentID("note-77"), + Author: botIdentity(), + Body: mustRenderAction(t, marker.ActionMarker{ + RunID: run.RunID, + ActionID: "submit-1", + Kind: marker.ActionKindSubmitReview, + SHA: run.SHA, + BaseSHA: run.BaseSHA, + }), + }}); err != nil { + t.Fatalf("SetIssueComments: %v", err) + } + + result, err := Post(context.Background(), Options{Store: store, Provider: provider, Limiter: limiter, Now: fixedClock()}, testRequest(run)) + if err != nil { + t.Fatalf("Post: %v", err) + } + if result.Outcome != ledger.OutcomeComment || result.ExitCode != exitOK { + t.Fatalf("Post result = %#v, want comment exit 0", result) + } + if len(provider.writes) != 0 { + t.Fatalf("provider writes = %#v, want none after reconciliation", provider.writes) + } + action := actionByID(t, store, run.RunID, "submit-1") + if action.Status != ledger.PlannedActionPosted || action.UpstreamID == nil || *action.UpstreamID != "note-77" { + t.Fatalf("submit-1 = %#v, want posted upstream note-77", action) + } +} + +func TestPostDoesNotReconcileSubmitReviewFromIssueCommentsWithoutCap(t *testing.T) { + store := openStore(t) + run := allocateRun(t, store, ledger.PostModeLive) + provider := newRecordingProvider() + ref := testPRRef() + limiter := &spyLimiter{} + + insertAction(t, store, plannedAction(run.RunID, "submit-1", ledger.PlannedActionSubmitReview, true, "", SubmitReviewPayload{ + Body: "review body", + Event: review.ReviewEventComment, + })) + + if err := provider.SetIssueComments(ref, []gitprovider.IssueComment{{ + ID: gitprovider.CommentID("note-77"), + Author: botIdentity(), + Body: mustRenderAction(t, marker.ActionMarker{ + RunID: run.RunID, + ActionID: "submit-1", + Kind: marker.ActionKindSubmitReview, + SHA: run.SHA, + BaseSHA: run.BaseSHA, + }), + }}); err != nil { + t.Fatalf("SetIssueComments: %v", err) + } + + result, err := Post(context.Background(), Options{Store: store, Provider: provider, Limiter: limiter, Now: fixedClock()}, testRequest(run)) + if err != nil { + t.Fatalf("Post: %v", err) + } + if result.Outcome != ledger.OutcomeComment || result.ExitCode != exitOK { + t.Fatalf("Post result = %#v, want comment exit 0", result) + } + if !reflect.DeepEqual(provider.writes, []string{"SubmitReview"}) { + t.Fatalf("provider writes = %#v, want fresh SubmitReview without the capability", provider.writes) + } +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 5ce064d5..7d4c4a01 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -453,10 +453,11 @@ func SelectionOnly(ctx context.Context, opts Options, req SelectionRequest) (Sel } result := prepared.selectionResult() if err := workbench.Prepare(ctx, workbenchDeps(opts), workbench.Request{ - PRRef: req.PRRef, - ReviewPR: prepared.reviewPR, - ChangedFiles: prepared.changedFiles, - Artifacts: prepared.artifacts, + PRRef: req.PRRef, + ReviewPR: prepared.reviewPR, + ChangedFiles: prepared.changedFiles, + Artifacts: prepared.artifacts, + HeadRefNamespace: opts.Provider.Capabilities().HeadRefNamespace, }); err != nil { return SelectionResult{}, err } @@ -612,10 +613,11 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) result.Run = run if err := workbench.Prepare(ctx, workbenchDeps(opts), workbench.Request{ - PRRef: req.PRRef, - ReviewPR: prepared.reviewPR, - ChangedFiles: prepared.changedFiles, - Artifacts: prepared.artifacts, + PRRef: req.PRRef, + ReviewPR: prepared.reviewPR, + ChangedFiles: prepared.changedFiles, + Artifacts: prepared.artifacts, + HeadRefNamespace: opts.Provider.Capabilities().HeadRefNamespace, }); err != nil { if errors.Is(err, workbench.ErrUnsafeFetchRef) || errors.Is(err, workbench.ErrInvalidRepositoryIdentity) { return Result{}, Failure(FailureTerminal, err) diff --git a/internal/prref/prref.go b/internal/prref/prref.go index 13b68baf..d0a5b671 100644 --- a/internal/prref/prref.go +++ b/internal/prref/prref.go @@ -19,6 +19,28 @@ func ShortSHA(sha string) string { return sha } +// Provider identifies the git-host URL family a pull-request reference was +// parsed from. +type Provider string + +// Providers recognized by ParsePullURL. +const ( + ProviderGitHub Provider = "github" + ProviderGitLab Provider = "gitlab" +) + +// ParsePullURL parses a GitHub pull-request URL or a GitLab merge-request URL +// and reports which provider family the URL belongs to. +func ParsePullURL(raw string) (gitprovider.PRRef, Provider, error) { + if ref, err := ParseGitHubPullURL(raw); err == nil { + return ref, ProviderGitHub, nil + } + if ref, err := ParseGitLabMergeRequestURL(raw); err == nil { + return ref, ProviderGitLab, nil + } + return gitprovider.PRRef{}, "", fmt.Errorf("PR must be a GitHub pull request URL or GitLab merge request URL") +} + // ParseGitHubPullURL parses the v1 GitHub pull-request URL form. func ParseGitHubPullURL(raw string) (gitprovider.PRRef, error) { parsed, err := url.Parse(strings.TrimSpace(raw)) @@ -45,6 +67,57 @@ func ParseGitHubPullURL(raw string) (gitprovider.PRRef, error) { return ref, nil } +// ParseGitLabMergeRequestURL parses a GitLab merge-request URL. Both the +// canonical `/-/merge_requests/` form and the legacy form without the +// `/-/` separator are accepted, and namespaces may be nested, so the parsed +// owner can contain slashes (for example `group/subgroup`). +func ParseGitLabMergeRequestURL(raw string) (gitprovider.PRRef, error) { + parsed, err := url.Parse(strings.TrimSpace(raw)) + if err != nil || parsed.Scheme != "https" || parsed.Host == "" { + return gitprovider.PRRef{}, fmt.Errorf("PR must be a GitLab merge request URL") + } + parts := strings.Split(strings.Trim(parsed.Path, "/"), "/") + last := len(parts) - 1 + if last < 3 || parts[last-1] != "merge_requests" { + return gitprovider.PRRef{}, fmt.Errorf("PR must be a GitLab merge request URL") + } + projectParts := parts[:last-1] + if projectParts[len(projectParts)-1] == "-" { + projectParts = projectParts[:len(projectParts)-1] + } + if len(projectParts) < 2 { + return gitprovider.PRRef{}, fmt.Errorf("PR must be a GitLab merge request URL") + } + for _, segment := range projectParts { + if segment == "" || segment == "." || segment == ".." || segment == "-" { + return gitprovider.PRRef{}, fmt.Errorf("PR must be a GitLab merge request URL") + } + } + number, err := strconv.Atoi(parts[last]) + if err != nil || number <= 0 { + return gitprovider.PRRef{}, fmt.Errorf("PR number must be positive") + } + ref := gitprovider.PRRef{ + Host: parsed.Host, + Owner: strings.Join(projectParts[:len(projectParts)-1], "/"), + Repo: projectParts[len(projectParts)-1], + Number: number, + } + if err := ref.Validate(); err != nil { + return gitprovider.PRRef{}, err + } + return ref, nil +} + +// MatchProvider returns a usage-shaped error when a parsed PR URL's provider +// family does not match the configured git provider kind. +func MatchProvider(urlProvider Provider, configured string) error { + if string(urlProvider) == configured { + return nil + } + return fmt.Errorf("PR URL is a %s URL but the configured git provider is %q", urlProvider, configured) +} + // SameHost reports whether two host strings identify the same host. func SameHost(left, right string) bool { return normalizeHost(left) == normalizeHost(right) diff --git a/internal/prref/prref_test.go b/internal/prref/prref_test.go new file mode 100644 index 00000000..8f6a480e --- /dev/null +++ b/internal/prref/prref_test.go @@ -0,0 +1,102 @@ +package prref + +import ( + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +func TestParseGitHubPullURL(t *testing.T) { + ref, err := ParseGitHubPullURL("https://github.com/open-cli-collective/codereview-cli/pull/510") + if err != nil { + t.Fatalf("ParseGitHubPullURL error = %v", err) + } + want := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 510} + if ref != want { + t.Fatalf("ref = %+v, want %+v", ref, want) + } +} + +func TestParseGitLabMergeRequestURL(t *testing.T) { + tests := []struct { + name string + raw string + want gitprovider.PRRef + }{ + { + name: "canonical", + raw: "https://gitlab.com/group/project/-/merge_requests/42", + want: gitprovider.PRRef{Host: "gitlab.com", Owner: "group", Repo: "project", Number: 42}, + }, + { + name: "nested namespace", + raw: "https://gitlab.example.com/group/subgroup/project/-/merge_requests/7", + want: gitprovider.PRRef{Host: "gitlab.example.com", Owner: "group/subgroup", Repo: "project", Number: 7}, + }, + { + name: "legacy without dash separator", + raw: "https://gitlab.example.com/group/project/merge_requests/9", + want: gitprovider.PRRef{Host: "gitlab.example.com", Owner: "group", Repo: "project", Number: 9}, + }, + { + name: "query and fragment ignored", + raw: "https://gitlab.com/group/project/-/merge_requests/42?tab=diffs#note_1", + want: gitprovider.PRRef{Host: "gitlab.com", Owner: "group", Repo: "project", Number: 42}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ref, err := ParseGitLabMergeRequestURL(tt.raw) + if err != nil { + t.Fatalf("ParseGitLabMergeRequestURL(%q) error = %v", tt.raw, err) + } + if ref != tt.want { + t.Fatalf("ref = %+v, want %+v", ref, tt.want) + } + }) + } +} + +func TestParseGitLabMergeRequestURLRejectsInvalid(t *testing.T) { + tests := []string{ + "https://gitlab.com/project/-/merge_requests/42", + "https://gitlab.com/group/project/-/merge_requests/0", + "https://gitlab.com/group/project/-/merge_requests/abc", + "https://gitlab.com/group/project/-/merge_requests/42/diffs", + "https://gitlab.com/group/project/-/issues/42", + "http://gitlab.com/group/project/-/merge_requests/42", + "https://gitlab.com/group/../project/-/merge_requests/42", + "https://github.com/owner/repo/pull/12", + } + for _, raw := range tests { + if _, err := ParseGitLabMergeRequestURL(raw); err == nil { + t.Errorf("ParseGitLabMergeRequestURL(%q) = nil error, want error", raw) + } + } +} + +func TestParsePullURLDetectsProvider(t *testing.T) { + ref, provider, err := ParsePullURL("https://github.com/owner/repo/pull/12") + if err != nil || provider != ProviderGitHub { + t.Fatalf("ParsePullURL github = (%+v, %q, %v), want github", ref, provider, err) + } + ref, provider, err = ParsePullURL("https://gitlab.com/group/sub/repo/-/merge_requests/12") + if err != nil || provider != ProviderGitLab { + t.Fatalf("ParsePullURL gitlab = (%+v, %q, %v), want gitlab", ref, provider, err) + } + if ref.Owner != "group/sub" { + t.Fatalf("owner = %q, want group/sub", ref.Owner) + } + if _, _, err := ParsePullURL("https://example.com/not/a/pr"); err == nil { + t.Fatal("ParsePullURL invalid = nil error, want error") + } +} + +func TestSameHost(t *testing.T) { + if !SameHost("https://GitLab.example.com/", "gitlab.example.com") { + t.Fatal("SameHost normalized comparison failed") + } + if SameHost("gitlab.com", "github.com") { + t.Fatal("SameHost distinct hosts matched") + } +} diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 92b89942..53f8aacb 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -53,6 +53,10 @@ type Request struct { ReviewPR gitprovider.PR ChangedFiles []string Artifacts runartifact.Paths + // HeadRefNamespace is the host's virtual ref namespace serving + // pull-request heads (gitprovider.ProviderCaps.HeadRefNamespace). Empty + // means the GitHub "pull" namespace. + HeadRefNamespace string } type metadataArtifact struct { @@ -129,7 +133,7 @@ func (p *RunPreparer) Prepare(ctx context.Context, req Request) error { return err } if !sameBranchRepo(req.ReviewPR.Base, req.ReviewPR.Head) { - if err := ensurePullRequestHead(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.PRRef.Number, req.ReviewPR.Head, baseRemoteURL); err != nil { + if err := ensurePullRequestHead(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.PRRef.Number, req.HeadRefNamespace, req.ReviewPR.Head, baseRemoteURL); err != nil { return err } } else if err := ensureCommit(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head, baseRemoteURL); err != nil { @@ -213,12 +217,23 @@ func branchRemoteURL(branch gitprovider.PRBranchRef) (string, error) { host := strings.TrimSpace(branch.Host) owner := strings.Trim(strings.TrimSpace(branch.Owner), "/") repo := strings.TrimSuffix(strings.Trim(strings.TrimSpace(branch.Repo), "/"), ".git") - if host == "" || owner == "" || repo == "" || strings.Contains(host, "://") || strings.Contains(owner, "/") || strings.Contains(repo, "/") { + // Owners may span nested namespaces on hosts like GitLab, so slashes are + // allowed there but every segment must still be a plain path element. + if host == "" || owner == "" || repo == "" || strings.Contains(host, "://") || !validRepoPathSegments(owner) || strings.Contains(repo, "/") || !validRepoPathSegments(repo) { return "", fmt.Errorf("%w %s/%s on %s", ErrInvalidRepositoryIdentity, owner, repo, host) } return (&url.URL{Scheme: "https", Host: host, Path: "/" + owner + "/" + repo + ".git"}).String(), nil } +func validRepoPathSegments(value string) bool { + for _, segment := range strings.Split(value, "/") { + if segment == "" || segment == "." || segment == ".." || segment == "-" { + return false + } + } + return true +} + func ensureCommit(ctx context.Context, deps Deps, repoDir string, branch gitprovider.PRBranchRef, remoteURL string) error { ref := strings.TrimSpace(branch.Ref) if err := validateFetchRef(ref); err != nil { @@ -238,14 +253,30 @@ func ensureCommit(ctx context.Context, deps Deps, repoDir string, branch gitprov return fmt.Errorf("pipeline: fetch commit %s for %s/%s from %q", prref.ShortSHA(branch.SHA), branch.Owner, branch.Repo, remoteURL) } -func ensurePullRequestHead(ctx context.Context, deps Deps, repoDir string, number int, head gitprovider.PRBranchRef, remoteURL string) error { - ref := fmt.Sprintf("refs/pull/%d/head", number) +func ensurePullRequestHead(ctx context.Context, deps Deps, repoDir string, number int, headRefNamespace string, head gitprovider.PRBranchRef, remoteURL string) error { + ref, err := pullHeadRef(headRefNamespace, number) + if err != nil { + return err + } if _, err := deps.gitCommand(ctx, repoDir, "fetch", "--no-tags", remoteURL, ref); err == nil && commitPresent(ctx, deps, repoDir, head.SHA) { return nil } return fmt.Errorf("pipeline: fetch PR head commit %s from %q ref %q", prref.ShortSHA(head.SHA), remoteURL, ref) } +func pullHeadRef(namespace string, number int) (string, error) { + namespace = strings.TrimSpace(namespace) + if namespace == "" { + namespace = gitprovider.PullHeadRefNamespace + } + for _, r := range namespace { + if r != '-' && r != '_' && (r < 'a' || r > 'z') { + return "", fmt.Errorf("%w refs/%s/%d/head", ErrUnsafeFetchRef, namespace, number) + } + } + return fmt.Sprintf("refs/%s/%d/head", namespace, number), nil +} + func validateFetchRef(ref string) error { if ref == "" { return nil diff --git a/internal/workbench/workbench_gitlab_test.go b/internal/workbench/workbench_gitlab_test.go new file mode 100644 index 00000000..f719ee48 --- /dev/null +++ b/internal/workbench/workbench_gitlab_test.go @@ -0,0 +1,140 @@ +package workbench + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/runartifact" +) + +func TestPullHeadRef(t *testing.T) { + tests := []struct { + name string + namespace string + want string + wantErr bool + }{ + {name: "default pull namespace", namespace: "", want: "refs/pull/371/head"}, + {name: "merge requests namespace", namespace: "merge-requests", want: "refs/merge-requests/371/head"}, + {name: "unsafe namespace", namespace: "../evil", wantErr: true}, + {name: "namespace with space", namespace: "pull head", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := pullHeadRef(tt.namespace, 371) + if tt.wantErr { + if !errors.Is(err, ErrUnsafeFetchRef) { + t.Fatalf("pullHeadRef error = %v, want ErrUnsafeFetchRef", err) + } + return + } + if err != nil { + t.Fatalf("pullHeadRef error = %v", err) + } + if got != tt.want { + t.Fatalf("pullHeadRef = %q, want %q", got, tt.want) + } + }) + } +} + +func TestBranchRemoteURLAllowsNestedNamespaces(t *testing.T) { + got, err := branchRemoteURL(gitprovider.PRBranchRef{Host: "gitlab.example.com", Owner: "group/subgroup", Repo: "project"}) + if err != nil { + t.Fatalf("branchRemoteURL error = %v", err) + } + if got != "https://gitlab.example.com/group/subgroup/project.git" { + t.Fatalf("branchRemoteURL = %q, want nested namespace URL", got) + } + for _, owner := range []string{"group//sub", "group/../sub", "group/-", "."} { + if _, err := branchRemoteURL(gitprovider.PRBranchRef{Host: "gitlab.example.com", Owner: owner, Repo: "project"}); !errors.Is(err, ErrInvalidRepositoryIdentity) { + t.Fatalf("branchRemoteURL(%q) error = %v, want ErrInvalidRepositoryIdentity", owner, err) + } + } +} + +func TestPrepareFetchesForkHeadThroughMergeRequestRef(t *testing.T) { + ctx := context.Background() + fixture := newGitLabForkWorkbenchFixture(t) + artifacts := runartifact.FromDir(t.TempDir()) + var fetchedRefs []string + gitRunner := func(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmdArgs := append([]string(nil), args...) + if len(cmdArgs) >= 3 && cmdArgs[0] == "fetch" { + fetchedRefs = append(fetchedRefs, cmdArgs[len(cmdArgs)-1]) + if cmdArgs[2] == "https://gitlab.example.com/group/subgroup/project.git" { + cmdArgs[2] = fixture.baseRemotePath + } + } + cmd := exec.CommandContext(ctx, "git", cmdArgs...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + out, err := cmd.CombinedOutput() + if err != nil { + return nil, fmt.Errorf("git %s: %s", strings.Join(cmdArgs, " "), strings.TrimSpace(string(out))) + } + return out, nil + } + + err := Prepare(ctx, Deps{GitCommand: gitRunner}, Request{ + PRRef: fixture.pr.Ref, + ReviewPR: fixture.pr, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + HeadRefNamespace: "merge-requests", + }) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + if got := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "rev-parse", "HEAD")); got != fixture.pr.Head.SHA { + t.Fatalf("workbench HEAD = %q, want fork head %q", got, fixture.pr.Head.SHA) + } + if !slices.Contains(fetchedRefs, "refs/merge-requests/371/head") { + t.Fatalf("fetched refs = %#v, want refs/merge-requests/371/head", fetchedRefs) + } +} + +func newGitLabForkWorkbenchFixture(t *testing.T) forkWorkbenchFixture { + t.Helper() + baseSeedDir := t.TempDir() + gitCommandMustSucceed(t, baseSeedDir, "init", "-b", "main") + gitCommandMustSucceed(t, baseSeedDir, "config", "user.name", "Workbench Test") + gitCommandMustSucceed(t, baseSeedDir, "config", "user.email", "workbench@example.com") + if err := os.WriteFile(filepath.Join(baseSeedDir, "main.go"), []byte("package main\n\nvar changed = false\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + gitCommandMustSucceed(t, baseSeedDir, "add", "main.go") + gitCommandMustSucceed(t, baseSeedDir, "commit", "-m", "base") + baseSHA := strings.TrimSpace(gitCommandOutput(t, baseSeedDir, "rev-parse", "HEAD")) + baseRemotePath := filepath.Join(t.TempDir(), "base-remote.git") + gitCommandMustSucceed(t, "", "clone", "--bare", baseSeedDir, baseRemotePath) + forkRemotePath := filepath.Join(t.TempDir(), "fork-remote.git") + gitCommandMustSucceed(t, "", "clone", baseRemotePath, forkRemotePath) + gitCommandMustSucceed(t, forkRemotePath, "checkout", "-b", "feature") + gitCommandMustSucceed(t, forkRemotePath, "config", "user.name", "Fork Workbench Test") + gitCommandMustSucceed(t, forkRemotePath, "config", "user.email", "fork@example.com") + if err := os.WriteFile(filepath.Join(forkRemotePath, "main.go"), []byte("package main\n\nvar changed = true\n"), 0o600); err != nil { + t.Fatalf("update fork main.go: %v", err) + } + gitCommandMustSucceed(t, forkRemotePath, "commit", "-am", "fork head") + headSHA := strings.TrimSpace(gitCommandOutput(t, forkRemotePath, "rev-parse", "HEAD")) + gitCommandMustSucceed(t, forkRemotePath, "push", baseRemotePath, "HEAD:refs/merge-requests/371/head") + ref := gitprovider.PRRef{Host: "gitlab.example.com", Owner: "group/subgroup", Repo: "project", Number: 371} + return forkWorkbenchFixture{ + baseRemotePath: baseRemotePath, + pr: gitprovider.PR{ + Ref: ref, Title: "GitLab fork workbench fixture", URL: "https://gitlab.example.com/group/subgroup/project/-/merge_requests/371", State: gitprovider.PRStateOpen, + Base: gitprovider.PRBranchRef{Host: ref.Host, Owner: ref.Owner, Repo: ref.Repo, Name: "main", Ref: "refs/heads/main", SHA: baseSHA}, + Head: gitprovider.PRBranchRef{Host: ref.Host, Owner: "fork-owner", Repo: "project-fork", Name: "feature", Ref: "refs/heads/feature", SHA: headSHA}, + }, + } +}