ci: add pull request validation gate - #457
Open
anatolinicolae wants to merge 6 commits into
Open
anatolinicolae wants to merge 6 commits into
anatolinicolae wants to merge 6 commits into
Conversation
added 3 commits
September 25, 2026 23:08
Found by the new PR validation gate; all four are latent rather than currently-failing problems. community.docker (34 call sites) and community.general (3) are used by the role tree but appeared in no requirements.yml. They resolved only as a transitive dependency of geerlingguy.docker, which declares no dependencies of its own, so a clean runner would have failed ansible-lint's syntax check. Both are now pinned explicitly in every host's requirements.yml. mx1's Nginx Configuration task set `become: true` twice in the same mapping. YAML keeps the last occurrence, so the block ran with no privilege escalation at all. The block-level key is now the single source of truth. The CrowdSec config.yaml template set no file mode, leaving permissions to the umask. Pinned to 0644, matching the role's own keyring. Commands that genuinely change state now say so. `changed_when: true` is already the default for command/shell, so these are declarations of existing behaviour rather than changes, and they clear the no-changed-when rule without lying about idempotency. The apt role's `== true` comparisons are dropped; stat.exists is already a boolean.
Required for the validation gate to be green, and mostly invisible in
review: yes/no to true/false, a document start on every file, trailing
newlines, and 151 fqcn fixes bringing the last unqualified actions in line
with the rest of the tree.
Every rewritten file was re-parsed and compared against HEAD to confirm the
parsed structure was unchanged; long lines were folded into YAML folded
scalars and verified to resolve byte-identical. Two bugs in that conversion
were caught this way and reverted: a regex that rewrote the interior of a
{{ }} Jinja expression, and a pass that touched 122 files instead of the 88
that actually needed it.
The one mixed-style file (mx1 system/config) was left alone apart from the
block-level `become` fix in the previous commit; indenting sequences the way
yamllint defaults to would have reformatted every task file in the repo.
Every workflow until now was deploy-to-production, so a malformed playbook was first discovered by running it against a live host. Adds static validation on pull_request, plus a local runner so the same failures surface before pushing. Seven checks, all green at this commit: - workflow secret guard, asserting validate.yml declares no credentials - compose policy, enforcing the container contract from CLAUDE.md - yamllint, ansible-lint, tofu validate/fmt, zizmor The compose check is the point of the exercise. The cap_drop and no-new-privileges rules were prose in CLAUDE.md, reviewable but not enforced. Services that genuinely cannot satisfy them (data stores needing CAP_SETUID for gosu, the authentik worker needing the Docker socket) are now listed explicitly with a written reason, keyed by (compose path, service) because service names are ambiguous across stacks. An exempted service that also sets cap_drop fails, so a stale exemption cannot linger. validate.yml runs on pull_request and so executes code from the pull request. It is therefore configured to hold no credentials at all, so its behaviour does not depend on who opened the PR. ci/check_workflow_secrets.sh re-checks that on every run, and zizmor audits the workflow independently. Both checkers were verified to fail on the conditions they claim to catch rather than passing vacuously. The tofu job uses -backend=false so it needs no OCI credentials. Known scope limit: the eight deploy workflows still produce 42 zizmor findings (mostly secrets interpolation and missing persist-credentials). They legitimately hold credentials and are never triggered by pull_request, so auditing them is a separate hardening pass. Noted in the workflow.
|
Important Review skippedToo many files! This PR contains 129 files, which is 29 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (129)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
added 3 commits
September 25, 2026 23:20
Neither survived scrutiny, and keeping them was worse than not having them. check_workflow_secrets.sh asserted that validate.yml declares no credentials. It ran as a job in the same workflow it was checking, on the pull request's own copy of itself — so a pull request could delete the script or the job and pass. main is not a protected branch either, so no check is required. It was documentation wearing a test's clothes. Testing also showed it was largely redundant with the zizmor job beside it: of its seven rules, zizmor independently catches secrets references, pull_request_target, workflow_run and write permissions. Its one unique rule was the environment: key, which zizmor passes. And it had a real bug — the persist-credentials check was file-global rather than per-step, so dropping the setting from any one of five checkouts still passed, which is the exact case the rule exists for. check_compose.py enforced the container contract from CLAUDE.md. That contract is prose, and expressing it in code created a second copy to keep in sync. The script was also the only thing forcing a hand-maintained exemption list into the repo, keyed by (compose path, service) because service names are ambiguous across stacks. Both were review aids dressed as gates. The underlying rules stay where they were read before: CLAUDE.md, with the reasoning about why they are easy to miss now written next to the rules. Leaves yamllint, ansible-lint, tofu validate/fmt and zizmor, all green.
main is now a protected branch with all four checks required, so the wording that described them as unenforced is stale. Replaced with what a maintainer actually needs to know: the checks block a merge, and retiring one requires removing it from required_status_checks.contexts in the same pull request.
The validate jobs pip-installed each tool in isolation, so ansible-lint brought its own ansible-core. That happened to match the deploys' ansible==14.4.0 (-> ansible-core~=2.21.4), but nothing tied them: bump ansible in requirements.txt and the deploys would move while validation checked a different version. Installing requirements.txt and ansible-lint into one uv environment makes the match structural. If they ever conflict the install fails loudly instead of quietly adding a second copy. Verified: all five packages co-resolve, and ansible-lint reports ansible-core 2.21.4 — the pinned one. Lint tool pins stay out of requirements.txt on purpose; that file is what the deploys install and a production playbook should not carry a linter. Local action referenced as $/.github/actions/... rather than ./. The $/ form is the current syntax and resolves to the exact running commit, so it does not depend on the workspace checkout or a hardcoded version. The tofu job needs no Python and is left alone.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds static validation on
pull_request. Every workflow until now deployed to production, so a malformed playbook was first discovered by running it against a live host.Five checks, all green:
yamllintansible-lint(production)tofu validate/tofu fmtzizmorci/validate.shruns the same set locally.Four latent bugs fixed
community.docker/community.generalused but undeclared in every host'srequirements.yml(34 and 3 call sites). Resolved only via a transitive dependency, so a clean runner would fail syntax-check. Now pinned.becomekey in mx1's Nginx block. YAML kept the last, so the block ran with no privilege escalation.config.yamltemplate set no file mode. Pinned to0644.changed_when: trueis already the default, so these are declarations, not behaviour changes.The remaining ~99 files are mechanical: 574 yamllint violations and 151
fqcnfixes, in a separate commit.Notes
CLAUDE.mdis not enforced — nothing checks a compose file. A new service omittingcap_dropwill pass CI and rely on review.main. Retiring or renaming one needs a matching change torequired_status_checks.contexts, or the merge blocks.