Skip to content

Yadhav/fix recent issues - #990

Open
decyjphr wants to merge 93 commits into
main-enterprisefrom
yadhav/fix-recent-issues
Open

Yadhav/fix recent issues#990
decyjphr wants to merge 93 commits into
main-enterprisefrom
yadhav/fix-recent-issues

Conversation

@decyjphr

@decyjphr decyjphr commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Background

Starting with the version 2.1.18 that seemed to be most stable, I've been testing and fixing minor bugs and adding a few critical features and enhancements:

This pull request introduces several major improvements and features to safe-settings, including enhanced plugin control, suborg re-evaluation logic, expanded documentation, and updated permissions for custom roles. The most important changes are grouped and summarized below.


Plugin Control Enhancements

  • Disabling plugins with disable_plugins: Adds support for disabling safe-settings plugins at any config layer (deployment, org, suborg, repo) using a new disable_plugins key. Includes a detailed strip matrix, cascade rules, and limitations. Documentation and sample settings files have been updated with usage examples. [1]], [2]], [3]], [4]], [5]])
  • Additive plugins with additive_plugins: Introduces the additive_plugins key at the org level, allowing selected Diffable plugins to only add or update entries, never remove them. This enables merging external changes with policy. Documentation and samples are provided. [1]], [2]])

Suborg Re-evaluation Logic

  • Automatic suborg re-evaluation after repo-level changes: When a repo-level change might affect suborg membership (e.g., teams, properties, name), safe-settings now re-evaluates suborgs and re-applies settings if a new suborg matches. Includes loop prevention and performance optimizations. ([README.mdR181-R201])

Permissions and Integration Updates

  • Custom roles permissions for GitHub App: Updates app.yml to request the necessary permissions for managing custom organization and repository roles, supporting new features in GitHub Enterprise Cloud. ([app.ymlR116-R123])
  • Webhook event deduplication and improved sync logic: Refactors the main sync functions in index.js to deduplicate repo/suborg changes and streamline sync operations for selected repos and suborgs. [1]], [2]], [3]])

Documentation Improvements

  • Smoke test documentation: Adds comprehensive instructions and explanations for running the end-to-end smoke test, including prerequisites, configuration, usage, and test phases. ([README.mdR717-R811])
  • External group linking for teams: Documents the new external_group property for teams, describing how to link GitHub teams to external IdP groups via API. ([docs/github-settings/4. teams.mdR51-R63])

Other

  • Minor cleanup in app.yml for formatting. ([app.ymlL28])

These changes significantly improve the flexibility, safety, and observability of safe-settings, especially for large organizations with complex policies.

decyjphr and others added 20 commits October 3, 2025 17:03
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Introduced a new "disable_plugins" property in the settings schema to allow disabling specific plugins at various configuration layers.
- Each entry can be a plugin name or an object specifying the plugin and its target layer (self, children, all).
- Updated smoke-test.js to include interactive mode for manual validation during test phases.
- Implemented new test cases for the disable_plugins feature, covering normalization, strip map computation, and integration with updateOrg and updateRepos functions.
- Added tests to ensure proper handling of valid and invalid disable_plugins configurations.
…nds survive

Without action.msg in the dedup key, multiple disable_plugins NopCommands
for the same repo (e.g. skipping 'labels' AND 'teams') all share the same
type+repo+plugin+endpoint key and only the first one survives, silently
dropping the rest from the PR comment and check-run output.

Adding action.msg to the key ensures each unique informational message
is retained while still deduplicating exact duplicates.

Also adds test 27 to cover this case.
- Introduced `additive_plugins` configuration to allow specific Diffable plugins to run in additive mode, preserving existing entries on GitHub.
- Updated `normalizeAdditivePlugins` method to validate and return a set of valid plugin names for additive mode.
- Modified `childPluginsList` to include section names for better tracking of additive flags.
- Enhanced existing tests to cover new functionality, ensuring proper behavior of plugins in additive mode.
- Added integration tests to verify that plugins behave correctly when configured with additive_plugins.
- Created a new environment file for webhook proxy configuration.
- Removed unnecessary comments and streamlined the constructor to enforce uppercase variable names.
- Simplified the `find` method to directly return the required variable data.
- Updated the `changed` method to directly compare values without additional sorting logic.
- Refactored `update`, `add`, and `remove` methods to return NopCommand instances when `nop` is true, preventing actual API calls.
- Enhanced unit tests to cover new NopCommand behavior and ensure proper functionality of the Variables plugin.
- Introduced phase 13 in smoke tests to validate variable creation, updating, and removal in repository settings.
- Added support for phase filtering in smoke tests to allow targeted execution of specific phases.
Generate safe-settings YAML from existing GitHub configuration for a repo,
org, or custom-property-based suborg.

- lib/settingsGenerator.js: extraction engine reusing each plugin's find()
  to read current state and produce config/YAML, with cross-repo
  intersection for suborg generation.
- generate-settings.js: standalone CLI that writes generated YAML to the
  local filesystem (.sample.yml unless --overwrite); loads .env manually.
- index.js + app.yml: repository_dispatch (safe-settings-generate) handler
  that always opens a PR against the admin repo (never commits to the
  default branch directly).
- Suborg files are named suborgs/<name>_<value>.yml.
- README: document generator usage and the PR-only guarantee.
- Unit tests for the generator (25 tests).
decyjphr and others added 9 commits June 16, 2026 23:40
- Added support for custom repository roles in smoke-test.js, including creation, deletion, and retrieval functions.
- Implemented new ruleset management functions for organizations and repositories.
- Updated smoke tests to validate the behavior of custom repository roles and rulesets under various scenarios.
- Enhanced existing tests to ensure proper handling of additive and disabled plugins for custom repository roles and rulesets.
- Introduced new test cases to cover scenarios where suborg configurations change and their impact on repository rulesets.
- Improved error handling and logging for better traceability during tests.
When a suborg.yml file changes its targeting rules (suborgrepos,
suborgteams, or suborgproperties), repos that no longer match the
updated targeting were not having their suborg-applied settings
(e.g. rulesets) removed. This happened because getSubOrgConfigs()
only resolves the new targeting, and repos not in the new targeting
were skipped in updateRepos().

Fix: Load the previous version of changed suborg config files from
the base ref (payload.before for push events, pull_request.base.ref
for PR/NOP mode), resolve which repos were previously targeted,
compare with current targeting, and process removed repos so
diffable's sync() detects and removes orphaned rulesets.

Changes:
- index.js: Pass payload.after/payload.before as ref/baseRef to
  syncSelectedSettings in push handler
- lib/settings.js: Add getReposRemovedFromSubOrgTargeting() method
  that compares old vs new targeting to find removed repos
- lib/settings.js: Add loadYamlFromRef() helper to load config
  from a specific git ref without cache interference
- lib/settings.js: Update syncSelectedRepos to accept baseRef,
  identify removed repos, and process them before the suborg loop
- test/unit/lib/settings.test.js: Add tests for targeting removal

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a sub-test to phase 5 that narrows suborg targeting from
suborgteams to suborgrepos (excluding demo-repo-service1), then
verifies the suborg ruleset is removed from the dropped repo while
retained on the still-targeted repo. Restores team-targeted config
afterward for subsequent phases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The plugin was updated to use github.rest.repos.* but the test was
still mocking github.repos.*, causing TypeError failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
decyjphr and others added 2 commits July 10, 2026 11:29
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fix external group handling, mergeDeep crash, and config validator gap
decyjphr and others added 22 commits July 27, 2026 14:09
Backport the applicable dependency bumps from PR #1001 onto
yadhav/fix-recent-issues. Only js-yaml (^4.1.0 -> 4.2.0, direct prod
dep, includes DoS fix) and shell-quote (^1.6.1 via npm-run-all -> 1.8.4)
are applied; qs cannot be bumped to 6.15.2 on this branch because
express 4.21.2 / body-parser 1.20.3 pin qs to exactly 6.13.0.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1d1ba9d4-e6ec-44ae-8432-3eee74d26b82
…-npm-yarn-group-deps

build(deps): bump js-yaml to 4.2.0 and shell-quote to 1.8.4
Backport of PR #1009 (github-community-projects/safe-settings) onto the
yadhav/fix-recent-issues line.

Team entries are filtered by the same Diffable include/exclude logic that
collaborators use, but the team schema never declared these keys, so editors
and linters couldn't validate them. Mirror the collaborators allOf pattern to
declare include/exclude on the teams items, rebuild the dereferenced schema,
document both in the teams guide, add a sample, and cover the filter path with
unit tests. No runtime changes -- filtering already works via
Diffable.filterEntries.

Adapted to this branch: schemas are consolidated in schema/settings.json (no
separate repos/suborgs schema files), teams is defined inline, and team tests
mock github.teams/github.repos (not github.rest.*).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5dac2ffd-8dc2-45b4-89c9-789f103d2be8
Exercises the backported team include/exclude schema end-to-end: creates a
repo via a repo-level config whose team entries carry include/exclude globs and
asserts safe-settings applies only the team whose include glob matches the repo
(and skips the excluded / non-matching teams). Self-contained so it runs alone
via `--phase 18`; teardown cleans up the repo and teams.

Verified live against org decyjphr-emu: 7/7 assertions passed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5dac2ffd-8dc2-45b4-89c9-789f103d2be8
The teams settings schema referenced the "Create a team" POST requestBody,
whose `permission` enum is limited to pull/push. safe-settings actually
grants repo access via the "Add or update team repository permissions" PUT
endpoint, which supports triage/maintain/admin and custom repository roles.
As a result, valid team entries (e.g. `permission: maintain`, matching the
docs examples) were incorrectly rejected by schema validation.

Switch teams.items to reference the team-repo-permissions PUT schema (where
`permission` is an unconstrained string) and declare the plugin's supported
keys locally: name (required), privacy, external_group, include, exclude.

Also strengthen smoke Phase 18 to grant the included team `maintain`
permission, proving a value the old schema rejected now validates and applies.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5dac2ffd-8dc2-45b4-89c9-789f103d2be8
The previous descriptions were vague and did not mention that values are
glob patterns matched against the repository name (as documented and as
implemented by Diffable.filterEntries() via minimatch).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5dac2ffd-8dc2-45b4-89c9-789f103d2be8
…cent-issues-team-include-exclude

Add include/exclude repo filters to team settings schema (backport #1009)
Bring PR #1010 (bug/issue-903) into this branch. The comprehensive teams.js
here still used the deprecated GET /orgs/{org}/security-managers endpoint, so
the security-manager modernization was not yet incorporated.

- teams.js: identify security manager teams via the organization roles API
  (GET /orgs/{org}/organization-roles and .../{role_id}/teams) instead of the
  deprecated security-managers endpoint. Adapted to this branch's non-`rest`
  Octokit client convention (this.github.repos/teams.*).
- Add team name/slug and role name normalization helpers so configured names
  match existing slugs without add/remove churn.
- Add skipTeamDeletion guard: if security manager discovery fails, keep
  repository teams unchanged instead of deleting them.
- app.yml already grants organization_custom_roles (write), so no permission
  change needed; document the org "Custom organization roles" permission in
  docs/deploy.md.
- Port unit + integration test coverage for the new behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e
checkSecurityManager() only filtered security manager teams out of the
existing repo team list, which suppressed deletes/updates when they were
absent from config. But a config entry naming a security manager team then
looked "missing" to Diffable.sync(), so add()/addOrUpdateRepoPermissionsInOrg
still fired — letting this plugin modify security manager teams, contrary to
the "should not be handled here" intent.

Persist the discovered security manager team identifiers on the instance and
no-op add(), update(), and remove() when the configured team matches them. In
nop mode an INFO command is emitted so PR reviewers see the skip.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e
…ncorporate-pr-1010-security-manager

# Conflicts:
#	test/unit/lib/plugins/teams.test.js
The mock returned a bare string, but production Octokit's request.endpoint()
returns an object with url/body. NopCommand reads endpoint.url and
endpoint.body, so the string mock silently produced undefined values and
reduced test fidelity. Return { url, body } to match the real shape.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e
…overy failure

When security-manager discovery fails, remove() sets skipTeamDeletion and
returned a bare Promise.resolve() even in nop mode. Diffable.sync() pushed
that undefined into the nop command list, hiding the fact that a deletion
was intentionally skipped. Return an INFO NopCommand in nop mode so the
dry-run output is accurate and no undefined entries accumulate.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e
…r-1010-security-manager

Modernize security manager team handling (incorporate PR #1010)
…ation appId harness

Incorporate PR #1017 (absent from this
branch), adapted to this branch's `this.github.repos` convention:

- Branch-protection diff message read `params.branch.name` (always undefined,
  since `params.branch` is already the branch string) -> use `params.branch`,
  and JSON.stringify the results in the debug log.
- NOP update path (protection already exists) was mislabeled 'Add Branch
  Protection' -> 'Update Branch Protection' (debug 'Updating'); the 404/add
  path keeps its 'Add' label.
- Add NOP-mode unit tests asserting the update label when protection exists,
  the add label on 404, and that the diff message names the real branch.

Integration harness: `createProbot` in probot 13 only reads
overrides/defaults/env, so the old `{ id, cert, githubToken }` args were
dropped, making @octokit/auth-app throw "appId option is required". Pass dummy
credentials via `overrides` (token auth) and stub the startup `/app/installations`
call so the app loads under nock.disableNetConnect().

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c735bbe7-feb9-472f-827c-d56ddfe7fe6a
…are-pr-1017

fix(branches): incorporate PR #1017 log/label fixes + mitigate integration appId harness
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A ruleset POST is not idempotent. When Octokit retries a create that already succeeded, or a repo is processed by two overlapping syncs (full sync racing with repository.created), the second create fails with 422 'Name must be unique'. Reconcile by looking the existing ruleset up by name and updating it in place.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…rporate-pr-1018-nop-results

fix: full-sync NOP results without check run + idempotent ruleset create
…rate PR #1023) (#1048)

Diffable.sync() now calls an optional resolveOverrides(existingRecords,
filteredEntries) hook after find() and before the comparison. The rulesets
plugin implements it: each config entry is matched to its live record (via
comparator) and passed through Overrides.removeOverrides, so
{{EXTERNALLY_DEFINED}} placeholders resolve to the live values before
changed() ever sees them. This mirrors how branches.js resolves overrides
inside compareDeep.

Behavior change (intentional, fixes #1022): a ruleset whose only difference
from GitHub is the placeholder no longer reports "Update Ruleset" in dry runs
and no longer issues a redundant PUT in apply mode. Rulesets with real
differences behave exactly as before. Entries are cloned (structuredClone)
before resolution since removeOverrides mutates its input.

Tests updated to the new behavior and nop-mode coverage added.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@decyjphr
decyjphr marked this pull request as ready for review August 10, 2026 11:37
Copilot AI lite review requested due to automatic review settings August 10, 2026 11:37

Copilot AI left a comment

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.

Pull request overview

This PR significantly expands safe-settings capabilities by adding enterprise app-installation management, a “reverse” settings generator, and multiple plugin-behavior enhancements (disable/additive plugins, teams external group linking, ruleset name→id aliasing), backed by broad unit/integration test additions and schema/docs updates.

Changes:

  • Add new app-installations management flow (enterprise-auth client, delta/full reconciliation, reporting subject support).
  • Add settings generation tooling (SettingsGenerator, CLI + repository_dispatch handler) and repo-selection utilities.
  • Improve diff/sync behavior (additive mode, ruleset/teams/variables/custom properties updates) and update schema/docs accordingly.
Show a summary per file
File Description
test/unit/lib/settingsGenerator.test.js New unit tests for SettingsGenerator helpers and output paths.
test/unit/lib/repoSelector.test.js New unit tests for resolving repos via names/teams/properties/all.
test/unit/lib/plugins/variables.test.js Expanded Variables plugin test coverage incl. nop behavior.
test/unit/lib/plugins/teams.test.js Expanded Teams plugin tests for security-manager and external_group logic.
test/unit/lib/plugins/custom_properties.test.js Update tests to use github.rest.repos custom properties APIs.
test/unit/lib/plugins/branches.test.js Add nop-mode regression coverage for branch protection messaging.
test/unit/lib/plugins/appInstallations.test.js New tests for app installation delta/full sync + additive/nop behavior.
test/unit/lib/mergeDeep.test.js New regression tests for ruleset/array identity and string-assign bug fix.
test/unit/lib/appOctokitClient.test.js New tests for enterprise org installations client batching/toggles.
test/integration/plugins/teams.test.js Extend integration test to cover security-manager role/team filtering.
test/integration/common.js Fix Probot init for newer API and stub startup installation listing.
script/build-schema Augment dereferenced schema with ruleset name/slug aliases.
schema/settings.json Expand schema: teams include/exclude/external_group, new plugins/keys.
package.json Add smoke-test npm scripts.
package-lock.json Dependency lock updates (e.g., js-yaml, shell-quote).
lib/settingsGenerator.js New SettingsGenerator to emit YAML from live GitHub state.
lib/repoSelector.js New RepoSelector helper for fixed repo selection criteria.
lib/plugins/variables.js Refactor Variables plugin: deterministic comparisons, nop commands.
lib/plugins/teams.js Add external_group linking + updated security-manager handling.
lib/plugins/rulesets.js Add name→id resolution + override placeholder resolution + idempotent create.
lib/plugins/repository.js Add change signals for suborg re-evaluation (hasChanges/renamed/created).
lib/plugins/diffable.js Add additive mode + per-plugin hasChanges signal + override resolution hook.
lib/plugins/custom_repository_roles.js New plugin to manage custom repository roles at org level.
lib/plugins/custom_properties.js Normalize entries robustly and switch to github.rest.repos APIs.
lib/plugins/branches.js Fix nop messaging/logging for branch protection updates.
lib/plugins/appInstallations.js New org-scoped plugin to manage GitHub App installation repo access.
lib/nopcommand.js Extend NopCommand with subject/subjectType overload for reporting.
lib/mergeDeep.js Improve identity/diffing: stable stringify fallback, bypass actor id quirks, parameters deletions.
lib/configManager.js Stop swallowing getContent errors; rely on outer error handling.
lib/commentmessage.js Replace legacy per-repo table with richer summarized check-run output.
lib/appOctokitClient.js New enterprise-level Octokit wrapper for org installation endpoints.
index.js Major sync refactor: enterprise context enrichment, selected sync dedupe, PR file detection changes, generator PR flow, installation_target handler.
generate-settings.js New standalone CLI to generate YAML to local filesystem.
docs/sample-settings/suborg.yml Document disable_plugins at suborg layer.
docs/sample-settings/settings.yml Document disable_plugins/additive_plugins + ruleset name/slug aliases.
docs/sample-settings/sample-deployment-settings.yml Document deployment-layer disable_plugins.
docs/README.md Link to root README disable_plugins documentation.
docs/github-settings/4. teams.md Document external_group/include/exclude semantics for teams.
docs/deploy.md Note custom org roles permission requirement.
docs/adr/README.md New ADR index.
docs/adr/0001-app-installation-plugin.md New ADR describing app_installations design and rationale.
app.yml Add repository_dispatch event + permissions for new features/APIs.
.gitignore Ignore all *.env files.

Review details

Tip

Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (1)

lib/plugins/teams.js:358

  • The idempotency GET for current external-group links also uses team_slug: attrs.name, which will not match when config uses a team display name. This can cause the PATCH to run every time (or fail) even when the team is already linked.
      const current = await this.github.request(`GET ${teamExternalGroupsEndpoint}`, {
        org: this.repo.owner,
        team_slug: attrs.name
      })
  • Files reviewed: 46/50 changed files
  • Comments generated: 5
  • Review effort level: Lite

Comment thread lib/nopcommand.js
Comment on lines 20 to 21
this.endpoint = endpoint ? endpoint.url : ''
this.body = endpoint ? endpoint.body : ''
Comment thread lib/plugins/diffable.js
const compare = mergeDeep.compareDeep(existingRecords, filteredEntries)
const results = { msg: 'Changes found', additions: compare.additions, modifications: compare.modifications, deletions: compare.deletions }
this.log.debug(`Results of comparing ${this.constructor.name} diffable target ${JSON.stringify(existingRecords)} with source ${JSON.stringify(filteredEntries)} is ${JSON.stringify(results)}`)
this.hasChanges = !!compare.hasChanges
Comment thread lib/plugins/teams.js
Comment on lines +335 to +339
const linkParams = {
org: this.repo.owner,
team_slug: attrs.name,
group_id: groupId
}
Comment thread index.js
Comment on lines +799 to +800
const changes = await context.octokit.pulls.listFiles(params)
const files = changes.data.map(f => { return f.filename })
Comment thread lib/plugins/rulesets.js
Comment on lines +248 to +250
const existing = existingRecords.find(record => this.comparator(record, attrs))
return Overrides.removeOverrides(overrides, structuredClone(attrs), existing || {})
})
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.

Feature proposal: Generate safe-settings YAML from existing GitHub configuration (reverse sync)

3 participants