Yadhav/fix recent issues - #990
Draft
decyjphr wants to merge 88 commits into
Draft
Conversation
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.
…roles permissions
- 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.
…AML configurations
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).
- 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.
…_reviewers drift detection
… object array key reordering
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>
teams.js: - resolveExternalGroupId() now matches external_group names case-insensitively, preventing false "not found" errors caused by casing differences vs. the SCIM-synced Azure AD group name. - syncExternalGroup() no longer treats a missing external group as a fatal error. Newly-created teams whose SCIM group hasn't finished provisioning yet now log a warning and emit a WARNING NopCommand instead of an ERROR, so the PR check surfaces it without failing. settings.js: - handleResults() gains full support for the new WARNING NopCommand type: tracked in stats.warnings, rendered in a dedicated "Warnings" PR comment section, and excluded from the check_run failure condition (only ERROR still flips the conclusion to failure). - childPluginsList(): fixed a validation gap where config entries declared only at suborg/repo level (no matching org-level baseline entry by name) were never passed to configvalidators/ overridevalidators. Only entries matching an org-baseline entry were validated; new entries with no org-level counterpart silently bypassed validation (e.g. an invalid team `permission` value would not be rejected). Now validated against an empty baseConfig too. mergeDeep.js: - Fixed a crash in compareDeepIfVisited(): merging an addition into a modification unconditionally called Object.assign, which threw "Cannot assign to read only property '0' of object '[object String]'" when both values were primitive strings (e.g. the `name` identifying attribute added by addIdentifyingAttribute). This happened whenever an array item had both a modified field (e.g. team `permission`) and a field missing from the target object (e.g. `external_group`). The merge now branches by value type — array: push, object: Object.assign, primitive: direct overwrite. Tests: - teams.test.js updated to assert the WARNING (not ERROR) NopCommand and warning log for the missing-external-group case. - mergeDeep.test.js: added a regression test reproducing the permission-change + new-external_group crash scenario.
…installation-plugin feat: add app installation plugin for managing GitHub App repo access
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fix external group handling, mergeDeep crash, and config validator gap
…community-projects/safe-settings into yadhav/fix-recent-issues
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
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
2 participants
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.
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
disable_plugins: Adds support for disabling safe-settings plugins at any config layer (deployment, org, suborg, repo) using a newdisable_pluginskey. 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: Introduces theadditive_pluginskey 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
safe-settingsnow 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
app.ymlto request the necessary permissions for managing custom organization and repository roles, supporting new features in GitHub Enterprise Cloud. ([app.ymlR116-R123])index.jsto deduplicate repo/suborg changes and streamline sync operations for selected repos and suborgs. [1]], [2]], [3]])Documentation Improvements
external_groupproperty for teams, describing how to link GitHub teams to external IdP groups via API. ([docs/github-settings/4. teams.mdR51-R63])Other
app.ymlfor formatting. ([app.ymlL28])These changes significantly improve the flexibility, safety, and observability of
safe-settings, especially for large organizations with complex policies.