Feature/compress pr comment#989
Open
madkoo wants to merge 32 commits into
Open
Conversation
…s in settings.json
…erty in settings.json
…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.
…protection methods
…to main-enterprise
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ation - Extract isEmptyChange() to filter null/empty top-level actions - Refactor handleResults() with structured per-plugin HTML sections - Add pagination support for comments exceeding GitHub API limits - Add org-level result labeling in updateOrg() - Update ETA template in commentmessage.js - Add comprehensive test suite for handleResults() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nce isEmptyChange logic
Render NOP validation results as compact per-plugin tables that show the affected repo, policy or setting target, and concise change summary. Keep the existing relevance filtering and pagination behavior while using the same compact row model for PR comments and check-run summaries. Prefer structured additions, deletions, and modifications over generic NOP action messages, using msg only as a fallback when no structured rows exist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Revise NOP validation comments to keep an affected-repos overview while rendering detailed per-plugin, per-repo, and per-rule field changes. Show before/after rows for modified fields, added/deleted rows for fields that only exist on one side, and nested details for complex values. Preserve existing PR-introduced-change filtering and keep comment/check-run length safeguards by reserving truncation suffix space. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Do not suppress non-identity fields just because their value matches the changed target name. Identity fields are already skipped by path while flattening, so same-value fields like description: bug should remain visible in field-level NOP comment details. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the NOP change matrix and HTML field tables with a table-free bullet layout that groups changes by plugin, target, and rule or setting. Preserve admin-repo display for organization-level settings and keep changed-field-only filtering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds richer NOP (dry-run) reporting and filtering so PR comments/check runs focus on PR-introduced config changes, plus improves selection of affected repos when repo/suborg config files changed.
Changes:
- Add base-branch config comparison and changed-file tracking (repos + suborgs) to filter out pre-existing drift in NOP results.
- Replace table-based NOP output with a collapsible, field-level diff summary and paginate PR comments to stay within GitHub limits.
- Update ruleset bypass actor schema to include
User, and refactor custom properties normalization.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/lib/settings.test.js | Adds coverage for changed config tracking (override + suborg-driven repo targeting). |
| test/unit/lib/handleResults.test.js | Adds extensive tests for new handleResults rendering, truncation/pagination, and filtering behavior. |
| lib/settings.js | Implements deep-empty detection, base-config drift filtering, changed-file repo targeting, and new markdown rendering + pagination. |
| lib/commentmessage.js | Updates check-run template to reflect new summary/details rendering and error presentation. |
| index.js | Loads base config for NOP filtering and passes changed-files metadata into sync flows. |
| lib/plugins/custom_properties.js | Refactors normalization logic for properties. |
| schema/dereferenced/settings.json | Updates repo schema text and ruleset bypass actor types; removes deprecated team permission field. |
| schema/dereferenced/repos.json | Updates ruleset bypass actor schema to include User. |
| schema/dereferenced/suborgs.json | Updates ruleset bypass actor schema to include User. |
Comments suppressed due to low confidence (1)
lib/settings.js:1
- The
errorflag is never set to true when ERROR results are encountered, so check-run output will report success even whenstats.errorsis populated. Seterror = trueinside the ERROR branch (and ensure any related conclusion/status logic uses the same flag) so the check run accurately reflects failures.
const path = require('path')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+31
| function isEmptyChange (action) { | ||
| if (!action) return true | ||
| const { additions, deletions, modifications } = action | ||
| if (additions === null && deletions === null && modifications === null) return true | ||
| return isDeepEmpty(additions) && isDeepEmpty(deletions) && isDeepEmpty(modifications) | ||
| } |
| }) | ||
| } | ||
|
|
||
| let error = false |
| @@ -305,7 +881,7 @@ ${this.results.reduce((x, y) => { | |||
| completed_at: new Date().toISOString(), | |||
| output: { | |||
| title: error ? 'Safe-Settings Dry-Run Finished with Error' : 'Safe-Settings Dry-Run Finished with success', | |||
Comment on lines
+38
to
+47
| return properties | ||
| .map(({ property_name: propertyName, name, value }) => ({ | ||
| name: propertyName ?? name, | ||
| value | ||
| })) | ||
| .filter(({ name }) => name != null) | ||
| .map(({ name, value }) => ({ | ||
| name: name.toLowerCase(), | ||
| value | ||
| })) |
| * Determines which named entries in an array-based config section actually changed | ||
| * between the base branch and the PR branch. Returns a Set of entry names that differ. | ||
| */ | ||
| function getChangedEntryNames (baseEntries, prEntries) { |
Comment on lines
+51
to
+64
| for (const prEntry of prEntries) { | ||
| if (!prEntry.name) continue | ||
| const baseEntry = baseEntries.find(b => b.name === prEntry.name) | ||
| if (!baseEntry || JSON.stringify(baseEntry) !== JSON.stringify(prEntry)) { | ||
| changed.add(prEntry.name) | ||
| } | ||
| } | ||
| // Check for deleted entries | ||
| for (const baseEntry of baseEntries) { | ||
| if (!baseEntry.name) continue | ||
| if (!prEntries.find(p => p.name === baseEntry.name)) { | ||
| changed.add(baseEntry.name) | ||
| } | ||
| } |
Comment on lines
+837
to
+838
| const HEADER_OVERHEAD = 80 | ||
| const BODY_LIMIT = COMMENT_LIMIT - HEADER_OVERHEAD |
decyjphr
requested changes
Jun 6, 2026
Collaborator
decyjphr
left a comment
decyjphr
left a comment
There was a problem hiding this comment.
I tried to run safe-settings (main-enterprise merged with this branch) with this repo.yml
repository:
name: test
description: Demo repository created via safe-settings
private: true
auto_init: true
force_create: true
has_issues: true
has_projects: false
has_wiki: false
delete_branch_on_merge: true
allow_squash_merge: true
allow_merge_commit: false
allow_rebase_merge: true
teams:
- name: expert-services-developers
permission: push
custom_properties:
- property_name: ent-ownership
value: expert-services
- property_name: ent-supervisory-org
value: expert-services
rulesets:
- name: synk
target: branch
enforcement: disabled
bypass_actors:
- actor_id: 1
actor_type: OrganizationAdmin
bypass_mode: pull_request
conditions:
ref_name:
include: ["~DEFAULT_BRANCH"]
exclude: ["refs/heads/oldmaster"]
rules:
- type: creation
- type: update
- type: deletion
- type: required_linear_history
- type: required_signatures
- type: pull_request
parameters:
dismiss_stale_reviews_on_push: true
require_code_owner_review: true
require_last_push_approval: true
required_approving_review_count: 2
required_review_thread_resolution: true
- type: commit_message_pattern
parameters:
name: test commit_message_pattern
negate: true
operator: starts_with
pattern: skip*
- type: commit_author_email_pattern
parameters:
name: test commit_author_email_pattern
negate: false
operator: regex
pattern: "^.*@example.com$"
- type: committer_email_pattern
parameters:
name: test committer_email_pattern
negate: false
operator: regex
pattern: "^.*@example.com$"
- type: branch_name_pattern
parameters:
name: test branch_name_pattern
negate: false
operator: regex
pattern: ".*\\/.*"
- name: Prevent merges when new SONAR alerts are introduced
target: branch
enforcement: active
conditions:
ref_name:
include:
- "~DEFAULT_BRANCH"
exclude: []
bypass_actors:
- actor_type: OrganizationAdmin
bypass_mode: always
rules:
- type: code_scanning
parameters:
code_scanning_tools:
- tool: Sonar
alerts_threshold: none
security_alerts_threshold: medium_or_higher
and I am getting this error:
DEBUG (event): Results of comparing CustomProperties diffable target [] with source [{"name":"ent-ownership","value":"expert-services"},{"name":"ent-supervisory-org","value":"expert-services"}] is {"msg":"Changes found","additions":{"0":{"name":"ent-ownership","value":"expert-services"},"1":{"name":"ent-supervisory-org","value":"expert-services"}},"modifications":{}}
id: "dc7eeaaa-61f6-11f1-9fe1-cebbd84a1535"
/Users/decyjphr/projects/node/probot/safe-settings-mar-26/lib/plugins/custom_properties.js:88
this.github.rest.repos.createOrUpdateCustomPropertiesValues.endpoint(params),
^
TypeError: Cannot read properties of undefined (reading 'endpoint')
at CustomProperties.modifyProperty (/Users/decyjphr/projects/node/probot/safe-settings-mar-26/lib/plugins/custom_properties.js:88:69)
at CustomProperties.add (/Users/decyjphr/projects/node/probot/safe-settings-mar-26/lib/plugins/custom_properties.js:63:17)
at /Users/decyjphr/projects/node/probot/safe-settings-mar-26/lib/plugins/diffable.js:115:33
at Array.forEach (<anonymous>)
at /Users/decyjphr/projects/node/probot/safe-settings-mar-26/lib/plugins/diffable.js:109:25
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.
This pull request introduces several enhancements and bug fixes focused on improving configuration change tracking, NOP (no-operation) diffing, schema accuracy, and reporting for the safe-settings app. The most notable updates include improved tracking of changed repository and sub-org configurations, better diffing logic between PR and base branch settings, and schema updates to support new actor types and clarify descriptions.
Change tracking and NOP diff improvements:
syncAllSettingsandsyncSelectedSettings. This allows more accurate reporting and filtering of changes that are PR-specific, especially during NOP runs. (index.js,Settings.syncAll,Settings.syncSelectedRepos) [1] [2] [3]test/unit/lib/settings.test.js)User-facing reporting improvements:
lib/commentmessage.js)Code quality and normalization:
lib/plugins/custom_properties.js) [1] [2]