Repository navigation
refactor(#7798): add GitLab pipeline-variable override-role methods - #7813
Conversation
|
🤖 Finished Review · ✅ Success · Started 9:54 PM UTC · Completed 10:11 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.44 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsAdditive forge primitive implementation with low metadata risk and tight issue alignment, balanced by active churn and regression history in core forge packages. Previous runRisk Assessment: moderate (2/5) DetailsAdditive forge primitive implementation with low metadata risk and tight issue alignment, balanced by active churn and regression history in core forge packages. |
ReviewFindingsLow
The prior review's The forge.Client interface extension is additive only — all three implementers ( Previous runReviewFindingsLow
The prior review's Next steps:
Previous run (2)ReviewFindingsLow
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 10:13 PM UTC · Completed 10:22 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.62 |
Address review feedback on PR #7813: - Reject any role passed to SetPipelineVariablesMinimumOverrideRole that isn't one of the four documented GitLab values, via a shared forge.ValidatePipelineVarOverrideRole helper used by both the GitLab LiveClient and FakeClient, so typos/unknown values are rejected before a PUT is issued or a value is stored. - Initialize PipelineVarOverrideRoles in NewFakeClient() alongside its sibling maps, matching the direct-assignment seeding convention used elsewhere in fake_test.go. - Document that GetPipelineVariablesMinimumOverrideRole returns ("", nil) when GitLab's response omits the field (older GitLab or an edition without it), since that's indistinguishable from an explicitly empty role. Addresses #7813
🔧 Fix agent — iteration 1 (bot-triggered)Addressed all four review findings on PR #7813: added a shared forge.ValidatePipelineVarOverrideRole helper enforcing the four documented GitLab roles, wired it into both the GitLab LiveClient and FakeClient Set methods, initialized the previously-missing PipelineVarOverrideRoles map in NewFakeClient(), documented the ambiguous empty-string-on-absent-field read behavior on the interface and LiveClient doc comments, and added regression tests for each. Also confirmed the PR title should be refactor(#7798) rather than feat(#7798) per COMMITS.md, but could not apply the retitle myself — the sandbox blocked the gh pr edit mutation by policy — so recommending a human/post-script apply it before merge. All tests and go vet pass on internal/forge/... (including github, gitlab, jira) with -race; gofmt is clean. pre-commit could not fetch remote hook repositories (same infra limitation noted in the original PR), so its applicable local checks (gofmt, go vet) were run directly instead. Fixed (4):
Tests: passed CI inspections (12):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:24 PM UTC · Completed 10:36 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.13 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix Rebase. Resolve all conflicts. Fix all review issues. Reply to and resolve all threads. |
|
🤖 Finished Fix · ✅ Success · Started 10:55 PM UTC · Completed 11:06 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.18 |
Add forge.Client methods to read and set GitLab's ci_pipeline_variables_minimum_override_role project setting (GitLab >= 17.1). This is the Wave 0 primitive #7769 needs before it can tighten who may override CI_JOB_TOKEN and other pipeline variables. GitLab LiveClient GETs and PUTs /projects/:id. GitHub stubs return ErrNotSupported. FakeClient round-trips the stored role. 404 maps to ErrNotFound and 403 to ErrForbidden, matching sibling project-settings methods. Note: pre-commit could not fetch remote hook repos (HTTP 403). gofmt, go vet, gitleaks, and local file checks were run directly on the changed files. Closes #7798
Address review feedback on PR #7813: - Reject any role passed to SetPipelineVariablesMinimumOverrideRole that isn't one of the four documented GitLab values, via a shared forge.ValidatePipelineVarOverrideRole helper used by both the GitLab LiveClient and FakeClient, so typos/unknown values are rejected before a PUT is issued or a value is stored. - Initialize PipelineVarOverrideRoles in NewFakeClient() alongside its sibling maps, matching the direct-assignment seeding convention used elsewhere in fake_test.go. - Document that GetPipelineVariablesMinimumOverrideRole returns ("", nil) when GitLab's response omits the field (older GitLab or an edition without it), since that's indistinguishable from an explicitly empty role. Addresses #7813
🔧 Fix agent — iteration 2 (human-triggered)Rebased PR #7813 onto origin/main and resolved conflicts in 5 files where PR #7780's pipeline-trigger-token/webhook primitives collided with this PR's override-role primitives at the same interface/struct insertion points, keeping both sets of additions. Verified with build, vet, gofmt, the repo's interface-doc-sync lint, gitleaks, and Fixed (5):
Disagreed (1):
Tests: passed CI inspections (10):
Decision points
Next steps:
|
1da82fb to
a89c97b
Compare
|
🤖 Finished Review · ✅ Success · Started 11:08 PM UTC · Completed 11:21 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.82 |
Superseded by updated review
fullsend-ai-review
Bot
added
the
ready-for-merge
ggallen
changed the title
…onverge Uses the forge primitive from #7813, Get/SetPipelineVariablesMinimumOverrideRole, to read and converge each enrolled GitLab repo's ci_pipeline_variables_minimum_override_role toward "owner" at both fresh install and converge/repair time, and report it as already-correct when it is. Idempotent: an already-owner project is a no-op every time. Active enforcement is gated behind FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION=enforced and defaults to report-only. Restricting the setting to owner also blocks the Developer-level poller/dispatcher's own CreatePipeline call with variables, internal/poll/dispatch.go, unless the poller identity is separately raised to Owner. internal/repos/gitlab_pipeline_access.go and gitlab_roles.go are built assuming a Developer-level poller throughout, and ADR 0125 documents that credential-cost tradeoff as an open, ship-gating decision, not a free hardening knob. Flipping the setting to owner fleet-wide before that follow-up ships would break dispatch on every enrolled repo, so this change wires and tests the primitive without forcing that call. Adds fresh-install, converge/repair, and already-correct coverage via the fake forge client, plus GitHub/ErrNotSupported and injected-error cases. Updates gitlab-role-credentials.md to point at the new wiring and the still-open credential decision. Addresses #7817
Summary
Add
forge.Clientmethods to read and set GitLab'sci_pipeline_variables_minimum_override_roleproject setting (GitLab >= 17.1). This is the Wave 0 primitive that #7769 needs before it can tighten who may overrideCI_JOB_TOKENand other pipeline variables.This change does not wire the setting into the security gate or install-time provisioning; that remains #7769 / #7772.
Related Issue
#7798 (prerequisite for #7769; coordinates with #7767 / #7772)
Changes
forge.Clientmethods:GetPipelineVariablesMinimumOverrideRoleandSetPipelineVariablesMinimumOverrideRole.no_one_allowed,developer,maintainer,owner.LiveClientGET/PUT on/api/v4/projects/:idwithci_pipeline_variables_minimum_override_role.forge.ErrNotSupported.FakeClientstores and round-trips the role value, including error injection and race coverage.Testing
go test -raceon./internal/forge/,./internal/forge/github/,./internal/forge/gitlab/gofmtandgo veton changed packagesmake lint— sandboxpre-commitcould not fetch remote hook repos (HTTP 403); ran gofmt/vet/gitleaks and local file checks directlyChecklist
!for breaking changes)Closes #7798
Post-script verification
agent/7798-gitlab-pipeline-override-role)1e5e984ab1c70501f4759eca73680f05ddfd14b0..HEAD)