[i,l,-r] Auto-narrow reviewers on empty picker selection (SECOP-952)#11
Draft
phosphore wants to merge 1 commit into
Draft
[i,l,-r] Auto-narrow reviewers on empty picker selection (SECOP-952)#11phosphore wants to merge 1 commit into
phosphore wants to merge 1 commit into
Conversation
When an MPA elevation request was submitted without picking specific
reviewers, JoinOperation.propose fell back to the full policy ACL and
SlackProposalHandler expanded any group in it to individuals — so a
role whose approver list names a broad group (e.g.
engineering@roles.wave.com) DM'd hundreds of people.
GroupsResource.post now resolves an effective reviewer filter when the
selection is empty:
- small, group-free approver set -> unchanged (DM everyone; no Cloud
Identity calls);
- broad set (contains a group, or > AUTO_NARROW_BROADCAST_LIMIT) ->
narrow to the requester's suggested teammates from
ReviewerCandidates;
- broad set with no teammate signal -> reject with a BadRequest so the
requester picks at least one reviewer, rather than broadcasting.
Same fail-safe if teammate resolution errors out.
Adds three TestGroupsResource cases covering the null-filter, narrow,
and reject paths.
phosphore
changed the title
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
1 participant
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.
Why
When an MPA elevation request is submitted without picking specific reviewers,
JoinOperation.proposefalls back to the full policy ACL andSlackProposalHandlerexpands any group in it to individuals. So a role whose approver list names a broad group (e.g.engineering@roles.wave.com) DMs hundreds of people — far too loud. Tracked in SECOP-952.What
GroupsResource.postnow resolves an effective reviewer filter when the picker selection is empty:AUTO_NARROW_BROADCAST_LIMIT, no group): unchanged — DM everyone. Makes zero Cloud Identity calls (cheap guard).ReviewerCandidateslogic. Never fans out to the whole group.BadRequest("pick at least one reviewer") rather than broadcast. Same fail-safe if teammate resolution errors out (Cloud Identity outage / permission gap) — we'd rather ask than spam.New constant
AUTO_NARROW_BROADCAST_LIMIT = 15(a generous single-team size).This is server-side only — the authoritative spot, since the picker UI just supplies the selection. The picker already computes the
suggestedset; this reuses it for the empty-selection default.Tests
Three new
TestGroupsResourcecases: null-filter (small set), narrow-to-suggested, and reject-rather-than-broadcast. Full unit suite green locally (1011 tests, 0 failures).Notes / follow-ups
view.js) — out of scope here.15threshold is a judgment call; trivial to tune.🤖 Generated with Claude Code