Fix external group handling, mergeDeep crash, and config validator gap

[vish-dawange]

Fixed by- Vishal Dawange

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.

[Copilot]

Pull request overview

This PR improves Safe-Settings’ robustness and PR dry-run output by making external group syncing more tolerant of SCIM timing, preventing a mergeDeep crash when combining additions with primitive modifications, and extending the dry-run reporting pipeline to support non-fatal warnings.

Changes:

• Make external group resolution case-insensitive and treat missing external groups as non-fatal (emit WARNING in NOP mode).
• Add WARNING handling to dry-run (NOP) results aggregation and PR comment rendering, without failing the check run.
• Fix mergeDeep’s visited-merge logic to avoid Object.assign on primitive values; add regression tests.

Show a summary per file

File	Description
lib/plugins/teams.js	Case-insensitive external group lookup and non-fatal handling for missing groups with WARNING in NOP mode.
lib/settings.js	Adds centralized ruleset bypass actor merging and extends NOP result handling/rendering to include WARNING.
lib/mergeDeep.js	Prevents primitive Object.assign crash when folding additions into modifications.
test/unit/lib/plugins/teams.test.js	Updates expectations for WARNING behavior when external groups are missing.
test/unit/lib/mergeDeep.test.js	Adds regression test covering the primitive-crash scenario.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Low