feat(gitprovider): GitLab merge request review support#513
Conversation
Adds a gitlab provider adapter over the GitLab REST v4 API and wires it through config, URL parsing, provider dispatch, posting semantics, and the git transport so cr review/respond/agents/me/config work against gitlab.com and self-managed GitLab with PAT auth. - config: git.provider discriminator (github default, gitlab), gitlab restricted to pat auth - prref: ParsePullURL recognizes GitHub pull URLs and GitLab merge request URLs (nested namespaces, legacy form), commands cross-check the URL provider family against the profile - gitlab adapter: discussions as inline threads, position-based diff notes with old/new line mapping, approvals API for approve/revoke, discussion resolution, raw_diffs with per-file reconstruction fallback, members API for review authority - caps: ReviewSummaryAsComment reconciles submit-review markers against notes; HeadRefNamespace fetches refs/merge-requests/N/head for fork MRs; workbench accepts nested namespaces in remote URLs - gitexec: provider-specific HTTPS basic auth username (oauth2)
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 6cffdd9bf4f6
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 1 |
| structure:repo-health | 1 |
policies:conventions (1 finding)
Major - internal/config/config.go:232
The new GitLab-provider invariant is only enforced on
profile.Git.AuthMode(validateGitConfig, line ~1464: gitlab requires pat), not on the reviewer-entity path. Whenprofile.Reviewer.Kind == ProfileReviewerKindEntity, the matchedReviewerEntityis validated only for host match and distinct credential location — itsAuthModeis never checked againstprofile.Git.ProviderKind(). This lets a config declaregit.provider: gitlabwith areviewer.entitywhoseauth_modeisgithub_app, directly contradicting this PR's own documented invariant in README.md ("patis the only supportedauth_modefor GitLab; there is no GitHub App equivalent") and the config_git_provider_test.go coverage, which only exercises the profile-levelgit.auth_modecase (TestValidateRejectsGitLabProviderWithGitHubAppAuth). Such a config passesValidate/Loadand would only fail later inside the GitLab adapter/dispatch path instead of at config-validation time. Add the sameProviderKind() == GitProviderGitLab -> AuthMode must be patcheck for the resolvedreviewerEntityinvalidateProfile(mirroring the check already applied togit), and extendconfig_git_provider_test.gowith a case covering a GitLab profile whose reviewer entity usesgithub_app.
structure:repo-health (1 finding)
Minor - internal/app/runtime.go:21
writeReviewAuthorityWarning hardcodes 'may not create GitHub reviews' even though warnOpinionatedReviewAuthority now runs unconditionally for every provider (Open() calls it for both GitHub and GitLab profiles via the new gitproviders dispatch seam). A GitLab MR review with insufficient authority will print a message claiming a GitHub-specific problem, which is confusing for GitLab users and undercuts the provider-neutral ReviewAuthority abstraction this PR otherwise completes. The existing test (runtime_test.go:200) even locks in the GitHub-specific wording, so this was missed rather than intentional. Fix: make the message provider-neutral (e.g. reference the host/provider kind from postingIdentity or profile.Git instead of the literal string 'GitHub'), and update the test assertion accordingly.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_skipped | internal/app/runtime.go, internal/cmd/mecmd/mecmd.go, internal/config/config.go, internal/gitexec/client.go, internal/gitprovider/gitlab/client.go, internal/gitprovider/gitlab/discussions.go, internal/gitprovider/gitlab/http.go, internal/gitprovider/gitlab/linemap.go, internal/gitprovider/gitlab/rest_reads.go, internal/gitprovider/gitlab/rest_writes.go, internal/gitprovider/gitlab/review_authority.go, internal/gitproviders/gitproviders.go, internal/gitproviders/gitproviders_test.go, internal/outbox/outbox.go, internal/prref/prref.go, internal/workbench/workbench.go | internal/app/runtime_selection.go, internal/app/runtime_test.go, internal/benchmark/suite.go, internal/cmd/agentscmd/agentscmd.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/configcmd/configcmd.go, internal/cmd/initcmd/initcmd.go, internal/cmd/mecmd/mecmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_gitlab_test.go, internal/config/config_git_provider_test.go, internal/gitexec/client_username_test.go, internal/gitprovider/gitlab/client_test.go, internal/gitprovider/gitlab/discussions_test.go, internal/gitprovider/gitlab/imports_test.go, internal/gitprovider/gitlab/linemap_test.go, internal/gitprovider/gitlab/rest_reads_test.go, internal/gitprovider/gitlab/rest_writes_test.go, internal/gitprovider/types.go, internal/outbox/outbox_review_summary_comment_test.go, internal/pipeline/pipeline.go, internal/prref/prref_test.go, internal/workbench/workbench_gitlab_test.go | git was unavailable in this environment, so review relied on reading full file contents plus the provided change map rather than a line-scoped diff. |
| policies:conventions | complete_broad | README.md, internal/cmd/agentscmd/agentscmd.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/configcmd/configcmd.go, internal/cmd/initcmd/initcmd.go, internal/cmd/mecmd/mecmd.go, internal/cmd/mecmd/mecmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_gitlab_test.go, internal/config/config.go, internal/config/config_git_provider_test.go, internal/gitexec/client.go, internal/gitexec/client_username_test.go | unavailable | Sibling convention docs (../cli-common/docs, ../.github) were not present in the accessible workbench, so this review relies only on repo-local conventions (README.md, .codereview/agents/, and code) visible in this checkout. |
| structure:repo-health | complete_broad | README.md, internal/app/runtime.go, internal/app/runtime_selection.go, internal/app/runtime_test.go, internal/cmd/agentscmd/agentscmd.go, internal/cmd/mecmd/mecmd.go, internal/config/config.go, internal/gitprovider/types.go, internal/gitproviders/gitproviders.go, internal/gitproviders/gitproviders_test.go, internal/outbox/outbox.go, internal/outbox/outbox_review_summary_comment_test.go, internal/pipeline/pipeline.go, internal/workbench/workbench.go, internal/workbench/workbench_gitlab_test.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 8m 26s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 8m 26s wall · 12m 11s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 42s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 6m 57s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 50s |
| structure:repo-health | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 07s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 32s |
…h validation - The review-authority advisory no longer claims a GitHub-specific problem: it names the PR host instead, since the probe now runs for every provider through the gitproviders dispatch seam - Config validation now rejects a gitlab-provider profile whose reviewer entity uses a non-pat auth mode, mirroring the existing git.auth_mode check so the contradiction fails at Validate/Load instead of inside the adapter
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 374ab74e3f4e
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
go:implementation-tests (1 finding)
Minor - internal/config/config.go:232
validateEffectiveProfile enforces the gitlab-requires-pat restriction for profile.Git (line 1467-1468) and validateProfile enforces it for reviewer.entity (line 1247-1248), but the third auth surface that shares the same taxonomy — profile.ReviewerCredentials, used by gitConfigForReviewerAuth (internal/app/runtime.go:296-303) to build the posting-identity GitConfig — is only checked for AuthMode.Valid()/Supported() here, never against provider gitlab. A profile with git.provider: gitlab and reviewer_credentials.auth_mode: github_app (or oauth_device) passes
cr config validate/resolve-profiletoday and only fails later, deep in runtime.Open, when gitlab.NewFromGitConfig rejects the synthesized GitConfig. Add the samegit.ProviderKind() == GitProviderGitLab && profile.ReviewerCredentials.AuthMode != GitAuthModePATcheck here (mirroring line 1467) plus a covering case in config_git_provider_test.go, so misconfiguration is caught at config-validation time instead of at first live run.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_skipped | internal/app/runtime.go, internal/app/runtime_selection.go, internal/config/config.go, internal/config/config_git_provider_test.go, internal/gitexec/client.go, internal/gitprovider/gitlab/client.go, internal/gitprovider/gitlab/client_test.go, internal/gitprovider/gitlab/discussions.go, internal/gitprovider/gitlab/linemap.go, internal/gitprovider/gitlab/linemap_test.go, internal/gitprovider/gitlab/rest_reads.go, internal/gitprovider/gitlab/rest_writes.go, internal/gitprovider/gitlab/rest_writes_test.go, internal/gitprovider/gitlab/review_authority.go, internal/gitprovider/types.go, internal/gitproviders/gitproviders.go, internal/gitproviders/gitproviders_test.go, internal/outbox/outbox.go, internal/outbox/outbox_review_summary_comment_test.go, internal/prref/prref.go, internal/workbench/workbench.go | internal/app/runtime_test.go, internal/benchmark/suite.go, internal/cmd/agentscmd/agentscmd.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/configcmd/configcmd.go, internal/cmd/initcmd/initcmd.go, internal/cmd/mecmd/mecmd.go, internal/cmd/mecmd/mecmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_gitlab_test.go, internal/gitexec/client_username_test.go, internal/gitprovider/gitlab/discussions_test.go, internal/gitprovider/gitlab/http.go, internal/gitprovider/gitlab/imports_test.go, internal/gitprovider/gitlab/rest_reads_test.go, internal/pipeline/pipeline.go, internal/prref/prref_test.go, internal/workbench/workbench_gitlab_test.go | git history/diff tooling was unavailable in this sandbox, so files were reviewed against their current head-of-branch contents rather than a line-by-line diff against main. |
| policies:conventions | complete_broad | README.md, internal/cmd/agentscmd/agentscmd.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/configcmd/configcmd.go, internal/cmd/initcmd/initcmd.go, internal/cmd/mecmd/mecmd.go, internal/cmd/mecmd/mecmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_gitlab_test.go, internal/config/config.go, internal/config/config_git_provider_test.go, internal/gitexec/client.go, internal/gitexec/client_username_test.go | unavailable | Sibling convenience checkouts at ../cli-common/docs and ../.github were not present in the workbench, so this review relies on conventions visible in-repo (README.md, .codereview/agents) rather than the canonical cli-common docs.; The two convention-drift concerns raised in prior discussion (GitLab reviewer-entity auth_mode enforcement, provider-neutral review-authority warning) are marked Settled/Fixed in the discussion context and were spot-checked as present in internal/config/config.go; not re-reported. |
| structure:repo-health | complete_broad | README.md, internal/app/runtime.go, internal/app/runtime_selection.go, internal/app/runtime_test.go, internal/cmd/agentscmd/agentscmd.go, internal/cmd/mecmd/mecmd.go, internal/config/config.go, internal/gitprovider/types.go, internal/gitproviders/gitproviders.go, internal/gitproviders/gitproviders_test.go, internal/outbox/outbox.go, internal/outbox/outbox_review_summary_comment_test.go, internal/pipeline/pipeline.go, internal/workbench/workbench.go, internal/workbench/workbench_gitlab_test.go | unavailable | Reviewed the assigned provider-neutral seam and cross-package call sites only (internal/app, agentscmd, mecmd, config, gitprovider/gitproviders, outbox, pipeline, workbench); the GitLab adapter internals under internal/gitprovider/gitlab were not in scope for this reviewer.; The two issues previously raised in inline discussion (runtime.go GitHub-specific warning text, config.go missing GitLab PAT-only check on reviewer entities) are marked Settled and verified fixed in the current diff, so they are not repeated here. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 49s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 49s wall · 6m 36s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 25s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 45s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 27s |
| structure:repo-health | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 47s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 10s |
…dentials validateEffectiveProfile now applies the same gitlab-requires-pat check to profile.ReviewerCredentials that config-file validation applies to git.auth_mode and reviewer entities, so programmatically assembled runtime profiles fail at validation instead of inside the adapter.
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 324115c00364
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_skipped | internal/app/runtime.go, internal/config/config.go, internal/config/config_git_provider_test.go, internal/gitprovider/gitlab/linemap.go, internal/gitprovider/gitlab/linemap_test.go, internal/gitprovider/gitlab/rest_writes.go, internal/gitprovider/types.go, internal/gitproviders/gitproviders.go, internal/gitproviders/gitproviders_test.go | internal/app/runtime_selection.go, internal/app/runtime_test.go, internal/benchmark/suite.go, internal/cmd/agentscmd/agentscmd.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/configcmd/configcmd.go, internal/cmd/initcmd/initcmd.go, internal/cmd/mecmd/mecmd.go, internal/cmd/mecmd/mecmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_gitlab_test.go, internal/gitexec/client.go, internal/gitexec/client_username_test.go, internal/gitprovider/gitlab/client.go, internal/gitprovider/gitlab/client_test.go, internal/gitprovider/gitlab/discussions.go, internal/gitprovider/gitlab/discussions_test.go, internal/gitprovider/gitlab/http.go, internal/gitprovider/gitlab/imports_test.go, internal/gitprovider/gitlab/rest_reads.go, internal/gitprovider/gitlab/rest_reads_test.go, internal/gitprovider/gitlab/rest_writes_test.go, internal/gitprovider/gitlab/review_authority.go, internal/outbox/outbox.go, internal/outbox/outbox_review_summary_comment_test.go, internal/pipeline/pipeline.go, internal/prref/prref.go, internal/prref/prref_test.go, internal/workbench/workbench.go, internal/workbench/workbench_gitlab_test.go | Spot-checked core seams (runtime, config, gitproviders, gitlab linemap/writes) rather than every line of all 40 assigned files. |
| policies:conventions | complete_broad | README.md, internal/cmd/agentscmd/agentscmd.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/configcmd/configcmd.go, internal/cmd/initcmd/initcmd.go, internal/cmd/mecmd/mecmd.go, internal/cmd/mecmd/mecmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_gitlab_test.go, internal/config/config.go, internal/config/config_git_provider_test.go, internal/gitexec/client.go, internal/gitexec/client_username_test.go | unavailable | No sibling ../cli-common/docs or ../.github convenience checkouts were present in the workbench, so cross-repo shared CLI standards were not directly diffed; review relied on in-repo conventions and the settled discussion threads only.; git diff/log tooling was unavailable in this session, so review was based on reading full current file contents at head rather than incremental hunks; change_map/discussion context was used to focus attention on the relevant regions. |
| structure:repo-health | complete_broad | README.md, internal/app/runtime.go, internal/app/runtime_selection.go, internal/app/runtime_test.go, internal/cmd/agentscmd/agentscmd.go, internal/cmd/mecmd/mecmd.go, internal/config/config.go, internal/gitprovider/types.go, internal/gitproviders/gitproviders.go, internal/gitproviders/gitproviders_test.go, internal/outbox/outbox.go, internal/outbox/outbox_review_summary_comment_test.go, internal/pipeline/pipeline.go, internal/workbench/workbench.go, internal/workbench/workbench_gitlab_test.go | unavailable | Prior discussion threads flagged provider-neutral gaps (GitHub-worded warning text, missing gitlab-requires-pat checks on reviewer entity and reviewer_credentials); all three are already fixed in the inspected diff, so they are not re-raised here. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 15s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 5m 15s wall · 9m 13s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 25s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 4m 06s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 41s |
| structure:repo-health | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 49s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 11s |
Summary
Adds GitLab merge request review support (#510):
cr review,cr respond,cr agents,cr me, andcr config resolve-profilenow work against gitlab.com and self-managed GitLab instances with PAT auth, alongside the existing GitHub support.Config.
git.provideris a new optional discriminator (github|gitlab). Empty means GitHub, so existing configs are untouched and never rewritten. GitLab profiles are restricted toauth_mode: patand reuse the existinggit_tokencredstore key —cr set-credentialworks unchanged.URL parsing.
prref.ParsePullURLrecognizes both GitHub/pull/NURLs and GitLab/-/merge_requests/NURLs (nested namespaces and the legacy form without/-/included), and every command cross-checks the URL's provider family against the resolved profile with a usage error on mismatch. Nested namespaces flow through as slashedPRRef.Owner; ledger keys already percent-encode segments so no key-format change was needed.GitLab adapter (
internal/gitprovider/gitlab, hand-rollednet/httpover REST v4, mirroring the github package's structure, error taxonomy, secret redaction, and pagination host-pinning):diff_refs, with old/new line counterparts computed from the per-file diff so comments on unchanged context lines post correctly, and rename-awareold_path/new_path. File-level positions are native (position_type: file).SubmitReviewmapsapproveto the approvals API (SHA-guarded; 409 → stale-SHA),request_changesrevokes any active self-approval, and posts the review summary as an MR note. The approval state change happens before the summary note so interrupted posts retry safely (the note carries the idempotency marker).ListReviewsmaps current approvals;ResolveThreadusesPUT .../discussions/:id?resolved=true;ReviewAuthorityuses the members API (Developer+ = eligible).GetDiffusesraw_diffs(GitLab ≥ 15.7) with a per-file reconstruction fallback for older instances; stale head SHAs are rejected before any write.Provider-neutral seams that had GitHub baked in:
ProviderCaps.ReviewSummaryAsComment— outbox reconciles submit-review markers against issue comments for hosts without first-class review objects.ProviderCaps.HeadRefNamespace— workbench fetches fork MR heads fromrefs/merge-requests/N/headinstead ofrefs/pull/N/head; workbench remote URLs accept nested namespaces.gitexecpairs the token with a provider-specific HTTPS basic-auth username (oauth2for GitLab,x-access-tokenunchanged for GitHub).internal/gitprovidersdispatch package is the single construction seam used byinternal/app,agentscmd, andmecmd.Deliberately out of scope (follow-ups):
cr initwizard support for GitLab (profiles are added viaconfig.yml, documented in the README), GitLab CI job-token / group-token auth modes, and benchmark suites against GitLab MRs.Note:
TestDryRunResumesPinnedReviewRunByPinnedSHAsandTestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefsfail locally on this machine onmaintoo —trustCurrentTempFixturespointsTMPDIRat a directory that is never created and this git build won't write temp files there. Unrelated to this change; happy to file separately.Type of Change
Checklist
make tidypassesmake lintpassesmake testpasses (two pre-existing environment-dependent pipeline test failures reproduce onmain, see note above)make buildpassesRelated Issues
Closes #510