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 |
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]