Normalize required check contexts and harden governance scripts#22
Open
chitcommit wants to merge 1 commit intomainfrom
Open
Normalize required check contexts and harden governance scripts#22chitcommit wants to merge 1 commit intomainfrom
chitcommit wants to merge 1 commit intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
📝 WalkthroughWalkthroughUpdates compliance ruleset JSON files to simplify required status check context strings by removing workflow/job prefixes (e.g., "CI / test" → "test"), and refactors utility scripts to improve error handling, webhook event validation, and check discovery/generation logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Normalizes required status-check contexts used in generated org rulesets to align with actual GitHub check names and hardens governance scripts that discover checks, audit org webhooks, and apply repo settings.
Changes:
- Updated ruleset generation to map friendly check names to canonical required-check
contextvalues (with aliases for compatibility) and added stricter env var validation. - Updated check discovery to emit both job-only names and
workflow / jobnames. - Hardened webhook auditing (capability probe + event-set mismatch detection) and improved repo settings patch error handling via per-run temp files.
- Regenerated org ruleset JSON artifacts to use canonical contexts (e.g.,
build,test,compliance,CodeQL,sync,validate).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/generate-org-rulesets.py | Introduces structured candidate check mapping (name/context/aliases) and validates REQUIRED_APPROVING_REVIEW_COUNT; uses canonical contexts in generated rulesets. |
| scripts/discover-org-checks.sh | Emits both job-only and workflow / job inventory entries to support context normalization and backward compatibility. |
| scripts/audit-org-webhooks.sh | Replaces scope-grep with an API-based permission probe; adds explicit webhook event-set mismatch detection and re-evaluation after apply. |
| scripts/apply-repo-settings.sh | Uses a temp file for PATCH stderr capture instead of a fixed /tmp path. |
| compliance/rulesets/orgs/furnished-condos/10-require-ci-build.json | Switches required context from CI / build to build. |
| compliance/rulesets/orgs/chittyos/10-require-compliance-check-compliance.json | Switches required context from Compliance Check / compliance to compliance. |
| compliance/rulesets/orgs/chittyos/10-require-codeql-analyze.json | Switches required context from CodeQL / Analyze to CodeQL. |
| compliance/rulesets/orgs/chittyos/10-require-ci-test.json | Switches required context from CI / test to test. |
| compliance/rulesets/orgs/chittyos/10-require-ci-lint-and-test.json | Switches required context from CI / lint-and-test to lint-and-test. |
| compliance/rulesets/orgs/chittyos/10-require-chittyconnect-sync-sync.json | Switches required context from ChittyConnect Sync / sync to sync. |
| compliance/rulesets/orgs/chittycorp/10-require-compliance-check-compliance.json | Switches required context from Compliance Check / compliance to compliance. |
| compliance/rulesets/orgs/chittycorp/10-require-ci-test.json | Switches required context from CI / test to test. |
| compliance/rulesets/orgs/chittycorp/10-require-ci-test-and-build.json | Switches required context from CI / test-and-build to test-and-build. |
| compliance/rulesets/orgs/chittycorp/10-require-ci-lint-and-format.json | Switches required context from CI / lint-and-format to lint-and-format. |
| compliance/rulesets/orgs/chittycorp/10-require-chittyconnect-sync-sync.json | Switches required context from ChittyConnect Sync / sync to sync. |
| compliance/rulesets/orgs/chittyapps/10-require-chittyos-compliance-check-compliance.json | Switches required context from ChittyOS Compliance Check / compliance to compliance. |
| compliance/rulesets/orgs/chittyapps/10-require-chittyconnect-sync-sync.json | Switches required context from ChittyConnect Sync / sync to sync. |
| compliance/rulesets/orgs/chicagoapps/10-require-pr-validation-validate.json | Switches required context from PR Validation / validate to validate. |
| compliance/rulesets/orgs/chicagoapps/10-require-codeql-analyze.json | Switches required context from CodeQL / Analyze to CodeQL. |
| compliance/rulesets/orgs/chicagoapps/10-require-chittyconnect-sync-sync.json | Switches required context from ChittyConnect Sync / sync to sync. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (4)
scripts/apply-repo-settings.sh (2)
36-46: Nice hardening; optional cleanup trap would make temp-file handling airtight.Current cleanup covers success/failure paths. A
trapin-function would also clean up on unexpected early exits.Optional refactor
local errfile errfile="$(mktemp)" + trap 'rm -f "$errfile"' RETURN if gh api "repos/$org/$repo" --method PATCH --input <(printf '%s' "$payload") >/dev/null 2>"$errfile"; then - rm -f "$errfile" echo "patched $org/$repo" return 0 fi echo "failed $org/$repo" sed -n '1,2p' "$errfile" - rm -f "$errfile" return 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/apply-repo-settings.sh` around lines 36 - 46, Add a trap to guarantee removal of the temporary error file on unexpected exits: after creating errfile with mktemp in the function, register a trap (e.g., on EXIT/ERR) that rm -f "$errfile"; ensure you still rm -f and echo on the normal success/failure branches and clear or reset the trap before returning to avoid duplicate handling; reference the existing local errfile, mktemp and the gh api call so the trap surrounds the call that may exit early.
1-2: Please ensure CODEOWNERS approval is enforced for this setup-script change.This file falls under governance/setup-script risk and should keep strict reviewer gating.
Based on learnings: Require code owner approval for all changes to workflow files and setup scripts (paths:
.github/workflows/and setup scripts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/apply-repo-settings.sh` around lines 1 - 2, This change affects a setup script (apply-repo-settings.sh) and must require CODEOWNERS approval; update the repository's .github/CODEOWNERS to include an entry that covers setup scripts and workflow files (e.g., patterns like /.github/workflows/ and /scripts/* or specifically /apply-repo-settings.sh) and list the appropriate code-owner team or user handle so code-owner review is required, and verify branch protection is configured to require CODEOWNERS approval for PRs.scripts/discover-org-checks.sh (1)
45-51: Potential race condition with shared/tmp/wf.yml.If multiple instances of this script run concurrently (e.g., parallel CI jobs or manual invocations), they will overwrite the same
/tmp/wf.ymlfile, causing unpredictable parsing results.Consider using
mktempfor a unique temporary file:♻️ Proposed fix using mktemp
+ tmpfile=$(mktemp) - if ! (printf "%s" "$content" | base64 --decode > /tmp/wf.yml 2>/dev/null || printf "%s" "$content" | base64 -D > /tmp/wf.yml 2>/dev/null); then + if ! (printf "%s" "$content" | base64 --decode > "$tmpfile" 2>/dev/null || printf "%s" "$content" | base64 -D > "$tmpfile" 2>/dev/null); then + rm -f "$tmpfile" continue fi parsed=$(ruby -ryaml -e ' begin - y = YAML.load_file("/tmp/wf.yml") + y = YAML.load_file("'"$tmpfile"'") rescue exit 0 end # ... rest of Ruby code ' 2>/dev/null || true) + rm -f "$tmpfile"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/discover-org-checks.sh` around lines 45 - 51, The script uses a fixed /tmp/wf.yml when decoding and parsing "$content" which causes a race condition when multiple instances run; change the flow to create a unique temp file with mktemp (or similar) and write the decoded content into that unique path instead of /tmp/wf.yml, then pass that temp path to the Ruby YAML.load_file call (and ensure the temp file is removed after parsing); update the references around the decode block and the parsed=$(ruby -ryaml -e '... YAML.load_file("/tmp/wf.yml")') invocation to use the generated temp filename variable.scripts/audit-org-webhooks.sh (1)
128-141: Duplicated event-sorting logic.The same sorting and comparison logic appears twice (lines 132-140 and 163-171). Consider extracting this into a helper function to reduce duplication and improve maintainability.
♻️ Proposed helper function
+normalize_events() { + tr ',' '\n' <<<"$1" | sed '/^$/d' | sort -u | tr '\n' ',' | sed 's/,$//' +} + +check_events_mismatch() { + local events_joined="$1" + if [ "$events_joined" = "*" ]; then + echo "wildcard-events" + else + local sorted_actual sorted_required + sorted_actual="$(normalize_events "$events_joined")" + sorted_required="$(normalize_events "$WEBHOOK_EVENTS_CSV")" + if [ "$sorted_actual" != "$sorted_required" ]; then + echo "events-mismatch" + fi + fi +}Then replace both occurrences with:
mismatch_reason="$(check_events_mismatch "$events_joined")" [ -n "$mismatch_reason" ] && needs_apply_reasons+=("$mismatch_reason")Also applies to: 157-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audit-org-webhooks.sh` around lines 128 - 141, The duplicated event-sorting and comparison logic should be extracted into a helper function named check_events_mismatch which accepts the events_joined string, compares it against the canonical WEBHOOK_EVENTS_CSV (handling the "*" wildcard), and returns either "wildcard-events", "events-mismatch", or an empty string; replace both inline blocks that operate on events_joined and compare to WEBHOOK_EVENTS_CSV with calls to check_events_mismatch and append its non-empty return value to needs_apply_reasons (use the existing variable names events_joined, WEBHOOK_EVENTS_CSV, and needs_apply_reasons to locate and wire up the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compliance/rulesets/orgs/chittyos/10-require-chittyconnect-sync-sync.json`:
- Line 40: The required-context value "sync" in the JSON ruleset is incorrect;
update the "context" value in the rule (the JSON key "context" inside the
ChittyConnect sync rule) to the full GitHub Actions check name "ChittyConnect
Sync / sync" so the required status check matches the actual check emitted by
the workflow.
In `@compliance/rulesets/orgs/chittyos/10-require-ci-lint-and-test.json`:
- Line 26: The ruleset currently only lists a check object with "context":
"lint-and-test"; add two additional status-check objects for the missing
required checks by inserting objects with "context": "beacon-check" and
"context": "security" into the same checks array (so alongside the existing
"lint-and-test", "sync", and "CodeQL" entries) to restore the required merge
gate coverage.
In `@compliance/rulesets/orgs/chittyos/10-require-codeql-analyze.json`:
- Line 26: The "context" value currently set as "CodeQL" is incorrect; update
the JSON property "context" (the string currently "CodeQL") to match the actual
emitted check-run name "CodeQL / Analyze" so the required check maps to the
workflow's check-run; locate the "context" key in the ruleset (the entry showing
"context": "CodeQL") and change its value to "CodeQL / Analyze".
---
Nitpick comments:
In `@scripts/apply-repo-settings.sh`:
- Around line 36-46: Add a trap to guarantee removal of the temporary error file
on unexpected exits: after creating errfile with mktemp in the function,
register a trap (e.g., on EXIT/ERR) that rm -f "$errfile"; ensure you still rm
-f and echo on the normal success/failure branches and clear or reset the trap
before returning to avoid duplicate handling; reference the existing local
errfile, mktemp and the gh api call so the trap surrounds the call that may exit
early.
- Around line 1-2: This change affects a setup script (apply-repo-settings.sh)
and must require CODEOWNERS approval; update the repository's .github/CODEOWNERS
to include an entry that covers setup scripts and workflow files (e.g., patterns
like /.github/workflows/ and /scripts/* or specifically /apply-repo-settings.sh)
and list the appropriate code-owner team or user handle so code-owner review is
required, and verify branch protection is configured to require CODEOWNERS
approval for PRs.
In `@scripts/audit-org-webhooks.sh`:
- Around line 128-141: The duplicated event-sorting and comparison logic should
be extracted into a helper function named check_events_mismatch which accepts
the events_joined string, compares it against the canonical WEBHOOK_EVENTS_CSV
(handling the "*" wildcard), and returns either "wildcard-events",
"events-mismatch", or an empty string; replace both inline blocks that operate
on events_joined and compare to WEBHOOK_EVENTS_CSV with calls to
check_events_mismatch and append its non-empty return value to
needs_apply_reasons (use the existing variable names events_joined,
WEBHOOK_EVENTS_CSV, and needs_apply_reasons to locate and wire up the change).
In `@scripts/discover-org-checks.sh`:
- Around line 45-51: The script uses a fixed /tmp/wf.yml when decoding and
parsing "$content" which causes a race condition when multiple instances run;
change the flow to create a unique temp file with mktemp (or similar) and write
the decoded content into that unique path instead of /tmp/wf.yml, then pass that
temp path to the Ruby YAML.load_file call (and ensure the temp file is removed
after parsing); update the references around the decode block and the
parsed=$(ruby -ryaml -e '... YAML.load_file("/tmp/wf.yml")') invocation to use
the generated temp filename variable.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f78c0422-d0cf-41c3-afce-6589325d3bfa
📒 Files selected for processing (20)
compliance/rulesets/orgs/chicagoapps/10-require-chittyconnect-sync-sync.jsoncompliance/rulesets/orgs/chicagoapps/10-require-codeql-analyze.jsoncompliance/rulesets/orgs/chicagoapps/10-require-pr-validation-validate.jsoncompliance/rulesets/orgs/chittyapps/10-require-chittyconnect-sync-sync.jsoncompliance/rulesets/orgs/chittyapps/10-require-chittyos-compliance-check-compliance.jsoncompliance/rulesets/orgs/chittycorp/10-require-chittyconnect-sync-sync.jsoncompliance/rulesets/orgs/chittycorp/10-require-ci-lint-and-format.jsoncompliance/rulesets/orgs/chittycorp/10-require-ci-test-and-build.jsoncompliance/rulesets/orgs/chittycorp/10-require-ci-test.jsoncompliance/rulesets/orgs/chittycorp/10-require-compliance-check-compliance.jsoncompliance/rulesets/orgs/chittyos/10-require-chittyconnect-sync-sync.jsoncompliance/rulesets/orgs/chittyos/10-require-ci-lint-and-test.jsoncompliance/rulesets/orgs/chittyos/10-require-ci-test.jsoncompliance/rulesets/orgs/chittyos/10-require-codeql-analyze.jsoncompliance/rulesets/orgs/chittyos/10-require-compliance-check-compliance.jsoncompliance/rulesets/orgs/furnished-condos/10-require-ci-build.jsonscripts/apply-repo-settings.shscripts/audit-org-webhooks.shscripts/discover-org-checks.shscripts/generate-org-rulesets.py
compliance/rulesets/orgs/chittyos/10-require-chittyconnect-sync-sync.json
Show resolved
Hide resolved
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
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.
Summary
sync,lint-and-test,CodeQL, etc.) to prevent branch-policy deadlocksREQUIRED_APPROVING_REVIEW_COUNTin ruleset generationValidation
bash -n scripts/discover-org-checks.sh scripts/audit-org-webhooks.sh scripts/apply-repo-settings.shpython3 -m py_compile scripts/generate-org-rulesets.pySummary by CodeRabbit
Chores
Bug Fixes
New Features