feat: add GitLab SCM integration with multi-provider dispatcher - #2773
feat: add GitLab SCM integration with multi-provider dispatcher#2773vytautas-petrikas wants to merge 32 commits into
Conversation
6a33f77 to
98d6072
Compare
whoisasx
left a comment
There was a problem hiding this comment.
Review Summary
I reviewed this implementation against AO's existing GitHub observer flow and GitLab's REST API behavior. Using REST exclusively for GitLab is acceptable and is not a blocker by itself. The provider abstraction, GitLab authentication support, and reuse of AO's existing SCM domain are directionally correct.
However, the implementation is not ready to merge because transient API failures can corrupt durable state, project changes trigger excessive MR refreshes, current GitLab mergeability values are mapped incorrectly, and rate-limit instructions are ignored. There are also correctness problems around provider isolation, repository identity, approvals, and pagination.
1. Transient failures can overwrite durable SCM state
FetchPullRequests records an error but returns a keyed Fetched=false observation with a nil batch error. The observer does not reject Fetched=false, so that placeholder can proceed to persistence. Pipeline, job, and approval failures are also converted to unknown, empty checks, or none, while the final observation remains Fetched=true.
A timeout, 429, 5xx response, or malformed payload can therefore replace previously valid MR metadata, CI, review, and mergeability facts. Failed retrieval must preserve the last durable state and must not advance ETags or refresh cursors.
2. A project ETag change refreshes every MR
The project guard requests only the latest open MR with per_page=1. When its ETag changes, AO paginates through every open MR, marks every tracked MR in that project as a refresh candidate, and executes approximately four additional REST requests for each tracked MR.
A change to one MR in a project containing 50 tracked MRs can produce approximately 252 requests. GitLab supports updated_after on project MR listings, so ordinary synchronization should request only MRs updated since the last successful cursor. Use state=all to include opened, closed, and merged MRs, retain an overlap window to avoid timestamp races, advance the cursor only after successful processing, and reserve complete listings for startup and periodic reconciliation. This would reduce the same one-MR update to approximately 56 requests under the current pipeline-guard design.
Reference: GitLab project merge-request listing.
3. Current mergeability values are mapped incorrectly
effectiveMergeStatus() prefers detailed_merge_status, but the mapper expects legacy values such as can_be_merged and cannot_be_merged. Current GitLab values include mergeable, conflict, checking, preparing, need_rebase, and requested_changes. Consequently, a mergeable MR can be reported as blocked, and a conflicting MR may not be reported as conflicting.
Tests must populate DetailedMergeStatus and cover current values while retaining legacy aliases for older GitLab installations.
4. GitLab rate-limit instructions are discarded
A 429 is reduced to a generic ErrRateLimited. AO ignores Retry-After, RateLimit-Reset, and remaining-quota headers and continues polling every 30 seconds. The adapter should expose structured retry information, apply a provider-level cooldown with bounded backoff and jitter, and use a finite HTTP timeout. Conditional 304 responses still consume request capacity.
References: GitLab.com limits and rate-limit headers.
5. One provider failure suppresses the other provider
The multi-provider dispatcher processes provider groups sequentially and returns immediately on the first error. A GitLab timeout can discard successful GitHub observations or prevent GitHub from being called, depending on map iteration order. Provider failures must remain scoped to their own references, and healthy-provider results should continue through persistence.
6. Repository identity is not preserved end to end
Claim validation compares owner and repository but ignores provider and host. Repositories with identical names on GitHub and GitLab can therefore pass validation incorrectly. Nested GitLab namespaces are parsed inconsistently, and self-managed hosts can be recognized while API traffic still uses https://gitlab.com/api/v4.
The provider, host, complete namespace, REST base, and credential host must be derived from one canonical repository identity. Either support self-managed GitLab end to end or explicitly scope this change to GitLab.com.
7. Approval state and persistence are unreliable
The approvals endpoint is requested twice during review refresh. The implementation compares len(approved_by) with approvals_required, but approved_by can contain approvals that do not satisfy the applicable rules. Use approved, approvals_left, or the REST approval-state endpoint.
Generated review summaries also need stable IDs; otherwise multiple approvers use the same empty persistence key and overwrite one another.
8. Jobs and discussions are truncated
Only the first 100 pipeline jobs and first 100 discussions are fetched. Later failed jobs and unresolved review threads can remain invisible indefinitely. Follow GitLab pagination links or explicitly represent bounded responses as partial without deleting previously persisted data.
API Consumption
For 50 active sessions, stable polling is approximately 102-200 requests per minute, moderate activity is approximately 150-300 requests per minute, and cold synchronization is approximately 400-500 requests. Large failed-pipeline bursts can add hundreds of job-trace requests. This makes incremental discovery and rate-limit recovery important for the REST-only design.
Conclusion
The REST-only direction is acceptable, but the blocking areas above need to be addressed before merging: durable-state preservation, incremental MR discovery, current mergeability mapping, rate-limit recovery, provider isolation, repository identity, and approval correctness.
The package documentation should also describe REST as a deliberate compatibility and raw-trace decision rather than claiming that GitLab GraphQL does not support pipelines or approvals. Once these issues are resolved with focused tests, the implementation can be reassessed for merge readiness.
Thank you for the thorough review. All eight blocking findings plus the documentation concern have been addressed. Build and vet are clean, 333 tests pass with 1. Transient failures preserve durable state
Tests: 2. Incremental MR discovery
Tests: 3. Current mergeability values mapped correctly
Tests: 4. Rate-limit recovery
Tests: 5. Provider isolation
Tests: 6. Repository identity end-to-end
Tests: 7. Approval state and persistence
Tests: 8. Full pagination for jobs and discussions
Tests: 9. Package documentation
Known non-blocking items
|
430d4c6 to
060e957
Compare
|
Thanks for contributing to Agent Orchestrator. This PR is being picked up by the current external contributor on-call pair: If someone is already working on this, please continue as usual. For faster context or live questions, you can also join the AO Discord. Join the session here: Come by if you want to see what is being built, ask questions, or just hang around with the community. |
whoisasx
left a comment
There was a problem hiding this comment.
Thank you for continuing to push revisions and for working through the earlier feedback. The latest changes make meaningful progress: the direct transient-failure persistence path is safer, the current GitLab mergeability mapping is corrected, approval records now use stable identities, discussion pagination is represented as partial, and the REST design is documented more clearly.
The package structure is broadly aligned with the existing GitHub implementation: GitLab stays behind the shared SCM provider boundary, the CLI remains a daemon client, and persistence/lifecycle continue through the common observer. However, the implementation does not yet preserve the same behavioral integrity. The remaining blockers are around incremental polling, terminal-state observation, cooldown acknowledgement, fork identity, credential isolation, and numeric issue handling.
I have also called out the additional correctness and reliability findings inline: nested namespaces, cross-provider display identity, pagination trust, log memory bounds, job completeness, and mixed-provider error propagation. Please add second-poll/cursor regression tests for the observer cases and multi-host/provider tests for the identity and credential cases.
Additional items that should be addressed with this pass:
- Preserve the scheme and port for self-managed instances, and support
ssh://remotes consistently. - Do not parse the undocumented
diff_statsobject from MR detail; use fields/endpoints that GitLab actually returns. - Three advertised repository-identity test cases in
service_test.goare currently part of comments and never execute. doc.goclaims periodic full reconciliation, but no reconciliation path currently resetsupdatedAfterto zero.
The architecture is moving in the right direction, but these issues can cause AO to permanently miss state changes, associate the wrong MR/issue with a session, or send credentials outside the intended GitLab host. I am keeping this as Changes Requested until those integrity gaps are closed.
| // Incremental discovery: on polls after the first (hasCursor), only | ||
| // refresh MRs that appeared in the updated set. On the first poll | ||
| // (no cursor), refresh all tracked MRs | ||
| if hasCursor && listedPRs != nil { |
There was a problem hiding this comment.
[P1] This post-cursor selection rule permanently misses GitHub terminal transitions. GitHub's repository listing still requests only state=open; when a tracked PR is merged or closed it disappears from listedPRs before its detail can be refreshed, while the repository ETag/cursor can still advance. Please keep tracked PRs eligible for terminal-state reconciliation, with a second-poll test that transitions an open PR to merged/closed.
There was a problem hiding this comment.
Fixed in bf81060. New reconcileTerminalGitHubPRs pass issues a detail fetch for tracked-open GitHub PRs absent from the state=open listing. A terminal (merged/closed) result is persisted normally; a "still open" result marks the repo refresh-incomplete so the ETag/cursor stay pinned. GitLab is excluded — it uses state=all, so terminal MRs never leave the listing.
| // same chunk | ||
| active := chunk[:0] | ||
| for _, ref := range chunk { | ||
| if o.inRateLimitCooldown(now, ref.Repo.Provider) { |
There was a problem hiding this comment.
[P1] Skipping this ref under cooldown must mark its repository refresh incomplete. Today it is removed from active without calling markRepoRefreshFailed, so the repository ETag and LastSyncCursor can be advanced even though no observation was fetched. After cooldown, a 304 can make the skipped update unrecoverable.
There was a problem hiding this comment.
Fixed in bf81060. The cooldown-skip path now calls markRepoRefreshFailed(ref.Repo) before continue, so the repo ETag and LastSyncCursor do not advance without an observation. A subsequent 304 can no longer make the skipped update unrecoverable.
| Merged: merged, | ||
| Closed: closed, | ||
| SourceBranch: mr.SourceBranch, | ||
| HeadRepo: repo.Repo, |
There was a problem hiding this comment.
[P1] This falsifies the head repository for fork MRs. source_project_id and target_project_id are already parsed, but HeadRepo is always set to the target project, allowing a fork MR with a matching branch name to pass AO's head-repository ownership guard. Resolve the source project path, or at minimum fail closed when the project IDs differ, and add a fork-MR regression test.
There was a problem hiding this comment.
Fixed in 7763adf. When source_project_id != target_project_id, the provider fetches /projects/:source_project_id and sets HeadRepo to the source project's path_with_namespace. On fetch failure the error propagates (fail closed) so HeadRepo is never falsified.
| strings.HasSuffix(host, ".github.com") || strings.HasSuffix(host, ".ghe.io") { | ||
| return false | ||
| } | ||
| return strings.Contains(host, "gitlab") |
There was a problem hiding this comment.
[P1] Treating every hostname containing gitlab as trusted is unsafe because clientForHost will attach the configured bearer token to that host. A remote such as gitlab.attacker.example is accepted here. Please restrict hosts to gitlab.com plus explicitly configured instances, and select/cache credentials per exact host rather than sharing one token source across all instances.
There was a problem hiding this comment.
Fixed in 7d934a7. isGitLabHost replaced with isHostAllowed: gitlab.com always allowed; self-managed hosts must appear in ProviderOptions.AllowedHosts. clientForHost returns nil for non-allowlisted hosts, and clientForRepoErr returns ErrHostNotAllowed before any credential is attached. The strings.Contains(host, "gitlab") heuristic is gone.
| } | ||
| } | ||
| owner, repo, err := githubRepoFromURL(project.RepoOriginURL) | ||
| _, owner, repo, err := repoFromURL(project.RepoOriginURL) |
There was a problem hiding this comment.
[P1] This fallback now accepts a GitLab origin and turns its numeric issue into a GitHub tracker ID. On a GitLab project, ao spawn --issue 42 can fetch owner/repo#42 from GitHub and inject unrelated issue content into the prompt. If SCM parsing identifies a non-GitHub provider, return false instead of applying the GitHub fallback.
There was a problem hiding this comment.
Fixed in d6d3bb2. githubRepoForTracker now checks repo.Provider != "github" and returns ("", false) when the origin is a non-GitHub provider, so ao spawn --issue 42 on a GitLab project no longer falls through to the GitHub issue-tracker fallback.
| return "", "", "", errors.New("bad repo") | ||
| } | ||
| return parts[0], strings.TrimSuffix(parts[1], ".git"), nil | ||
| return host, parts[0], strings.TrimSuffix(parts[1], ".git"), nil |
There was a problem hiding this comment.
[P2] This truncates nested GitLab namespaces for HTTPS remotes. https://gitlab.com/group/subgroup/repo.git becomes owner=group, repo=subgroup, so a numeric claim constructs /group/subgroup/-/merge_requests/N and fails same-repository validation. Split the final path component as the repository and preserve every preceding component as the namespace, matching cliParsePRURL.
There was a problem hiding this comment.
Fixed in 593692e. cliRepoFromURL now splits the final path component as the repo name and joins all preceding components as the owner/namespace, matching cliParsePRURL. https://gitlab.com/group/subgroup/repo.git now yields owner=group/subgroup, name=repo. Both SSH and HTTPS paths use the same split logic.
| title: session.title, | ||
| state: pr.state, | ||
| provider: "github", | ||
| provider: detectProviderFromUrl(pr.url), |
There was a problem hiding this comment.
[P2] Now that summaries can come from multiple providers/repositories, PR number is no longer a unique identity. sessionPRDisplaySummaries still indexes and deduplicates by number alone, so GitHub PR #7 and GitLab MR #7 (or two child repos with MR #7) reuse one summary and hide the other. Key by canonical URL or provider+host+repo+number and add a collision test.
There was a problem hiding this comment.
| return nil, err | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
| body, _ := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
[P2] Job traces can be very large, but this reads the entire response into daemon memory even though callers retain only the last 20 lines. Please stream into a bounded rolling tail (and bound error bodies separately) so one large trace cannot create unbounded memory pressure.
There was a problem hiding this comment.
Fixed in a1ff90b. doGETRaw now streams the response through readTail, a 64 KB rolling tail, so one pathological trace cannot create unbounded memory pressure. Error bodies (status ≥ 400) are capped at 4 KB before classification. Callers retain only the last 20 lines, which operate on the bounded buffer.
| path := fmt.Sprintf("/projects/%s/pipelines/%d/jobs", projectPath(repo.Owner, repo.Name), pipelineID) | ||
| q := url.Values{"per_page": {strconv.Itoa(pipelineJobsPageSize)}} | ||
| var allJobs []restJob | ||
| _, err := p.clientForHost(repo.Host).doGETPaginated(ctx, path, q, func(body []byte) error { |
There was a problem hiding this comment.
[P2] The truncation result is discarded, so more than 1,000 jobs are treated as a complete CI snapshot and later failures can be omitted while Fetched=true replaces durable checks. Please propagate partial/incomplete state instead of authoritatively persisting a capped result. Also parse allow_failure; optional failed jobs currently become actionable failed checks even when the pipeline succeeds.
There was a problem hiding this comment.
Fixed in ebe4c97 + 16ce0cb. fetchPipelineJobs returns the truncated flag from doGETPaginated and propagates it as CI.Partial. The observer gates persistence on CI.Partial (preserveLocalCIHash) so a capped result does not overwrite durable checks. allow_failure: true jobs appear in Checks but are excluded from FailedChecks, so optional failures don't become actionable when the pipeline succeeds.
060e957 to
f465001
Compare
Restrict GitLab hosts to gitlab.com plus an explicitly configured allowlist so gitlab.attacker.example is rejected before any credential is attached. Adds a GitLab sub-struct on config.Config (matching TelemetryConfig pattern) loaded once at boot from env vars (no hot-reload): AO_GITLAB_ALLOWED_HOSTS comma-separated self-managed hosts (may include :port) AO_GITLAB_HOST_TOKENS host=token,host=token per-host credential overrides The existing AO_GITLAB_TOKEN/GITLAB_TOKEN continues to apply to gitlab.com; allowlisted hosts without an explicit per-host token fall back to the default. The provider builds an internal host→TokenSource map at NewProvider time; clientForHost selects the right source per host. The TokenSource interface is unchanged. The per-host client map preserves scheme (https) and port (e.g. https://gitlab.internal:8443/api/v4). ssh:// remotes parse to the same host:port as their https:// equivalents. Integration tests (httptest.NewServer end-to-end): - gitlab.attacker.example rejected before any HTTP request (0 hits) - per-host credential isolation (gitlab.com token not leaked to self-managed) - custom-port self-managed instance (REST base includes :8443) - ssh:// and https:// remotes parse to identical host:port Addresses reviewer Items 5 and S1 (PR AgentWrapper#2773 review by @whoisasx).
ed5c374 to
689f9c9
Compare
Restrict GitLab hosts to gitlab.com plus an explicitly configured allowlist so gitlab.attacker.example is rejected before any credential is attached. Adds a GitLab sub-struct on config.Config (matching TelemetryConfig pattern) loaded once at boot from env vars (no hot-reload): AO_GITLAB_ALLOWED_HOSTS comma-separated self-managed hosts (may include :port) AO_GITLAB_HOST_TOKENS host=token,host=token per-host credential overrides The existing AO_GITLAB_TOKEN/GITLAB_TOKEN continues to apply to gitlab.com; allowlisted hosts without an explicit per-host token fall back to the default. The provider builds an internal host→TokenSource map at NewProvider time; clientForHost selects the right source per host. The TokenSource interface is unchanged. The per-host client map preserves scheme (https) and port (e.g. https://gitlab.internal:8443/api/v4). ssh:// remotes parse to the same host:port as their https:// equivalents. Integration tests (httptest.NewServer end-to-end): - gitlab.attacker.example rejected before any HTTP request (0 hits) - per-host credential isolation (gitlab.com token not leaked to self-managed) - custom-port self-managed instance (REST base includes :8443) - ssh:// and https:// remotes parse to identical host:port Addresses reviewer Items 5 and S1 (PR AgentWrapper#2773 review by @whoisasx).
Add a transient Error field to ports.SCMObservation so the multi-provider dispatcher can attach a failed provider's error to Fetched=false observations without discarding the classification. Previously a rate-limited GitLab alongside a healthy GitHub produced Fetched=false placeholders but the observer never received the rate-limit signal, so GitLab was retried every tick instead of entering cooldown. The observer now checks obs.Error per-observation before persistence: rate-limit errors trigger per-provider cooldown (reusing existing rateLimitCooldown/setRateLimitCooldown machinery) while non-rate-limit errors mark the repo refresh-incomplete. The Error field is nil-ed out before persistence so the storage layer never sees provider-error classification (transient metadata, not durable state — finding 1). SCMCredentialsAvailable stops discarding transient credential errors: when no provider reports usable credentials, the first real error is returned so CheckCredentialsOnce retries on the next poll rather than definitively disabling SCM observation until daemon restart. Addresses reviewer Item 7 (per-provider failure metadata) and Item 8 (credential error propagation) from PR AgentWrapper#2773 review 4729722038. Also marks issue/12 (scheme/port/ssh remotes) done — its acceptance criteria were implemented as part of ticket 01 (commit ed5c374).
775fe54 to
16ce0cb
Compare
Restrict GitLab hosts to gitlab.com plus an explicitly configured allowlist so gitlab.attacker.example is rejected before any credential is attached. Adds a GitLab sub-struct on config.Config (matching TelemetryConfig pattern) loaded once at boot from env vars (no hot-reload): AO_GITLAB_ALLOWED_HOSTS comma-separated self-managed hosts (may include :port) AO_GITLAB_HOST_TOKENS host=token,host=token per-host credential overrides The existing AO_GITLAB_TOKEN/GITLAB_TOKEN continues to apply to gitlab.com; allowlisted hosts without an explicit per-host token fall back to the default. The provider builds an internal host→TokenSource map at NewProvider time; clientForHost selects the right source per host. The TokenSource interface is unchanged. The per-host client map preserves scheme (https) and port (e.g. https://gitlab.internal:8443/api/v4). ssh:// remotes parse to the same host:port as their https:// equivalents. Integration tests (httptest.NewServer end-to-end): - gitlab.attacker.example rejected before any HTTP request (0 hits) - per-host credential isolation (gitlab.com token not leaked to self-managed) - custom-port self-managed instance (REST base includes :8443) - ssh:// and https:// remotes parse to identical host:port Addresses reviewer Items 5 and S1 (PR AgentWrapper#2773 review by @whoisasx).
Add a transient Error field to ports.SCMObservation so the multi-provider dispatcher can attach a failed provider's error to Fetched=false observations without discarding the classification. Previously a rate-limited GitLab alongside a healthy GitHub produced Fetched=false placeholders but the observer never received the rate-limit signal, so GitLab was retried every tick instead of entering cooldown. The observer now checks obs.Error per-observation before persistence: rate-limit errors trigger per-provider cooldown (reusing existing rateLimitCooldown/setRateLimitCooldown machinery) while non-rate-limit errors mark the repo refresh-incomplete. The Error field is nil-ed out before persistence so the storage layer never sees provider-error classification (transient metadata, not durable state — finding 1). SCMCredentialsAvailable stops discarding transient credential errors: when no provider reports usable credentials, the first real error is returned so CheckCredentialsOnce retries on the next poll rather than definitively disabling SCM observation until daemon restart. Addresses reviewer Item 7 (per-provider failure metadata) and Item 8 (credential error propagation) from PR AgentWrapper#2773 review 4729722038. Also marks issue/12 (scheme/port/ssh remotes) done — its acceptance criteria were implemented as part of ticket 01 (commit ed5c374).
Thank you for the second pass. All six remaining blockers plus the six inline findings and the four additional items are addressed, split into one commit per finding for reviewability. Build and vet are clean; 539 tests pass across the gitlab, multi, cli, session, and observe/scm packages. Six remaining blockers:
Six inline findings: nested namespaces (593692e), cross-provider display identity (8d4489f), pagination trust (56f526a), log memory bounds (a1ff90b), job completeness (ebe4c97 + 16ce0cb), mixed-provider error propagation (2017bb3). Four additional items: scheme/port + Requested tests: observer regression tests for second-poll/cursor and cooldown cases — Happy to restructure or expand any of these if you'd prefer a different breakdown. |
Restrict GitLab hosts to gitlab.com plus an explicitly configured allowlist so gitlab.attacker.example is rejected before any credential is attached. Adds a GitLab sub-struct on config.Config (matching TelemetryConfig pattern) loaded once at boot from env vars (no hot-reload): AO_GITLAB_ALLOWED_HOSTS comma-separated self-managed hosts (may include :port) AO_GITLAB_HOST_TOKENS host=token,host=token per-host credential overrides The existing AO_GITLAB_TOKEN/GITLAB_TOKEN continues to apply to gitlab.com; allowlisted hosts without an explicit per-host token fall back to the default. The provider builds an internal host→TokenSource map at NewProvider time; clientForHost selects the right source per host. The TokenSource interface is unchanged. The per-host client map preserves scheme (https) and port (e.g. https://gitlab.internal:8443/api/v4). ssh:// remotes parse to the same host:port as their https:// equivalents. Integration tests (httptest.NewServer end-to-end): - gitlab.attacker.example rejected before any HTTP request (0 hits) - per-host credential isolation (gitlab.com token not leaked to self-managed) - custom-port self-managed instance (REST base includes :8443) - ssh:// and https:// remotes parse to identical host:port Addresses reviewer Items 5 and S1 (PR AgentWrapper#2773 review by @whoisasx).
Add a transient Error field to ports.SCMObservation so the multi-provider dispatcher can attach a failed provider's error to Fetched=false observations without discarding the classification. Previously a rate-limited GitLab alongside a healthy GitHub produced Fetched=false placeholders but the observer never received the rate-limit signal, so GitLab was retried every tick instead of entering cooldown. The observer now checks obs.Error per-observation before persistence: rate-limit errors trigger per-provider cooldown (reusing existing rateLimitCooldown/setRateLimitCooldown machinery) while non-rate-limit errors mark the repo refresh-incomplete. The Error field is nil-ed out before persistence so the storage layer never sees provider-error classification (transient metadata, not durable state — finding 1). SCMCredentialsAvailable stops discarding transient credential errors: when no provider reports usable credentials, the first real error is returned so CheckCredentialsOnce retries on the next poll rather than definitively disabling SCM observation until daemon restart. Addresses reviewer Item 7 (per-provider failure metadata) and Item 8 (credential error propagation) from PR AgentWrapper#2773 review 4729722038. Also marks issue/12 (scheme/port/ssh remotes) done — its acceptance criteria were implemented as part of ticket 01 (commit ed5c374).
Restrict GitLab hosts to gitlab.com plus an explicitly configured allowlist so gitlab.attacker.example is rejected before any credential is attached. Adds a GitLab sub-struct on config.Config (matching TelemetryConfig pattern) loaded once at boot from env vars (no hot-reload): AO_GITLAB_ALLOWED_HOSTS comma-separated self-managed hosts (may include :port) AO_GITLAB_HOST_TOKENS host=token,host=token per-host credential overrides The existing AO_GITLAB_TOKEN/GITLAB_TOKEN continues to apply to gitlab.com; allowlisted hosts without an explicit per-host token fall back to the default. The provider builds an internal host→TokenSource map at NewProvider time; clientForHost selects the right source per host. The TokenSource interface is unchanged. The per-host client map preserves scheme (https) and port (e.g. https://gitlab.internal:8443/api/v4). ssh:// remotes parse to the same host:port as their https:// equivalents. Integration tests (httptest.NewServer end-to-end): - gitlab.attacker.example rejected before any HTTP request (0 hits) - per-host credential isolation (gitlab.com token not leaked to self-managed) - custom-port self-managed instance (REST base includes :8443) - ssh:// and https:// remotes parse to identical host:port Addresses reviewer Items 5 and S1 (PR AgentWrapper#2773 review by @whoisasx).
faf13e5 to
3a6124d
Compare
multi.FetchPullRequests partitions refs by provider key, processes each group independently, and returns partial results with nil error when at least one group succeeds. Error returned only when all groups fail. Addresses review finding AgentWrapper#5.
Describes REST as a deliberate decision for raw trace access and API compatibility across GitLab.com and self-managed instances, rather than claiming GraphQL does not support pipelines or approvals. Addresses review finding AgentWrapper#9.
mergeabilityFromMR switch extended for all current detailed_merge_status values: mergeable, conflict, checking, preparing, need_rebase, requested_changes, not_open, merge_request_blocked, etc. Legacy aliases (can_be_merged, cannot_be_merged, unchecked) retained for older self-managed installations. Also includes gitlabRateLimited helper and clientForHost call sites needed for the mergeability tests to compile. Addresses review finding AgentWrapper#3.
Finding 1 (transient failures): FetchPullRequests propagates the first sub-fetch error as a non-nil return and leaves a Fetched=false placeholder. Observer rejects Fetched=false observations so last durable state is preserved and ETags/cursors are not advanced. Finding 7 (approvals): approvalDecision trusts the approved bool rather than comparing len(approved_by) with approvals_required. FetchReviewThreads fetches approvals once and reuses for both decision and review summaries. Review summaries get stable IDs (approval:<username>). Finding 8 (pagination): doGETPaginated follows Link rel=next headers, capped at 10 pages, and returns a truncated flag. Partial is set from the truncation signal, not from item count. Also includes clientForHost call sites in CommitChecksGuard and fetchSingleMR (finding 6), observer rate-limit cooldown filtering (finding 4), and remaining test coverage for all findings. Addresses review findings AgentWrapper#1, AgentWrapper#7, and AgentWrapper#8.
Restrict GitLab hosts to gitlab.com plus an explicitly configured allowlist so gitlab.attacker.example is rejected before any credential is attached. Adds a GitLab sub-struct on config.Config (matching TelemetryConfig pattern) loaded once at boot from env vars (no hot-reload): AO_GITLAB_ALLOWED_HOSTS comma-separated self-managed hosts (may include :port) AO_GITLAB_HOST_TOKENS host=token,host=token per-host credential overrides The existing AO_GITLAB_TOKEN/GITLAB_TOKEN continues to apply to gitlab.com; allowlisted hosts without an explicit per-host token fall back to the default. The provider builds an internal host→TokenSource map at NewProvider time; clientForHost selects the right source per host. The TokenSource interface is unchanged. The per-host client map preserves scheme (https) and port (e.g. https://gitlab.internal:8443/api/v4). ssh:// remotes parse to the same host:port as their https:// equivalents. Integration tests (httptest.NewServer end-to-end): - gitlab.attacker.example rejected before any HTTP request (0 hits) - per-host credential isolation (gitlab.com token not leaked to self-managed) - custom-port self-managed instance (REST base includes :8443) - ssh:// and https:// remotes parse to identical host:port Addresses reviewer Items 5 and S1 (PR AgentWrapper#2773 review by @whoisasx).
Add a transient Error field to ports.SCMObservation so the multi-provider dispatcher can attach a failed provider's error to Fetched=false observations without discarding the classification. Previously a rate-limited GitLab alongside a healthy GitHub produced Fetched=false placeholders but the observer never received the rate-limit signal, so GitLab was retried every tick instead of entering cooldown. The observer now checks obs.Error per-observation before persistence: rate-limit errors trigger per-provider cooldown (reusing existing rateLimitCooldown/setRateLimitCooldown machinery) while non-rate-limit errors mark the repo refresh-incomplete. The Error field is nil-ed out before persistence so the storage layer never sees provider-error classification (transient metadata, not durable state — finding 1). SCMCredentialsAvailable stops discarding transient credential errors: when no provider reports usable credentials, the first real error is returned so CheckCredentialsOnce retries on the next poll rather than definitively disabling SCM observation until daemon restart. Addresses reviewer Item 7 (per-provider failure metadata) and Item 8 (credential error propagation) from PR AgentWrapper#2773 review 4729722038. Also marks issue/12 (scheme/port/ssh remotes) done — its acceptance criteria were implemented as part of ticket 01 (commit ed5c374).
c977dcc to
122d8bf
Compare
Restrict GitLab hosts to gitlab.com plus an explicitly configured allowlist so gitlab.attacker.example is rejected before any credential is attached. Adds a GitLab sub-struct on config.Config (matching TelemetryConfig pattern) loaded once at boot from env vars (no hot-reload): AO_GITLAB_ALLOWED_HOSTS comma-separated self-managed hosts (may include :port) AO_GITLAB_HOST_TOKENS host=token,host=token per-host credential overrides The existing AO_GITLAB_TOKEN/GITLAB_TOKEN continues to apply to gitlab.com; allowlisted hosts without an explicit per-host token fall back to the default. The provider builds an internal host→TokenSource map at NewProvider time; clientForHost selects the right source per host. The TokenSource interface is unchanged. The per-host client map preserves scheme (https) and port (e.g. https://gitlab.internal:8443/api/v4). ssh:// remotes parse to the same host:port as their https:// equivalents. Integration tests (httptest.NewServer end-to-end): - gitlab.attacker.example rejected before any HTTP request (0 hits) - per-host credential isolation (gitlab.com token not leaked to self-managed) - custom-port self-managed instance (REST base includes :8443) - ssh:// and https:// remotes parse to identical host:port Addresses reviewer Items 5 and S1 (PR AgentWrapper#2773 review by @whoisasx).
Add a transient Error field to ports.SCMObservation so the multi-provider dispatcher can attach a failed provider's error to Fetched=false observations without discarding the classification. Previously a rate-limited GitLab alongside a healthy GitHub produced Fetched=false placeholders but the observer never received the rate-limit signal, so GitLab was retried every tick instead of entering cooldown. The observer now checks obs.Error per-observation before persistence: rate-limit errors trigger per-provider cooldown (reusing existing rateLimitCooldown/setRateLimitCooldown machinery) while non-rate-limit errors mark the repo refresh-incomplete. The Error field is nil-ed out before persistence so the storage layer never sees provider-error classification (transient metadata, not durable state — finding 1). SCMCredentialsAvailable stops discarding transient credential errors: when no provider reports usable credentials, the first real error is returned so CheckCredentialsOnce retries on the next poll rather than definitively disabling SCM observation until daemon restart. Addresses reviewer Item 7 (per-provider failure metadata) and Item 8 (credential error propagation) from PR AgentWrapper#2773 review 4729722038. Also marks issue/12 (scheme/port/ssh remotes) done — its acceptance criteria were implemented as part of ticket 01 (commit ed5c374).
Summary
Adds GitLab as a first-class SCM provider alongside GitHub. The daemon's SCM observer, session PR claiming, and CLI now support both providers through a new multi-provider dispatcher that routes calls based on the parsed repository host.
The GitLab adapter is REST-only (unlike GitHub which uses GraphQL) because GitLab's GraphQL coverage for CI pipelines and MR approvals is incomplete. Token resolution chains
AO_GITLAB_TOKEN→GITLAB_TOKEN→glab auth status --show-tokenshell-out, usingAuthorization: Bearerfor OAuth2 + PAT compatibility.Changes
New adapter packages (
backend/internal/adapters/scm/gitlab/):auth.go— token source chain: env vars →glab auth statusshell-out, with TTL-based cachingclient.go— REST API v4 client with internal ETag caching and externally-managed ETag support for the observerobserver_provider.go— fullscmobserve.Providerimplementation: MR list, MR detail, CI pipelines/jobs, approvals, mergeability mapping, review threads/discussions, failed check log tailprovider.go— provider struct, constructor, credential availability checkdoc.go— package documentation with state mapping tables (GitLab states → domain types)Multi-provider dispatcher (
backend/internal/adapters/scm/multi/):provider.go— newProvidertype that routesParseRepository, guard calls,FetchPullRequests, and review/log fetches to the correct sub-provider based onSCMRepo.Provider;FetchPullRequestspartitions refs by provider key and batches each group independentlyDaemon wiring (
backend/internal/daemon/):scm_wiring.go— both GitHub and GitLab providers registered at startup; missing credentials for one provider logs a warning but does not block the other; observer disables itself only when no provider has usable credentialslifecycle_wiring.go— session service usesnewMultiSCMProviderinstead of GitHub-only providerCLI (
backend/internal/cli/pr_ref.go):/-/merge_requests/N), including nested groups (e.g.group/subgroup/repo/-/merge_requests/42); SSH remote parsing now handles any hostSession service (
backend/internal/service/session/claim_pr.go):parseGitHubPRURL→parsePRURL(host-aware),githubRepoFromURL→repoFromURL(host-aware),requireSameGitHubRepo→requireSameRepo; newproviderKey()maps host →"github"/"gitlab";prURLFromParts()constructs correct URL format per providerAPI contract:
backend/internal/httpd/controllers/dto.go—SessionPRSummary.Providerenum expanded from"github"to"github" | "gitlab"backend/internal/httpd/apispec/openapi.yaml— regeneratedfrontend/src/api/schema.ts— regeneratedFrontend (
frontend/src/renderer/lib/pr-display.ts):Testing
go test ./internal/adapters/scm/gitlab/...— 84 tests passgo test ./internal/adapters/scm/multi/...— 11 tests passgo test ./internal/cli/...— 160 tests pass (including new GitLab MR claim-pr and spawn tests)go test ./internal/service/session/...— 309 tests pass (including GitLab MR claim, normalize, and same-repo tests)go test ./internal/httpd/...— 75 tests pass (spec parity + drift checks)go test ./internal/daemon/...— 7 tests passgolangci-lint— 0 issuesnpm run typecheck(frontend) — pre-existing errors only, no new errors introducedIntentional omissions
approved/approvals_required > granted/none)invalidateTokenon 401/403 (unlike GitHub) — theglabtoken source has its own TTL-based cache;tokenInvalidatorinterface andFallbackTokenSource.InvalidateTokenare wired for future use