fix(mergedeep): handle null actor_id in bypass actor comparison#1035
Open
tdabasinskas wants to merge 2 commits into
Open
fix(mergedeep): handle null actor_id in bypass actor comparison#1035tdabasinskas wants to merge 2 commits into
null actor_id in bypass actor comparison#1035tdabasinskas wants to merge 2 commits into
Conversation
GitHub ignores actor_id for certain bypass actor types (OrganizationAdmin, DeployKey) and always returns null. Previously, the merge logic would fail to converge because it couldn't properly identify these actors when comparing config to live state. Changes: - Add actor_type to IDENTITY_FIELDS for proper actor differentiation - Skip null-valued identity fields when finding the primary identifier - Update sample config to use null actor_id for OrganizationAdmin - Add test coverage for null actor_id scenarios
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes non-converging ruleset bypass_actors diffs when GitHub returns actor_id: null for certain actor types (notably OrganizationAdmin and DeployKey), by improving identity selection during deep comparison so config-to-live matching works correctly.
Changes:
- Adds
actor_typeto the identity-field scan and skipsnullidentity values socompareDeepcan matchbypass_actorseven whenactor_idisnull. - Updates the sample settings to document
actor_id: nullforOrganizationAdmin. - Adds unit tests covering the null
actor_idbypass-actor scenario (with a recommended extension to also coverDeployKey).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/mergeDeep.js | Updates identity selection logic used by deep comparison for object-array matching. |
| test/unit/lib/mergeDeep.test.js | Adds regression tests for bypass_actors entries where actor_id is null. |
| docs/sample-settings/settings.yml | Updates sample ruleset configuration to use actor_id: null for OrganizationAdmin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add test case to verify that bypass actors with null `actor_id` but different `actor_type` values (e.g., OrganizationAdmin vs DeployKey) are correctly distinguished during comparison. This ensures the fix for null actor_id comparison properly handles multiple actor types with null IDs.
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.
Fixes #1034.
GitHub ignores
actor_idfor certain bypass actor types (OrganizationAdmin,DeployKey) and always returnsnull. Previously, the merge logic would fail to converge because it couldn't properly identify these actors when comparing config to live state.Changes: