test: cover the organization PR baseline gate detector - #10
Conversation
The detector had no committed tests. It was verified once by hand against the organization's workflows, which proves nothing about the next edit. Add 29 tests that extract the audit script out of the workflow YAML and execute it, rather than re-implementing the logic. The code under test is therefore the code that ships, and a change to the workflow changes what the tests exercise. Coverage is split by consequence. Fake gates that must be blocked: inline and block-scalar echo, a set -e prefix, any other phrase, exit 0, true, chained inert commands, and a checkout step followed by an echo — the shape actually present in meditation-service and healify-org. Real gates that must pass, which matter more because a false positive here red-lines a compliant repository: compound commands after an echo, command words that merely start with an inert word, semicolon and pipe separation, an exit 1 guard, aggregation through needs.<job>.result, and a gate sharing a file with an unrelated job that mentions the phrase. Advisory behaviour is pinned in both directions: it must annotate without failing by default and must fail under strict, each advisory class must be detected, and SHA-pinned and local actions must not be reported as mutable. Robustness covers unparsable YAML, an empty workflow directory, the step summary, and a clean stderr. Run them in CI through a separate workflow. They cannot live in the reusable baseline, which executes against the caller's checkout and so cannot see this repository's test files.
|
Warning Review limit reached
Next review available in: 55 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded tests for the inline baseline audit script and a GitHub Actions workflow that runs them on relevant pull requests and pushes to ChangesBaseline audit validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant BaselineSelftestWorkflow
participant BaselineAuditTests
participant AuditScript
GitHubActions->>BaselineSelftestWorkflow: trigger on pull request or main push
BaselineSelftestWorkflow->>BaselineAuditTests: execute test_baseline_audit.py
BaselineAuditTests->>AuditScript: extract and execute audit step
AuditScript-->>BaselineAuditTests: return audit results
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: 3
🤖 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 @.github/tests/test_baseline_audit.py:
- Around line 69-75: Update the subprocess.run call in the baseline audit test
to include a short timeout for the extracted audit script, ensuring hung
detector regressions fail promptly while preserving the existing capture and
text settings.
- Around line 64-72: Update the subprocess environment construction in the
baseline audit test around audit_source() so it contains only the required
STRICT and GITHUB_STEP_SUMMARY values, removing the os.environ spread; keep the
existing strict-mode conversion and summary path unchanged.
In @.github/workflows/baseline-selftest.yml:
- Around line 16-20: Update the push.paths filter for the Baseline Self-Test
workflow to include .github/workflows/baseline-selftest.yml, matching the
existing pull_request path filter so changes to this workflow on main trigger
the workflow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c7c41c05-cca2-4212-9399-7a0e5d60a032
📒 Files selected for processing (2)
.github/tests/test_baseline_audit.py.github/workflows/baseline-selftest.yml
Three fixes from review. The harness copied the full test-runner environment into a script that is read out of a workflow file, and a pull request can change that file. Pass only the two variables the script reads, plus PATH, so a modified audit step has nothing inherited to disclose. All 29 tests pass under the restricted environment, which confirms nothing else was needed. Bound the subprocess with a 60 second timeout so a detector regression that hangs fails the test pointing at the audit, rather than stalling until the job timeout. Align the push path filter with the pull_request one: a push to main touching only baseline-selftest.yml would previously have skipped the self-test.
Keeping PATH was a half-measure. The script imports only stdlib and yaml and never shells out, so it needs no inherited value at all. It now receives exactly the two variables it reads. All 29 tests pass, which confirms it. Drop the now-unused os import.
Viktor Didkovskyi (viktor958)
left a comment
Viktor Didkovskyi (viktor958)
left a comment
There was a problem hiding this comment.
Full review at ff0c0a17. This is the right way to test a gate, and I would merge it.
The design decision that makes it worth having: run_audit does not reimplement the detector, it yaml.safe_loads organization-pr-baseline.yml, pulls the run body of the named audit step, and executes that via subprocess against a synthetic .github/workflows tree in a tempdir. So the code under test is provably the code that ships. Tests that paraphrase a workflow's shell script are the usual pattern here and they drift from the artifact within a release or two; this cannot.
The extraction also fails in the right direction — raise AssertionError(f"step {AUDIT_STEP!r} not found in {WORKFLOW}") when the step name no longer matches. Renaming the step breaks the suite loudly instead of leaving it silently exercising nothing, which is exactly the failure mode I just flagged on #9 in this same repo. Worth calling out because the two PRs make opposite choices about what a miss should do, and this one is the choice to keep.
Splitting the assertions by consequence is the other thing I liked: separating cases that must block from cases that must pass matters more than raw count here, because a false positive red-lines a compliant repo and that is the expensive direction of failure for an org-wide baseline. Reading ::error and ::warning off stdout to distinguish blocking from advisory keeps the tiers pinned in both directions rather than just asserting an exit code.
The self-test workflow is wired sensibly: path-filtered so it only runs when the baseline or its tests change, setup-python pinned by SHA rather than a floating tag, pyyaml installed explicitly, and the suite run directly. The tests cannot live inside the reusable baseline itself, since that executes against the caller's checkout and cannot see this repo's files — so a separate workflow is the correct structure, not a workaround.
All checks pass or skip, three review threads are resolved with zero unresolved, and GitHub reports it mergeable and clean at this head. .github main requires no approvals, so nothing gates it.
One follow-up rather than an objection: baseline-selftest.yml is not itself a required check, so a detector regression would go red without blocking a merge. Given the whole point is that the gate keeps working, adding it to the required set once it has a few green runs would close the loop.
Merge it.
1 participant
Summary
Adds test coverage for the gate detector merged in #8. Follow-up to that PR; no change to the baseline workflow itself.
The detector had no committed tests. It was verified once by hand against the organization's 216 workflow files, which proves nothing about the next edit.
Approach
The audit ships as an inline
shell: pythonstep so the reusable workflow carries no dependency on files in this repository. A test that re-implemented that logic would drift from it silently. So these tests extract the script out of the workflow YAML and execute it — the code under test is provably the code that ships, and changing the workflow changes what the tests exercise.Coverage — 29 tests, split by consequence
Fake gates that must be blocked (9): inline and block-scalar echo, a
set -eprefix, any other phrase,exit 0,true, chained inert commands, all-inert multiline, and a checkout step followed by an echo — the shape actually present inmeditation-serviceandhealify-org.Real gates that must pass (9): these matter more, because a false positive in the blocking tier red-lines a compliant repository. Compound commands after an echo (
echo "verifying" && make verify), command words that merely begin with an inert word (truncate_logs), semicolon and pipe separation, anexit 1guard, aggregation throughneeds.<job>.result, a gate sharing a file with an unrelated job that mentions the phrase, and non-gate jobs never being judged.Advisory tier, pinned in both directions (5): must annotate without failing by default, must fail under
strict: true, each advisory class detected, and SHA-pinned and local actions not reported as mutable.Robustness (4): unparsable YAML is blocking, an empty workflow directory is clean, the step summary is written, stderr stays empty.
Self-consistency (2): the extracted step parses as Python, and this repository passes its own blocking tier.
Why a separate workflow
The tests cannot run inside the reusable baseline: that executes against the caller's checkout, so it cannot see this repository's test files.
baseline-selftest.ymlruns them on pull requests touching the baseline, the tests, or itself.Verification
mainis exactly the two new files.Linear: https://linear.app/lifecycle-innovations/issue/HEA-7249
Summary by CodeRabbit
New Features
Tests