chore(ci): extend argspec drift check with a bare is-defined scanner - #196
Conversation
Codifies the elasticstack_cert_pass class of regression: when a role
default is commented out or explicitly empty, a downstream
'when: X is defined' gate silently changes behaviour the moment
someone uncomments or sets X to the empty string. The scanner walks
roles/*/tasks/*.yml and fails CI when a bare 'X is defined' gate
references a default var whose declared value is empty or null. Vars
with real defaults are left alone (the gate is dead code but not a
regression risk), and register-style checks (X.stdout, X.content) or
lines that already length-guard are excluded.
Two remaining offenders on main are fixed in the same PR:
- beats/tasks/metricbeat.yml: 'when: beats_metricbeat_modules is
defined' + 'loop: {{ beats_metricbeat_modules }}' would crash on the
role's own empty default. Now length-guarded with default([]).
- elasticsearch/tasks/main.yml: the extra_config duplicate-key guard
had a redundant 'is defined' next to a legit length check;
simplified to just the length check.
CLAUDE.md gets a short guideline pointing at the check.
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesNon-empty default handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
roles/beats/tasks/metricbeat.yml (1)
26-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the empty-module regression check to
beats_security. Setbeats_metricbeat: trueandbeats_metricbeat_modules: [], then assert that/etc/metricbeat/pipelines_createdis absent on Beats hosts.beats_advanceduseslogstash, so it cannot exercise the ingest-pipeline condition. Extend the existing scenario so the test fails before this change and passes after it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@roles/beats/tasks/metricbeat.yml` around lines 26 - 27, Add an empty-module regression scenario to the existing beats_security test, configuring beats_metricbeat: true and beats_metricbeat_modules: [] and asserting /etc/metricbeat/pipelines_created is absent on Beats hosts. Extend the current scenario rather than using beats_advanced, since that scenario uses logstash and does not exercise the Metricbeat ingest-pipeline condition.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_argspecs.py`:
- Line 131: Update empty-default tracking in the argument-spec scanning flow
around empty_default_vars so defaults are stored under their owning role name
rather than in one global set. When validating each task file, select only the
empty defaults for that file’s current role, preserving independent behavior
between roles.
- Around line 116-123: Replace the raw line scan around IS_DEFINED_RE with
task-YAML parsing, and inspect only each task’s when expressions. For every
condition branch, associate the guard with the same variable, requiring a
non-empty or register-based guard; do not treat unrelated stdout, content, or
length references as exemptions. Preserve the existing hit reporting while
excluding comments and task names.
---
Nitpick comments:
In `@roles/beats/tasks/metricbeat.yml`:
- Around line 26-27: Add an empty-module regression scenario to the existing
beats_security test, configuring beats_metricbeat: true and
beats_metricbeat_modules: [] and asserting /etc/metricbeat/pipelines_created is
absent on Beats hosts. Extend the current scenario rather than using
beats_advanced, since that scenario uses logstash and does not exercise the
Metricbeat ingest-pipeline condition.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0fa2864-e751-4b0f-800c-2571ec6e6851
📒 Files selected for processing (4)
CLAUDE.mdroles/beats/tasks/metricbeat.ymlroles/elasticsearch/tasks/main.ymlscripts/check_argspecs.py
💤 Files with no reviewable changes (1)
- roles/elasticsearch/tasks/main.yml
Two CodeRabbit findings on the initial scanner: 1. Line-based regex matched comments and task-name strings, and a stray '.stdout' / '| length' / '.content' anywhere on the same line exempted an unrelated 'X is defined'. Rewrite to parse each task file, walk block/rescue/always, extract 'when:' as string or list, and check each condition individually. The pair guard now targets the same variable specifically: 'var | ... | length', 'var | bool', 'var.stdout' / 'var.content', or 'var | default(...) | length' (with any intermediate filter chain). An 'other.stdout' next to 'var is defined' no longer counts as a guard for 'var'. 2. empty_default_vars was one global set across roles. An empty 'foo' in role A therefore falsely flagged 'foo is defined' in role B where 'foo' held a real default. Store empties per role and only apply the same-role set when walking that role's tasks/*.yml. Verified against the real tree (no false positives) and against four synthetic cases: elasticstack_cert_pass regression class caught, cross-role clash not caught, guarded expression allowed, cross-var 'other.stdout' no longer excuses a bare 'var is defined'.
1 participant
Codifies the
elasticstack_cert_passclass of regression: when a role default is commented out or explicitly empty, a downstreamwhen: X is definedgate silently changes behaviour the moment someone uncomments or sets X to the empty string. The scanner walksroles/*/tasks/*.ymland fails CI when a bareX is definedgate references a default var whose declared value is empty or null. Vars with real defaults are left alone (the gate is dead code but not a regression risk), and register-style checks (X.stdout,X.content) or lines that already length-guard are excluded.Two remaining offenders on
mainare fixed in the same PR —beats/tasks/metricbeat.yml(loopoverbeats_metricbeat_moduleswith the same undefined-vs-empty gate that hit us before) and the redundantis definedin the elasticsearch extra_config duplicate-key guard.CLAUDE.mdgets a short guideline pointing contributors at the check.Summary by CodeRabbit
Bug Fixes
Quality Improvements