Repository navigation
Conversation
…SUE-507) Anyone with push access could create, move or delete a release tag, and tags drive release workflows and action pins. tagMustBeProtected flags a tag pattern in use (an existing tag matches it, or a workflow triggers on it) whose tags are not immutable: updates and deletions must be blocked, and creation restricted unless requireCreationRestricted is false. The patterns checked are namePatterns (default v*) plus the GitHub on.push.tags filters, read with GitHub's filter syntax. A finding is critical when a workflow releases from tags (push tags, on: release, GitLab only: tags or a positive $CI_COMMIT_TAG rule), high otherwise. GitHub reads active tag rulesets (repository, organization, enterprise) and the tags with Metadata: read; GitLab reads protected tags and tags with read_api. A token without access, or a listing past its page cap, reports the control not evaluated, never a finding; a network failure degrades the run. In GitLab platform mode the control reports lane_not_served until the platform serves a tag lane. Refs #398 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| case "mergeRequestSettingsMustBeCompliant": | ||
| return "mrSettingsResult", buildMRSettingsBlock(common, findings) | ||
| case "tagMustBeProtected": | ||
| return "tagProtectionResult", buildTagProtectionBlock(common, result, findings) |
There was a problem hiding this comment.
🔴 Test coverage
Medium: the new buildTagProtectionBlock legacy-JSON block and its dispatch cases (both providers) have no test, so a dropped/renamed route would silently remove tagProtectionResult from results.json. (Medium)
buildTagProtectionBlock (cmd/legacy_json.go:404) and its two dispatch cases — buildLegacyResult at cmd/legacy_json.go:281-282 (GitLab) and buildLegacyResultGitHub at cmd/legacy_json_github.go:78-79 (GitHub) — are untested. This is exactly the regression class the sibling TestMRSettingsJSONBlock (cmd/legacy_json_gitlab_test.go:248) was written to guard, whose own comment warns: "an unrouted control falls through to return "", nil and its block is dropped from results.json entirely, silently, with every other test still green." No test exercises the tag route, so if either case "tagMustBeProtected" were removed or the "tagProtectionResult" name changed, the block would vanish from --output/plumber-report.json and the whole suite would stay green.
The block also has untested branch logic worth pinning: it reads result.Pipeline and falls back to result.GitHubPipeline (cmd/legacy_json.go:406-410), and gates the tagRules/tagsChecked metrics on tp.Known (line 411-413). A regression that read the wrong pipeline field or dropped the Known guard (reporting counts from an unreadable collection) would go uncaught.
Suggested tests (mirror TestMRSettingsJSONBlock): (1) buildLegacyResult with entry tagMustBeProtected and an ISSUE-507 finding returns name tagProtectionResult, a block whose issues carries the finding and whose metrics.unprotectedTagPatterns == len(findings); (2) the same via buildLegacyResultGitHub; (3) with GitHubPipeline carrying a TagProtection{Known:true, Rules:…, Tags:…}, assert tagRules/tagsChecked equal the counts, and with Known:false assert both are 0; (4) a clean run (no findings) still emits a non-nil block.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
| MinMergeAccessLevel: intPtrInit(parseIntInit(st.BranchMinMergeAccessLevel, 30)), | ||
| MinPushAccessLevel: intPtrInit(parseIntInit(st.BranchMinPushAccessLevel, 40)), | ||
| } | ||
| gl.Controls.TagMustBeProtected = defaultTagProtection() |
There was a problem hiding this comment.
🔴 Test coverage
Low: applyAccessControls now writes TagMustBeProtected on both providers but no test asserts the wizard emits it, so the mapping could silently break. (Low)
applyAccessControls now unconditionally assigns gl.Controls.TagMustBeProtected = defaultTagProtection() (cmd/init.go:1319) and gh.Controls.TagMustBeProtected = defaultTagProtection() (cmd/init.go:1356) — a new, always-on behavior of plumber config init. The existing mapping tests in cmd/init_mr_approval_test.go call applyAccessControls but assert only on the MR-approval controls; none checks TagMustBeProtected. That test's own comment (cmd/init_mr_approval_test.go:9-13) explains why this matters: "A field mix-up … would silently emit a wrong .plumber.yaml from plumber config init with no drift guard." If defaultTagProtection regressed (returned nil, Enabled:false, or empty patterns) or an assignment were dropped, the wizard would stop emitting the control and no test would fail.
Suggested test: extend TestApplyAccessControls_* so that after (&initWizardState{}).applyAccessControls(gl, gh), both gl.Controls.TagMustBeProtected and gh.Controls.TagMustBeProtected are non-nil, IsEnabled() is true, EffectiveNamePatterns() == ["v*"], and IsCreationRestrictionRequired() is true.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
| # _positive_tag_condition: some $CI_COMMIT_TAG reference that is neither | ||
| # negated (`!$CI_COMMIT_TAG`) nor compared equal to an empty value | ||
| # (`$CI_COMMIT_TAG == null`, `== ""`), the shapes that mean "not a tag". | ||
| _positive_tag_condition(cond) if { |
There was a problem hiding this comment.
🔴 Test coverage
Low: the _positive_tag_condition negation branch (!$CI_COMMIT_TAG) is never exercised, so a regression that mis-classifies a "skip on tags" guard as a tag trigger (critical vs high) would go uncaught. (Low)
_positive_tag_condition (policies/tag_unprotected.rego:165) rejects negated references via m[1] == "", so a rule like rules: - if: '!$CI_COMMIT_TAG' (a common "don't run on tag pipelines" guard) must NOT count as a positive tag condition and must leave occurrenceSeverity at high. The TestIssue507_OccurrenceSeverity cases (policies/tag_unprotected_test.go:3023) cover the positive form, == null, == "", != null, when: never, and a longer variable name — but never the leading-! negation. The false branch of m[1] == "" is therefore untested: if it regressed, !$CI_COMMIT_TAG would be read as a tag trigger and the finding would be escalated to critical.
Suggested case: a GitLab job {Rules: [{"if": "!$CI_COMMIT_TAG"}]} with a matching tag present expects occurrenceSeverity == high and triggered == false.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
…ed tag rule - tagProtectionResult is routed on both providers, carries the finding with its tagPattern, reads the counts from whichever pipeline holds the collection, reports 0 counts when it could not be read, and is still emitted on a clean run. - plumber config init emits tagMustBeProtected enabled on both providers with the v* default and creation restriction required. - A `!$CI_COMMIT_TAG` rule is no tag trigger: ISSUE-507 stays high. Refs #398 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return !anyRepoPatternMatches(c.Exclude, repo) | ||
| } | ||
|
|
||
| func anyRepoPatternMatches(patterns []string, repo string) bool { |
There was a problem hiding this comment.
🔴 Test coverage
Medium: the GitHub org-ruleset applicability logic (rulesetAppliesToRepo / anyRepoPatternMatches) is never tested for the ~ALL repository_name sentinel or the exclude branch, so a regression that drops org-wide tag rulesets would ship silently and produce false ISSUE-507 findings. (Medium)
anyRepoPatternMatches (github/github_tag_protection.go:195) special-cases p == "~ALL" to mean "all repositories", which is exactly how GitHub represents an organization ruleset that targets every repo — the most common org-ruleset shape. rulesetAppliesToRepo (line 184) also honours conditions.repository_name.exclude via return !anyRepoPatternMatches(c.Exclude, repo).
The only test that exercises repository_name conditions is TestFetchGitHubTagProtection_MapsActiveRulesetsAndTags, which uses include: ["r*"] (matches repo r) and a second ruleset with include: ["elsewhere"] (does not match). Neither the ~ALL include branch nor the exclude branch is ever evaluated.
Concrete regression this misses: an org-wide tag ruleset with "repository_name": {"include": ["~ALL"]} that fully protects v*. If the ~ALL short-circuit regressed (e.g. someone changes the sentinel string or the comparison), anyRepoPatternMatches returns false, rulesetAppliesToRepo returns false, the ruleset is discarded, and ISSUE-507 fires as critical on a v* pattern that is in fact protected — a false positive on the exact case the design works hardest to avoid. Likewise, a bug inverting the exclude logic (dropping a ruleset whose exclude does not match, or keeping one whose exclude does match) is uncaught.
Suggested tests (in github/github_tag_protection_test.go): (1) an active org ruleset with repository_name.include: ["~ALL"] and full tag rules on v* must yield a rule that rulesetAppliesToRepo accepts, so FetchGitHubTagProtection returns it (and downstream, no finding); (2) an org ruleset whose repository_name.exclude matches the analysed repo must be dropped from Rules; (3) an org ruleset whose exclude does not match the repo must be kept.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
| orderBy := "updated" | ||
| sort := "desc" | ||
| tagOptions := &glab.ListTagsOptions{ListOptions: glab.ListOptions{PerPage: perPage}, OrderBy: &orderBy, Sort: &sort} | ||
| for page := int64(1); page <= maxTagListingPages; page++ { |
There was a problem hiding this comment.
🔴 Test coverage
Low: on GitLab, a failure or page-cap truncation of the repository/tags listing in FetchProjectTagProtectionData is untested, unlike the GitHub collector which tests both — so a regression that swallows the tags-fetch error (turning "not evaluated" into a false pass on partial data) would go undetected. (Low)
FetchProjectTagProtectionData fetches protected tags and the tag names. The tag-names loop (gitlab/collect_tag_protection.go:99-114) has two behaviours with no test coverage:
-
Tags-endpoint error (line 102-104): if
client.Tags.ListTagsfails, the function returns the error soCollectGitlabTagProtectionyieldsKnown=false→ control reports "not evaluated". The test helpertagProtectionServerhardcodes therepository/tagsendpoint to always return 200 (onlyprotectedStatusis configurable), so no test exercises a tags-endpoint failure. A regression that swallowed this error (returning the partialtagsslice withnilerr) would flipKnownto true with an incomplete tag set, silently hiding ISSUE-507 findings. The GitHub collector covers the parallel case (TestFetchGitHubTagProtection_ForbiddenOrMissingIsUnknown"tags 401",TestFetchGitHubTagProtection_ServerErrorIsReturned). -
Tags truncation (line 113-114): exceeding
maxTagListingPagesreturnstruncated=true. The GitHub side hasTestFetchGitHubTagProtection_TagListingCap; GitLab has no equivalent. A regression here (e.g. erroring on the cap like the protected-tags loop does, instead of returningtruncated) would make a repo with >1000 tags report "not evaluated" instead of checking the first 1000.
Suggested tests (in gitlab/collect_tag_protection_test.go): add a case where protected_tags returns 200 but repository/tags returns 500/403 — expect an error and Known=false; and a case where repository/tags returns 10 full pages — expect TagsTruncated=true, Known=true.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
| _workflow_tag_patterns contains p if { | ||
| some job in _jobs | ||
| _live(job) | ||
| some raw in object.get(job, "pushTags", []) |
There was a problem hiding this comment.
🔴 New control check
Medium — GitHub workflows that trigger on tag pushes via on.push.tags-ignore are not recognized as tag triggers, so the control under-reports severity (high instead of critical) and can miss the pattern entirely. (Medium)
The control discovers workflow-driven tag patterns and decides the "triggered" (critical) severity purely from on.push.tags and on: release:
_workflow_tag_patterns(policies/tag_unprotected.rego:71-79) reads onlyobject.get(job, "pushTags", [])._triggered(policies/tag_unprotected.rego:126-138) fires only on apushTagspattern,"release" in triggers, or GitLab tag intent.
The IR has a separate PushTagsIgnore field (internal/ir/pipeline.go:486-487, populated by extractGitHubPushFilters in github/github_workflows.go:1602 from tags-ignore). The Rego never references it, and a tags-ignore-only on: push yields Triggers: ["push"] (not "release") and an empty pushTags.
Concrete failure scenario. A release workflow that builds on every tag except pre-releases:
# .github/workflows/release.yml
on:
push:
tags-ignore:
- '*-rc*'
jobs:
publish:
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v4
- run: ./publish.shThis workflow genuinely runs on tag pushes (e.g. v1.0.0, 2024.1), so per the documented contract it should be critical (docs/GITHUB_ISSUES.md: "critical when a workflow runs on a tag push ... high otherwise").
- If the repo has
v*tags, the pattern is still flagged via the defaultnamePatterns+ existing tag, but_triggeredis false, sooccurrenceSeverityis high instead of critical — a severity misclassification that reaches scoring (control/scoring_v4.goreadsoccurrenceSeverity). - If the repo's release tags do not match the configured
namePatterns(e.g. calver tags2024.1with the defaultv*), thetags-ignoreworkflow contributes no pattern at all, nothing is_in_use, and the unprotected release-triggering tags are not flagged — a true false negative.
Suggested fix. Treat a live GitHub job whose on: push declares tags-ignore (and no tags) as a tag trigger in _triggered/_occurrence_severity (so it elevates to critical like on: release does), and add a test fixture covering an on.push.tags-ignore release workflow to policies/tag_unprotected_test.go / the TestIssue507_OccurrenceSeverity table.
Control: .github/claude-prompts/new-control-check.md · resolve this conversation once addressed.
A GitHub workflow on `push: tags-ignore` runs on every tag it does not ignore, so a moved tag ships through it like through `on: release`. It now raises the finding to critical; like the other pattern-less signals it does not put a pattern in use on its own. Also pins, from the PR review: - organization ruleset applicability: the ~ALL repository sentinel, name patterns and the exclude list; - the GitLab tag listing: a failed listing is unknown, and a listing past the page cap is checked up to the cap with tagsTruncated. Refs #398 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // into "not evaluated". Returns true when the fetch failed outright | ||
| // (network, 5xx, rate limit), so the caller degrades the run instead of | ||
| // letting an empty answer pass (#220). | ||
| func enrichGitHubTags(l *logrus.Entry, pipeline *ir.NormalizedPipeline, host string, conf *configuration.Configuration, projectPath string) (fetchFailed bool) { |
There was a problem hiding this comment.
🔴 Test coverage
Medium: the new enrichGitHubTags function and its use in RunGitHubAnalysis/RunGitHubAnalysisRemote have no test coverage, so a regression that silently disables the GitHub tag control or drops the degrade-on-fetch-failure path would not be caught. (Medium)
enrichGitHubTags (control/task_github.go:333) is new logic with several branches — the pipeline == nil || !tagLaneCollected(...) gate, the pipeline.TagProtection = tp assignment, the error path that returns fetchFailed = true, and the tp != nil && !tp.Known info path — and none of them is exercised by any test. Its callers (control/task_github.go:560 and :715) and the follow-on wiring if tagFetchFailed { markDegraded(...) } plus markTagProtectionGap(result, GitHubControls(...)) (:611, :761) are also untested. Grep confirms enrichGitHubTags / tagFetchFailed / FetchGitHubTagProtection appear only in control/task_github.go and github/github_tag_protection*.go; unlike the GitLab path there is no RunGitHubAnalysis integration test that sets up rulesets/tags (control/platform_call_inventory_test.go is GitLab-only, and control/tag_protection_test.go only unit-tests the helper functions on fabricated AnalysisResults).
Concrete regressions this lets through:
- If the
pipeline.TagProtection = tpassignment were dropped ortagLaneCollectedwiring changed,tagMustBeProtectedwould silently report "not evaluated" on every GitHub repository (the pipeline never carries tag data), yet every existing unit test (FetchGitHubTagProtection_*,TestTagLaneCollected) would still pass. - If
enrichGitHubTagsstopped returningtrueon a 5xx/network failure, a GitHub tag/ruleset fetch that hits a 500 would no longer mark the runDataCollectionDegraded; no test assertstagFetchFailed→markDegraded.
Suggested test: add a RunGitHubAnalysis (or direct enrichGitHubTags) test using an httptest server (as github/github_tag_protection_test.go already does) for three scenarios — (a) a readable repo where pipeline.TagProtection.Known becomes true and fetchFailed is false; (b) a 500 where fetchFailed is true and the run is marked degraded with degradedReasonTagProtectionPrefix; (c) a 403 where fetchFailed is false, Known is false, and the control ends up not_evaluable via markTagProtectionGap.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
| var tErr error | ||
| tagData, tErr = gitlab.CollectGitlabTagProtection(conf.ProjectPath, conf.GitlabToken, conf) | ||
| if tErr != nil { | ||
| if isNetworkError(tErr) { |
There was a problem hiding this comment.
🔴 Test coverage
Medium: the GitLab RunAnalysis tag-collection failure split (network error degrades the run vs. permission error only marks the control not-evaluated) is untested, so mis-classifying a fetch failure would silently change the run's degraded status without any test failing. (Medium)
In RunAnalysis (control/task.go:1294-1298) a failed CollectGitlabTagProtection is split: if isNetworkError(tErr) { markDegraded(...) }, otherwise the error only produces a warning and the control is later marked not-evaluated by markTagProtectionGap (:1345). This decision is new and has no test. The only integration coverage for the GitLab tag lane (control/platform_call_inventory_test.go, which enables tagMustBeProtected and asserts the /protected_tags and /repository/tags calls) exercises the success path with empty [] bodies; the error branch is never hit at the RunAnalysis level, and the unit tests in gitlab/collect_tag_protection_test.go stop at CollectGitlabTagProtection returning (Known=false, err) and never reach this classification.
Concrete regression: if the isNetworkError branch were inverted or removed (e.g. during a refactor of the shared collector error handling), a genuine network/timeout failure fetching GitLab tag protection would no longer set DataCollectionDegraded — the user would see tagMustBeProtected as a silent "not evaluated" with no indication that data collection failed — and no test would catch it. Conversely a permission 403 wrongly classified as network would spuriously degrade the whole run.
Suggested test: drive RunAnalysis (or extract the split into a helper) against a GitLab server returning a 5xx/timeout on /protected_tags and assert result.DataCollectionDegraded is true with a degradedReasonTagProtectionPrefix reason; and against a 403 assert the run is not degraded but tagMustBeProtected is not_evaluable (ReasonCollectionFailed).
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
… ISSUE-507 - The GitLab tag collection moves into collectGitLabTagProtection (unchanged behavior). Against the real collector: a refused connection degrades the run, a 403 only leaves tagMustBeProtected not evaluated, a readable project is known, and a platform run or a disabled control fetches nothing. - The GitHub tag fetch gets a test seam (fetchGitHubTagProtection, like scanGitHubWorkflowsRemote). Through RunGitHubAnalysisRemote: readable data reaches the pipeline, a fetch error degrades the run, and an unreadable answer leaves the control not evaluated without degrading. Refs #398 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-510
ISSUE-507 and ISSUE-508 are retired on getplumber.io ("will not be
reused"), and ISSUE-509 still redirects to ISSUE-803 from the
pre-renumber GitHub codes. ISSUE-510 has never been used. The control
is unreleased, so the id change breaks no contract: the control id and
catalog goldens are regenerated and the fingerprint pin follows the code.
Refs #398
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Exypte
changed the title
| ) | ||
| applyGitLabVisibility(pipeline, project) | ||
| applyGitLabRegistryHost(pipeline, project, conf.GitlabURL) | ||
| pipeline.TagProtection = gitlab.BuildTagProtection(tagData) |
There was a problem hiding this comment.
🔴 Test coverage
Medium: the GitLab wiring that feeds collected tag data into the Rego engine (control/task.go:423) is never exercised to produce a finding, so a regression that drops it would silently disable tagMustBeProtected on GitLab undetected. (Medium)
runRegoEngine assigns the collected GitLab tag data into the pipeline the policy reads:
pipeline.TagProtection = gitlab.BuildTagProtection(tagData)This single line is the only thing connecting the GitLab collector (collectGitLabTagProtection → BuildTagProtection) to the tag_unprotected Rego rule, which keys off input.pipeline.tagProtection. No test exercises it.
TestRunRegoEngine_FailingPolicyDegradesTheRun(control/policy_failure_gitlab_test.go:37) callsrunRegoEnginewithtagData == niland asserts only degradation.- The
RunAnalysisintegration tests incontrol/platform_call_inventory_test.goenable the control but the fake GitLab serves[]for both/protected_tagsand/repository/tags(lines 141–142), so no tag ever matches a pattern and no ISSUE-510 finding is ever produced through the real flow. - The GitLab collector unit tests (
gitlab/collect_tag_protection_test.go,control/tag_collection_test.go) callBuildTagProtectiondirectly in a hand-builtAnalysisResult; they never go throughrunRegoEngine/RunAnalysis.
Concrete regression this leaves uncaught: if the assignment at task.go:423 were deleted or mis-wired (e.g. assigning to the wrong field, or collectGitLabTagProtection's result not being threaded into runRegoEngine), pipeline.TagProtection would stay nil, the Rego rule would abstain, and every GitLab repository with unprotected release tags would silently pass tagMustBeProtected. The entire existing test suite would still be green.
Suggested test: a RunAnalysis (or runRegoEngine) test with a GitLab fake serving a v*-matching tag (e.g. [{"name":"v1.0.0"}]) and no protected tags, asserting result.Findings contains an ISSUE-510 finding with tagPattern == "v*". A mirror test with a fully-protecting protected tag should assert no finding.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
| } | ||
| branchFetchFailed = enrichGitHubBranches(l, pipeline, conf.GithubAPIHost, branchScope, conf.ProjectPath, onProgress) | ||
| } | ||
| tagFetchFailed := enrichGitHubTags(l, pipeline, conf.GithubAPIHost, conf, conf.ProjectPath) |
There was a problem hiding this comment.
🔴 Test coverage
Medium: the tag-protection enrichment call on the local GitHub analysis path (control/task_github.go:564, RunGitHubAnalysis) has no test — only the remote variant is covered — so dropping the local call would disable the control for local-clone GitHub runs undetected. (Medium)
enrichGitHubTags is invoked from two separate entry points:
RunGitHubAnalysis(local checkout path, line 564) — untested.RunGitHubAnalysisRemote(line 719) — covered byTestRunGitHubAnalysisRemote_TagProtectionin control/tag_collection_test.go, which swapsfetchGitHubTagProtectionand asserts the pipeline is populated, the run degrades on fetch failure, and the gap is marked.
No test drives RunGitHubAnalysis (the common CLI path for an analyzed local repo) through the tag enrichment. Because the two functions duplicate the enrichment/degrade/gap wiring rather than sharing it, the local call site at line 564 plus its two follow-on lines (markDegraded on tagFetchFailed and markTagProtectionGap(result, GitHubControls(...))) are an independent, uncovered copy.
Concrete regression this leaves uncaught: if the enrichGitHubTags call (or the subsequent markTagProtectionGap) were removed from RunGitHubAnalysis, local GitHub analyses would never populate pipeline.TagProtection and would report the control as a clean pass (instead of a finding or "not evaluated"), while every test — including the remote-path test — stays green.
Suggested test: a RunGitHubAnalysis test paralleling TestRunGitHubAnalysisRemote_TagProtection — swap fetchGitHubTagProtection to return (a) a Known protection with an unprotected matching tag and assert an ISSUE-510 finding / populated result.Pipeline.TagProtection, (b) an error and assert the run degrades with a tag reason, and (c) Known=false and assert NotEvaluable[tagMustBeProtected] == ReasonCollectionFailed.
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
…bAnalysis - GitLab: RunAnalysis against the recording fake now serves a v1.0.0 tag. Unprotected, it yields an ISSUE-510 finding on v*; with a v* protected tag left to Maintainers, none. This pins the pipeline wiring in runRegoEngine. The fake serves [] by default, so the call ledgers are unchanged. - GitHub: the local RunGitHubAnalysis path gets the same table as the remote one: an unprotected tag is a finding, a failed fetch degrades the run, an unreadable answer leaves the control not evaluated. Removing either wiring line now fails its test. Refs #398 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // included, so a job-less include finding (ISSUE-404) names its include | ||
| // path instead of falling through to an empty subject. | ||
| var bestFixSubjectKeys = []string{"uses", "image", "expression", "variableName", "branchName", "url", "includePath"} | ||
| var bestFixSubjectKeys = []string{"uses", "image", "expression", "variableName", "branchName", "tagPattern", "url", "includePath"} |
There was a problem hiding this comment.
🔴 Test coverage
Low: the newly added tagPattern entry in bestFixSubjectKeys has no test, so a regression that drops it would silently leave ISSUE-510 best-fix sentences with an empty (``) subject — the exact defect class already guarded for ISSUE-404. (Low)
bestFixSubjectKeys was changed to add "tagPattern" (control/bestfix.go:96). This matters because ISSUE-510 findings are deliberately job-less (the Rego rule at policies/tag_unprotected.rego emits no job and comments # No "job": a tag pattern is not a job.). bestFixSubject (control/bestfix.go:511-518) walks bestFixSubjectKeys and falls back to f.Job — which is empty for ISSUE-510 — so without the tagPattern key the best-fix subject becomes "" and the rendered sentence contains empty backticks (``).
That is precisely the regression TestBestFixSubjectUsesIncludePathForAJobLessFinding (control/explain_test.go:919-932) was written to prevent for the analogous job-less finding ISSUE-404 (it asserts the subject resolves to includePath and that the sentence has no (``)). No equivalent test exists for ISSUE-510, so removing or renaming tagPattern here (or in the policy's tagPattern output) would pass CI while degrading the critical tag finding's best-fix message. ISSUE-510 is critical/gate severity, so it can legitimately be selected by ComputeBestFix, making the path reachable.
Suggested test: mirror TestBestFixSubjectUsesIncludePathForAJobLessFinding for ISSUE-510 — build a job-less finding finding("ISSUE-510", "", map[string]any{"tagPattern": "v*"}), run ComputeBestFix, and assert fix.Subject == "v*" and that fix.Sentence does not contain (``).
Control: .github/claude-prompts/test-coverage.md · resolve this conversation once addressed.
ISSUE-510 findings carry no job, so the best-fix subject comes from the tagPattern key in bestFixSubjectKeys. Mirror the ISSUE-404 test: a job-less tag finding names its pattern, and the sentence has no empty backticks. Dropping the key now fails the test. Refs #398 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What type of PR is this?
feat
What this PR does / why we need it:
Plumber checks branch protection but not tags. Anyone with push access can create, move or delete a release tag, and tags drive release workflows and action pins (the tj-actions attack moved existing tags). This adds a cross-provider control,
tagMustBeProtected(CTRL-510 / ISSUE-510, gate role).namePatterns(defaultv*), plus the patterns GitHub workflows trigger on (on.push.tags, read with GitHub's filter syntax, where+and?are quantifiers). A pattern is only checked while it is in use: an existing tag matches it, or a workflow triggers on it. So a repository that never tags stays quiet.requireCreationRestricted: falseto drop the creation requirement).update,deletionandcreationrules. Each protection is judged across all rulesets, because GitHub layers them.on: release, GitLabonly: tagsor a positive$CI_COMMIT_TAGrule), and high otherwise.permissions: {}grants (checked in a real workflow run). Rulesets need a public repo or GitHub Pro+, and GHES 3.11+.read_api.plumber init, both default configs, the legacy JSON block, the catalog, the issue docs, and theaction.yml/ README token text are updated.Which issue(s) this PR is related to:
Fixes #398
Special notes for your reviewer:
lane_not_served(throughcontrolsWithNoPlatformLane), which keeps the no-extended-rights contract. A platform follow-up issue will serve the lane.tagsTruncated, which can hide a finding but never invent one.[…]patterns ("Invalid target patterns"), while workflow filters accept them.make lintreports 7 govetreflect.Ptrwarnings in files this PR does not touch (configuration/schema.go,control/catalog.go).Does this PR introduce a user-facing change?
How was this tested?
Unit tests:
make build,go test ./...and gofmt are clean; lint as noted above.plumber analyzewith the local binary:GitHub, Exypte/tag-protection-lab (public lab repo):
v1.0.0, no rulesetrelease-*!entries ignoredon: releasev*criticalrequireCreationRestricted: false~ALLcreation rulesetv[0-9]+.[0-9]+.[0-9]+,v*rulesetv*ruleset disabledGitHub, Exypte/garbage (private, Free plan): not evaluated (rulesets API 403).
GitLab, thibaultcondemine/garbage:
v1.0.0$CI_COMMIT_TAGrelease job$CI_COMMIT_TAG_MESSAGEonlyv*protected for Maintainersv*protected for Developersv2/hotfixA real GitHub Actions run confirmed that
GITHUB_TOKENlists tag rulesets and tags both withcontents: readand withpermissions: {}.AI usage disclosure:
YES. Implemented with Claude Code (design, code, tests and e2e runs), reviewed by the author, who is responsible for the submitted changes (CONTRIBUTING.md, AI_POLICY.md).
🤖 Generated with Claude Code