Feat/add gitlab mr approval controls by Joseph94m · Pull Request #423 · getplumber/plumber

Joseph94m · 2026-08-17T17:24:20Z

Closes #413
Closes #414

Test on https://gitlab.com/getplumber/examples/go-test-with-hash @thomasboni

thomasboni · 2026-08-21T13:06:28Z

@Joseph94m Reviewed + tested e2e. Also checked legacy fidelity line-by-line against the backend2 controls. All of this by Claude. I've verified its results.

Blocker

An approval rule with an empty name is invisible to ISSUE-502. MRApprovalRule.Name is serialized with json:"name,omitempty" (internal/ir/pipeline.go), and mr_approval_rules_min_approvals.rego references rule.name bare in the message sprintf and the ruleName data field. When the key is absent the whole deny body is undefined, so no finding is emitted. Reproduced end to end: a rule {id: 99, name: "", rule_type: "any_approver", approvals_required: 0, applies_to_all_protected_branches: true} with minimumRequiredApprovals: 2 yields status: passed, zero issues. The same rule with a name is flagged. GitLab API-created and auto-created rules can carry empty names, so this is a realistic silent false negative on exactly the violation class the control exists for. Fix: object.get(rule, "name", "") in the rego (and/or drop the omitempty), plus the unnamed-rule case in policies/rules_test.go. Identity keys on approvalRuleId, so the fix re-keys nothing.
gitlab/dataCollectionGitlabProtection.go classifies 403/404 by substring on the error string, and this PR makes that drive MRApprovalRulesKnown and a user-visible status. The error string embeds the request URL, so a project whose ID contains "403"/"404" returning a 500 gets misclassified as premium-missing (safe direction, Known=false, but a hard failure is silently downgraded). Prefer the typed response status code.
The config key renames the platform's minimumRequiredApprovalAllProtectedBranches to minimumRequiredApprovals and the mapping is recorded nowhere (FINGERPRINT.md covers the identity correction only). Write it down for the platform migration.
Same terminal-rendering gap as Feat/add gitlab variables controls #422: on a 403 the JSON says error but the report shows both controls as a bare green check under "Passed Controls". Needs the caveat stat line (one shared fix for both PRs).
There is still one not-resolved Claude code comment review in the PR

Nits

enabled: true without minimumRequiredApprovals is a silent no-op (rego defaults the minimum to 0). Matches legacy and is documented, but a config-load warning would save users from a control that can never fire.
Collector/IR comments say non-premium yields 403/404 while the rego/defaultConfig comments say Free returns 200-empty; both paths are handled, harmonize the wording.
Docs say "401/403"; a genuine 401 actually aborts the whole protection collection (outcome still safe via nil ProtectionData).
404 branch of the tolerance is untested (403 is covered, same path, cheap to add).
Init wizard: MRApprovalMinCount is unvalidated free text; "0" produces an enabled no-op, non-numeric silently falls back.

Joseph94m · 2026-08-21T15:35:08Z

@thomasboni should be good !

Adds two GitLab controls over merge-request approval rules: - ISSUE-502 flags an approval rule that covers all protected branches yet requires fewer approvals than the configured minimum. Identity is keyed on the stable approval-rule ID, not the renameable rule name, so renaming a rule does not re-key its finding. - ISSUE-504 flags a project where no approval rule applies to all protected branches, including the zero-rules case. Both abstain when the approvals listing could not be read (MRApprovalRulesKnown=false) and report not-evaluable rather than a false pass: GitLab Free 200-empties the approvals API instead of returning 403, so an empty listing cannot be read as compliant. The renderers carry a Premium/Ultimate caveat for the ambiguous zero-rules case, and an unreadable listing shows the caveat in the terminal instead of a bare green check. Refs #412

StatusFor had no case for mergeRequestSettingsMustBeCompliant, so when the project settings could not be read the control reported a green pass on data it never saw, while its sibling ISSUE-503 correctly reported error on the same run. Add the matching guard, ahead of the CI-file checks: this control reads Settings > Merge requests, not the pipeline, so a repo with no .gitlab-ci.yml must not turn a genuine evaluation into an error either. The shipped default broke the "a setting left unset is not checked" contract that #415/#416 specify. A config inheriting plumber:default can override a value but cannot remove a key, so shipping the eight ISSUE-506 expectations and the ISSUE-503 surface populated forced every one of those opinions on anyone who merely enabled the control. They now ship commented out, so enabling a control enforces only what the operator uncomments. The MR comment dropped findings for every MR control: writeIssueDetails walks a hand-written list, and a control missing from it renders as failed in the table with no detail lines beneath. Four MR controls were absent (two from this branch, two from #423) plus externalRefsMustNotCollide, which predates both. That regression has now shipped three times, so the list moved to package scope and TestMRCommentOrderCoversEveryGitLabControl ties it to GitLabControls, the same source the table is built from. Terminal caveats for the two settings controls, matching the #423 pattern, so an unreadable payload stops printing an unqualified check. A test pins each caveat to StatusFor so the two surfaces cannot drift apart. Tests for the two gaps the review bot found in this branch's own code: the buildMRSettings projection (eight field copies, previously unexercised because every ISSUE-506 test hand-built the IR value) and the ISSUE-506 legacy JSON dispatch (an unrouted control silently loses its whole block). Not addressed here: an unreadable control still lands under Passed Controls and still counts toward the score, because neither the renderer's bucketing nor ComputeCompliance consults StatusFor. That predates this branch and hits the #422 and #423 controls identically, so it needs its own change. Refs #415, #416 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Joseph94m force-pushed the feat/add-gitlab-mr-approval-controls branch from d96ac25 to f33e099 Compare August 20, 2026 07:18

Joseph94m marked this pull request as ready for review August 20, 2026 07:52

Joseph94m requested review from Totara-thib, coconux3 and thomasboni as code owners August 20, 2026 07:52