Skip to content

Bug/issue 465#970

Open
madkoo wants to merge 9 commits intogithub:main-enterprisefrom
madkoo:bug/issue-465
Open

Bug/issue 465#970
madkoo wants to merge 9 commits intogithub:main-enterprisefrom
madkoo:bug/issue-465

Conversation

@madkoo
Copy link
Copy Markdown

@madkoo madkoo commented Apr 7, 2026

Fixes #465

  • Update schemas to target latest GitHub API

  • feat(schema): add bypass mode for reviewers in suborgs schema

  • Introduced a new "mode" property in the suborgs schema to specify the bypass mode for reviewers, with options "ALWAYS" and "EXEMPT".

fix(build-schema): enhance schema dereferencing with GitHub API spec

  • Updated the build-schema script to fetch and use the GitHub API spec URL from schema files, ensuring all internal $refs are resolved correctly before dereferencing.

test(branches): preserve existing branch protection settings

  • Enhanced tests to ensure that existing branch protection settings, including enforce_admins and restrictions, are preserved when updating branch protection configurations.
  • Added tests to verify normalization of restrictions and defaulting of missing arrays when preserving from GitHub.

…borg levels

- Introduced `repos.json` schema for repository-level safe-settings overrides.
- Updated `settings.json` schema to include additional properties for org-level configurations.
- Created `suborgs.json` schema for suborg-level safe-settings configuration.
- Enhanced the build script to dereference all schemas and handle errors during the process.
Copilot AI review requested due to automatic review settings April 7, 2026 11:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves safe-settings’ config validation and branch protection syncing by introducing JSON schemas (and a dereferencing build step tied to the GitHub REST API spec), updating REST API versioning references, and enhancing branch protection preservation behavior/tests.

Changes:

  • Added new JSON Schemas for org/suborg/repo config files and committed dereferenced schema outputs.
  • Updated script/build-schema to resolve GitHub REST API spec $refs reliably before dereferencing.
  • Enhanced branch protection syncing/tests to preserve existing GitHub settings (e.g., enforce_admins, restrictions) when omitted from config.

Reviewed changes

Copilot reviewed 25 out of 28 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
lib/plugins/branches.js Preserves existing branch protection fields and normalizes restrictions/status checks before updates.
test/unit/lib/plugins/branches.test.js Adds/adjusts tests for preserving enforce_admins/restrictions and normalizing restriction arrays.
script/build-schema Fetches GitHub REST API spec URL from schema sources and pre-dereferences it for stable dereferencing.
schema/settings.json Refactors schema into $defs and updates GitHub REST API spec references/version.
schema/suborgs.json Introduces suborg-level schema referencing GitHub REST API spec shapes.
schema/repos.json Introduces repo-level override schema referencing GitHub REST API spec shapes.
schema/dereferenced/settings.json Updated generated dereferenced org-level schema output.
schema/dereferenced/repos.json Added generated dereferenced repo-level schema output.
lib/settings.js Ensures org-level rulesets are applied first when syncing selected repos.
lib/plugins/custom_properties.js Avoids crashes by filtering null property names during normalization.
lib/plugins/rulesets.js Updates API version header and adjusts override metadata.
test/unit/lib/plugins/rulesets.test.js Updates expected API version header.
lib/plugins/variables.js / lib/plugins/autolinks.js / lib/plugins/collaborators.js Updates REST API doc links to the newer API version.
index.js Updates REST API version headers for rename-related content operations.
docs/** Updates documentation links to the newer API version; sample deployment validator script made safer via optional chaining.
app.yml Updates documentation link for Actions variables permission.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// See: https://docs.github.com/rest/branches/branch-protection#update-branch-protection
const requiredBranchProtectionDefaults = {
required_status_checks: null,
enforce_admins: null,
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

updateBranchProtection requires required_pull_request_reviews to be present in the payload (it can be null), but requiredBranchProtectionDefaults only sets required_status_checks, enforce_admins, and restrictions. When both the config and the existing GitHub protection omit required_pull_request_reviews, the request will be sent without it and can fail validation. Add required_pull_request_reviews: null to requiredBranchProtectionDefaults (and ensure it is included in the 404 branch as well).

Suggested change
enforce_admins: null,
enforce_admins: null,
required_pull_request_reviews: null,

Copilot uses AI. Check for mistakes.
Comment on lines +153 to +155
if (Array.isArray(protection.required_status_checks.contexts) && protection.required_status_checks.contexts.length === 0) {
delete protection.required_status_checks.contexts
}
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

reformatAndReturnBranchProtection deletes required_status_checks.contexts when it is an empty array. The Branch Protection API schema typically requires contexts to be present whenever required_status_checks is non-null, and removing it can cause 422 errors when the repo uses status checks but has no contexts configured. Consider keeping contexts: [] (or only deleting it when checks is present and the API explicitly allows omitting contexts).

Suggested change
if (Array.isArray(protection.required_status_checks.contexts) && protection.required_status_checks.contexts.length === 0) {
delete protection.required_status_checks.contexts
}

Copilot uses AI. Check for mistakes.
await context.octokit.request('GET /repos/{owner}/{repo}/contents/{path}', {
owner: payload.repository.owner.login,
repo: env.ADMIN_REPO,
repo: env.ADMIN_REPO,
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

Trailing whitespace after env.ADMIN_REPO, will fail standard/eslint formatting checks. Remove the extra space.

Suggested change
repo: env.ADMIN_REPO,
repo: env.ADMIN_REPO,

Copilot uses AI. Check for mistakes.
Comment on lines +120 to +127
},
"force_create": {
"description": "Force create the repository even if it already exists",
"type": "boolean"
},
"template": {
"description": "Name of a template repository to use when creating the repository",
"type": "string"
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The force_create description says "Force create the repository even if it already exists", which is misleading: you can’t create a repo that already exists. Align this description with the behavior used elsewhere in the schemas (e.g., "create the repository if it does not exist").

Copilot uses AI. Check for mistakes.
Comment on lines 13 to +20
// Force all names to lowercase to avoid comparison issues.
normalizeEntries () {
this.entries = this.entries.map(({ name, value }) => ({
name: name.toLowerCase(),
value
}))
this.entries = this.entries
.filter(({ name }) => name != null)
.map(({ name, value }) => ({
name: name.toLowerCase(),
value
}))
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

normalizeEntries() now silently drops custom property entries with name == null. This avoids a crash but can hide invalid config and make debugging harder. Consider logging/stashing a validation error when filtering out invalid entries so users know their input was ignored.

Copilot uses AI. Check for mistakes.
Comment on lines 41 to +48
// Force all names to lowercase to avoid comparison issues.
normalize (properties) {
return properties.map(({ property_name: propertyName, value }) => ({
name: propertyName.toLowerCase(),
value
}))
return properties
.filter(({ property_name: propertyName }) => propertyName != null)
.map(({ property_name: propertyName, value }) => ({
name: propertyName.toLowerCase(),
value
}))
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

normalize() now silently drops properties with property_name == null. This prevents a crash but can hide malformed GitHub responses/config data. Consider emitting a debug/error log (or error stash entry) when such entries are encountered so the behavior is observable.

Copilot uses AI. Check for mistakes.
Comment on lines 6 to +12
"properties": {
"repositories": {
"$ref": "#/$defs/RepositorySettings"
},
"labels": {
"$ref": "#/$defs/LabelSettings"
},
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

Schema uses top-level key repositories, but the actual safe-settings YAML (and Settings.updateRepos) uses repository (singular) for repo settings (see docs/sample-settings/settings.yml and lib/settings.js usage of this.config.repository). As-is, this schema won’t validate real configs. Update the schema to define repository (and optionally alias/accept repositories if needed for backwards compatibility).

Copilot uses AI. Check for mistakes.
Comment on lines +6 to +12
"properties": {
"repositories": {
"$ref": "#/$defs/RepositorySettings"
},
"labels": {
"$ref": "#/$defs/LabelSettings"
},
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

Schema uses top-level key repositories, but safe-settings configs use repository (singular) for repo settings (see docs/sample-settings/repo.yml and lib/settings.js uses this.config.repository). This will prevent schema validation from working for repo-level override files. Define repository here (and optionally allow repositories as an alias if required).

Copilot uses AI. Check for mistakes.
Comment on lines +39 to +46
}
},
"repositories": {
"$ref": "#/$defs/RepositorySettings"
},
"labels": {
"$ref": "#/$defs/LabelSettings"
},
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

Schema uses top-level key repositories, but safe-settings configs use repository (singular) for repo settings (see docs/sample-settings/settings.yml and lib/settings.js uses this.config.repository). This mismatch will cause valid suborg configs to fail validation. Define repository here (and optionally allow repositories as an alias if required).

Copilot uses AI. Check for mistakes.
>[!TIP]
>GitHub's API documentation defines these inputs and types:
>1. [Update a repository variable](https://docs.github.com/en/rest/actions/variables?apiVersion=2022-11-28#update-a-repository-variable)
>1. [Update a repository variable](https://docs.github.com/en/rest/actions/variables?apiVersion=2026-03-10#update-a-repository-variable)
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The Branch Protection docs TIP links to the Actions Variables endpoint ("Update a repository variable") instead of the Branch Protection endpoint. Update the link text + URL to point at the branch protection REST API docs for updating branch protection.

Suggested change
>1. [Update a repository variable](https://docs.github.com/en/rest/actions/variables?apiVersion=2026-03-10#update-a-repository-variable)
>1. [Update branch protection](https://docs.github.com/en/rest/branches/branch-protection?apiVersion=2026-03-10#update-branch-protection)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Schema for validation of repos and suborgs

2 participants