Skip to content

feat: implement issue #580 — Retire/delineate the detection-based apply-rulesets.sh — it diverges from the codified ruleset (#575 debt) #591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
don-petry wants to merge 1 commit into from
Closed

feat: implement issue #580 — Retire/delineate the detection-based apply-rulesets.sh — it diverges from the codified ruleset (#575 debt) #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions .github/workflows/apply-rulesets-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Quality gates for the apply-rulesets script and its unit tests.
#
# Triggered on any PR that touches:
# - scripts/apply-rulesets.sh
# - test/scripts/apply-rulesets/**
#
# Gates (all must pass before merge):
# 1. shellcheck — static analysis for the applier script
# 2. bats — unit tests pinning that the detection-based builder does not
# inject the non-codified `Dev-Lead Agent / dev-lead` context
# (it must agree with standards/rulesets/code-quality.json)

name: Apply Rulesets Tests

on:
pull_request:
paths:
- 'scripts/apply-rulesets.sh'
- 'test/scripts/apply-rulesets/**'
- '.github/workflows/apply-rulesets-tests.yml'
push:
branches:
- main
paths:
- 'scripts/apply-rulesets.sh'
- 'test/scripts/apply-rulesets/**'
- '.github/workflows/apply-rulesets-tests.yml'

permissions:
contents: read

concurrency:
group: apply-rulesets-tests-${{ github.ref }}
cancel-in-progress: true

jobs:
test:
name: Lint and bats
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- name: Checkout repository
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
fetch-depth: 1

- name: Install bats, shellcheck, and jq
run: |
set -euo pipefail
sudo apt-get update -qq
sudo apt-get install -y --no-install-recommends bats shellcheck jq

- name: shellcheck
run: |
set -euo pipefail
shellcheck --severity=warning -x scripts/apply-rulesets.sh

- name: Run bats suite
run: bats --print-output-on-failure test/scripts/apply-rulesets/
38 changes: 35 additions & 3 deletions scripts/apply-rulesets.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,25 @@
#!/usr/bin/env bash
# apply-rulesets.sh — Apply standard repository rulesets to petry-projects repos
#
# ─────────────────────────────────────────────────────────────────────────────
# LEGACY: DETECTION-BASED RULESET BUILDER
#
# This copy *builds* the `code-quality` ruleset in-code by probing each repo's
# workflow files (detect_required_checks). It predates the codified,
# file-driven applier in `.github-private/scripts/apply-rulesets.sh` (#889),
# which reads the checked-in desired-state JSON in `standards/rulesets/*.json`
# and PUTs it verbatim. Those JSON files — not this script — are the canonical
# source of truth for the `pr-quality` / `code-quality` rulesets (see
# standards/rulesets/README.md).
#
# When editing what this script produces, reconcile it against
# `standards/rulesets/code-quality.json`: the two tools must not disagree about
# the required-context set. In particular this builder deliberately does NOT add
# `Dev-Lead Agent / dev-lead` as a required context — the codified file omits it
# on purpose (#579: Dev-Lead Agent is per-PR review, not a merge gate; requiring
# it deadlocks any PR that touches workflow files). See #580.
# ─────────────────────────────────────────────────────────────────────────────
#
# Companion script to compliance-audit.sh. Creates or updates the rulesets defined in:
# standards/github-settings.md#repository-rulesets
#
Expand Down Expand Up @@ -125,9 +144,12 @@ detect_required_checks() {
# the per-ecosystem jobs.
checks+=("dependency-audit / Detect ecosystems")
fi
if echo "$workflows" | grep -qx "dev-lead.yml"; then
checks+=("Dev-Lead Agent / dev-lead")
fi
# NOTE: dev-lead.yml is intentionally NOT mapped to a required check. The
# `Dev-Lead Agent / dev-lead` context is per-PR review, not a merge gate: the
# agent's GitHub App refuses to mint a token for any PR touching workflow
# files, so requiring it would deadlock every workflow-modifying PR. The
# codified standards/rulesets/code-quality.json deliberately omits it (#579),
# and this builder must agree (#580).
# dependabot-automerge / dependabot-rebase are intentionally NOT
# required: dependabot-automerge runs only on dependabot[bot] PRs and
# dependabot-rebase runs only on push to main, neither of which are
Expand Down Expand Up @@ -350,6 +372,11 @@ apply_rulesets() {
fi
}

# ---------------------------------------------------------------------------
# Entry point — guarded so the pure functions above (detect_required_checks,
# build_ruleset_json, …) can be sourced and unit-tested without running main.
# ---------------------------------------------------------------------------
main() {
# ---------------------------------------------------------------------------
# Parse arguments
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -404,3 +431,8 @@ if [ "$TARGET" = "--all" ]; then
else
apply_rulesets "$TARGET"
fi
}

if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
main "$@"
fi
2 changes: 1 addition & 1 deletion standards/github-settings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ the `release-channel-tags` ruleset is managed in `.github-private` (see
#### Required Check Categories

The table below covers the **unconditional fleet-wide required check contexts** — the base set `detect_required_checks()` applies when each org-standard workflow file is present.
The script also conditionally adds **Dev-Lead Agent** (when `dev-lead.yml` exists) and **Secret scan** (when `ci.yml` contains the gitleaks action), but those are in staged rollout; see below.
The script also conditionally adds **Secret scan** (when `ci.yml` contains the gitleaks action), which is in staged rollout; see below. It does **not** add **Dev-Lead Agent** as a required context — that is per-PR review, not a merge gate ([#579](https://github.com/petry-projects/.github/issues/579), [#580](https://github.com/petry-projects/.github/issues/580)); see the table row below.

Comment on lines 274 to 276
| Check | Status | Check Name(s) | Notes |
|-------|--------|---------------|-------|
Expand Down
77 changes: 77 additions & 0 deletions test/scripts/apply-rulesets/dev-lead-not-required.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#!/usr/bin/env bats
# Regression tests pinning the #580 fix: the detection-based
# scripts/apply-rulesets.sh must NOT inject a `Dev-Lead Agent / dev-lead`
# required status check, so its generated `code-quality` set agrees with the
# codified standards/rulesets/code-quality.json (which deliberately omits it,
# per #579 — Dev-Lead Agent is per-PR review, not a merge gate).
#
# These source the script's pure functions directly (guarded main block), so
# they exercise detect_required_checks / build_ruleset_json in isolation.

bats_require_minimum_version 1.5.0

load 'helpers/setup'

setup() {
tt_make_tmpdir
tt_install_gh_stub
# detect_required_checks + build_ruleset_json are defined below the
# BASH_SOURCE guard, so sourcing does not run main.
source "$TT_SCRIPT"
}

teardown() {
tt_cleanup_tmpdir
}

@test "detect_required_checks omits Dev-Lead Agent even when dev-lead.yml is present" {
GH_STUB_WORKFLOWS="agent-shield.yml dependency-audit.yml dev-lead.yml" \
GH_STUB_CODEQL_STATE="configured" \
run detect_required_checks "some-repo"

[ "$status" -eq 0 ]
[[ "$output" != *"Dev-Lead Agent / dev-lead"* ]]
}

@test "detect_required_checks still emits the codified contexts" {
GH_STUB_WORKFLOWS="agent-shield.yml dependency-audit.yml dev-lead.yml" \
GH_STUB_CODEQL_STATE="configured" \
run detect_required_checks "some-repo"

[ "$status" -eq 0 ]
[[ "$output" == *"CodeQL"* ]]
[[ "$output" == *"agent-shield / AgentShield"* ]]
[[ "$output" == *"dependency-audit / Detect ecosystems"* ]]
}

@test "detect_required_checks agrees with the codified code-quality.json context set" {
# The generated set (for a repo carrying the codified workflows) must be a
# superset-free match of the checked-in source of truth: every context the
# detector emits for these workflows is present in code-quality.json, and it
# adds nothing the codified file omits.
GH_STUB_WORKFLOWS="agent-shield.yml dependency-audit.yml dev-lead.yml" \
GH_STUB_CODEQL_STATE="configured" \
run detect_required_checks "some-repo"
[ "$status" -eq 0 ]

codified="$(jq -r '.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context' \
"${TT_REPO_ROOT}/standards/rulesets/code-quality.json")"
Comment on lines +57 to +58

@gemini-code-assist gemini-code-assist Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

When accessing nested properties of an object that may be null or missing in jq filters, use the optional/try operator ? to prevent fatal 'Cannot index null with string' errors.

  codified="$(jq -r '.rules[] | select(.type=="required_status_checks") | .parameters?.required_status_checks[]?.context?' \\
    "${TT_REPO_ROOT}/standards/rulesets/code-quality.json")"
References
  1. In jq filters within shell scripts, when accessing nested properties of an object that may be null (e.g., .author.login), use the optional/try operator ? (e.g., .author.login?) to prevent fatal 'Cannot index null with string' errors.


while IFS= read -r ctx; do
[ -z "$ctx" ] && continue
grep -qxF "$ctx" <<< "$codified" || {
echo "detector emitted non-codified context: '$ctx'" >&2
false
}
done <<< "$output"
Comment on lines +60 to +66

@gemini-code-assist gemini-code-assist Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In Bash, a while loop's exit status is determined by the last executed command in the loop. If a non-codified context is encountered in an earlier iteration, false is executed, but the loop continues to the next iteration. If the last iteration succeeds, the loop exits with status 0, causing the test to silently pass despite the failure.

To fix this, track the failure state using a local variable and assert its value after the loop completes.

  local failed=0
  while IFS= read -r ctx; do
    [ -z "$ctx" ] && continue
    grep -qxF "$ctx" <<< "$codified" || {
      echo "detector emitted non-codified context: '$ctx'" >&2
      failed=1
    }
  done <<< "$output"
  [ "$failed" -eq 0 ]

}

@test "build_ruleset_json injects no contexts beyond those passed" {
run build_ruleset_json "code-quality" "CodeQL" "agent-shield / AgentShield"
[ "$status" -eq 0 ]

contexts="$(echo "$output" | jq -r '.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context')"

@gemini-code-assist gemini-code-assist Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

When accessing nested properties of an object that may be null or missing in jq filters, use the optional/try operator ? to prevent fatal 'Cannot index null with string' errors.

  contexts="$(echo "$output" | jq -r '.rules[] | select(.type=="required_status_checks") | .parameters?.required_status_checks[]?.context?')"
References
  1. In jq filters within shell scripts, when accessing nested properties of an object that may be null (e.g., .author.login), use the optional/try operator ? (e.g., .author.login?) to prevent fatal 'Cannot index null with string' errors.

[[ "$contexts" == *"CodeQL"* ]]
[[ "$contexts" == *"agent-shield / AgentShield"* ]]
[[ "$contexts" != *"Dev-Lead Agent / dev-lead"* ]]
}
38 changes: 38 additions & 0 deletions test/scripts/apply-rulesets/helpers/setup.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/usr/bin/env bash
# Common test helpers for the apply-rulesets bats suite.
set -euo pipefail

TT_REPO_ROOT="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../../../.." && pwd)"
readonly TT_REPO_ROOT
export TT_REPO_ROOT

TT_SCRIPT="${TT_REPO_ROOT}/scripts/apply-rulesets.sh"
readonly TT_SCRIPT
export TT_SCRIPT

TT_STUBS_DIR="${TT_REPO_ROOT}/test/scripts/apply-rulesets/stubs"
readonly TT_STUBS_DIR
export TT_STUBS_DIR

tt_make_tmpdir() {
TT_TMP="$(mktemp -d)"
export TT_TMP
}

tt_cleanup_tmpdir() {
if [[ -n "${TT_TMP:-}" && -d "${TT_TMP}" ]]; then
rm -rf "${TT_TMP}"
fi
}

# Install the fake `gh` binary on PATH for the test.
tt_install_gh_stub() {
local tt_tmp="${TT_TMP:?TT_TMP must be set (call tt_make_tmpdir first)}"
local stubs_dir="${TT_STUBS_DIR:?TT_STUBS_DIR must be set}"
local stub_dir="${tt_tmp}/bin"
mkdir -p "$stub_dir"
cp "${stubs_dir}/gh" "$stub_dir/gh"
chmod +x "$stub_dir/gh"
PATH="${stub_dir}:${PATH}"
export PATH
}
50 changes: 50 additions & 0 deletions test/scripts/apply-rulesets/stubs/gh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#!/usr/bin/env bash
# Fake `gh` binary for the apply-rulesets bats suite.
#
# detect_required_checks() in scripts/apply-rulesets.sh probes a repo via a
# handful of `gh api` calls. This stub answers those endpoints from env vars so
# tests can shape a repo's workflow set without per-fixture stub variants.
#
# Recognized vars:
# GH_STUB_WORKFLOWS — whitespace-separated workflow filenames returned for
# the `.github/workflows` contents listing (default:
# empty → repo has no workflows).
# GH_STUB_CODEQL_STATE — value returned for the code-scanning default-setup
# `.state` probe (default: "configured").
#
# Content fetches for individual workflow files return empty (the script guards
# every content call with `|| echo ""`), which is sufficient for the checks
# these tests exercise (they do not depend on sonarcloud.yml / ci.yml bodies).

set -euo pipefail

# Locate the api path: the first positional argument after the `api` subcommand.
path=""
prev=""
for arg in "$@"; do
if [ "$prev" = "api" ]; then
path="$arg"
break
fi
prev="$arg"
done
Comment on lines +21 to +30

case "$path" in
*/contents/.github/workflows)
# The real call uses `--jq '.[].name'`; emit the already-projected names.
for wf in ${GH_STUB_WORKFLOWS:-}; do
printf '%s\n' "$wf"
done
;;
*/code-scanning/default-setup)
printf '%s' "${GH_STUB_CODEQL_STATE:-configured}"
;;
*/contents/.github/workflows/*)
# Individual workflow file content — not needed for these tests.
printf ''
;;
*)
printf 'gh stub: unhandled api path: %s\n' "$path" >&2
exit 1
;;
esac
Loading