Verify environment reviewers during release setup

context/getting-started.md

@@ -69,10 +69,14 @@ reviewers:
 
 Replace `your-org/managers` with your organization and team slug, for example `socketry/managers`. Individual user logins are also supported. Reviewers need at least read access to the repository. Setup resolves their GitHub IDs without changing repository access or team membership.
 
+Reviewer teams must have **Visible** visibility in GitHub team settings. Plan and apply reject **Secret** teams before changing any settings, because GitHub can silently discard them from the environment reviewer list. Setup does not change team visibility.
+
 GitHub accepts one to six users or teams, and **one approval from any listed reviewer or team member is sufficient**. It does not support a minimum environment approval count. The default two PR approvals are independent of this publishing approval.
 
 Create the environment and restrict its deployment branch as described above before running plan or apply with reviewers configured. The plan previews the current and desired environment settings. Apply replaces its reviewer list while preserving its wait timer, self-review prevention, administrator bypass setting, and deployment branch restrictions. Custom deployment protection rules are managed separately and are not modified. Reapplying an identical reviewer list leaves the environment unchanged.
 
+After updating the environment, apply reads its settings back and fails if the reviewer identities or preserved protections differ from the requested configuration. Reviewer order does not matter. Earlier updates may already have completed; inspect the environment in GitHub and rerun `gem:github:setup:plan` before retrying apply.
+
 Omitting `reviewers` leaves environment settings unmanaged, including any existing reviewer requirement. An empty list is rejected. To remove an existing requirement, change the environment settings explicitly in GitHub.
 
 The RubyGems Trusted Publisher must explicitly require the `rubygems` environment; leaving that field blank would allow this trusted publisher to authenticate jobs without the environment approval. Keep administrator bypass enabled if administrators should be able to explicitly authorize publication without a reviewer. Environment approvals are available for public repositories on GitHub Free.

fixtures/bake/gem/github/project_client.rb

@@ -32,7 +32,9 @@ def readlines(*arguments, **options)
 						@writes << File.read(arguments[arguments.index("--body-file") + 1])
 						["https://github.com/socketry/example/pull/42\n"]
 					else
-						[JSON.generate(@responses.fetch(arguments.fetch(2)))]
+						response = @responses.fetch(arguments.fetch(2))
+						response = response.call if response.respond_to?(:call)
+						[JSON.generate(response)]
 					end
 				end
 

guides/getting-started/readme.md

@@ -69,10 +69,14 @@ reviewers:
 
 Replace `your-org/managers` with your organization and team slug, for example `socketry/managers`. Individual user logins are also supported. Reviewers need at least read access to the repository. Setup resolves their GitHub IDs without changing repository access or team membership.
 
+Reviewer teams must have **Visible** visibility in GitHub team settings. Plan and apply reject **Secret** teams before changing any settings, because GitHub can silently discard them from the environment reviewer list. Setup does not change team visibility.
+
 GitHub accepts one to six users or teams, and **one approval from any listed reviewer or team member is sufficient**. It does not support a minimum environment approval count. The default two PR approvals are independent of this publishing approval.
 
 Create the environment and restrict its deployment branch as described above before running plan or apply with reviewers configured. The plan previews the current and desired environment settings. Apply replaces its reviewer list while preserving its wait timer, self-review prevention, administrator bypass setting, and deployment branch restrictions. Custom deployment protection rules are managed separately and are not modified. Reapplying an identical reviewer list leaves the environment unchanged.
 
+After updating the environment, apply reads its settings back and fails if the reviewer identities or preserved protections differ from the requested configuration. Reviewer order does not matter. Earlier updates may already have completed; inspect the environment in GitHub and rerun `gem:github:setup:plan` before retrying apply.
+
 Omitting `reviewers` leaves environment settings unmanaged, including any existing reviewer requirement. An empty list is rejected. To remove an existing requirement, change the environment settings explicitly in GitHub.
 
 The RubyGems Trusted Publisher must explicitly require the `rubygems` environment; leaving that field blank would allow this trusted publisher to authenticate jobs without the environment approval. Keep administrator bypass enabled if administrators should be able to explicitly authorize publication without a reviewer. Environment approvals are available for public repositories on GitHub Free.

lib/bake/gem/github/project.rb

@@ -157,6 +157,7 @@ def inspect_commit(commit)
 
 				# Return a read-only comparison of managed settings and current repository settings.
 				# @returns [Hash] Desired rules, existing rules, environments, optional environment changes, and expected Trusted Publisher settings. This does not verify RubyGems ownership or publisher configuration.
+				# @raises [RuntimeError] If a reviewer team is secret or belongs to another organization.
 				def doctor
 					{
 						desired_rules: Setup.rules(@config),
@@ -174,8 +175,9 @@ def doctor
 
 				# Apply the named rulesets and configured reviewers for an existing environment. Invoke after reviewing doctor output.
 				# Preserves the environment's wait timer, self-review prevention, administrator bypass, and branch restrictions.
+				# Reads back updated environment settings to verify GitHub retained them.
 				# @returns [Hash] The desired ruleset payloads after successful application.
-				# @raises [RuntimeError] If more than one existing ruleset has a managed name.
+				# @raises [RuntimeError] If a reviewer team is unsuitable, managed rulesets are ambiguous, or environment verification fails. Earlier updates may already have completed.
 				# @raises [Bake::Gem::CommandExecutionError] If an API operation fails; earlier updates may already have completed.
 				def apply
 					changes = environment_changes
@@ -193,6 +195,9 @@ def apply
 
 					if changes && changes.fetch(:current) != changes.fetch(:desired)
 						write_api("repos/#{@repository}/#{environment_path}", changes.fetch(:desired), method: "PUT")
+						unless environment_settings == changes.fetch(:desired)
+							raise "GitHub did not retain the requested settings for environment #{changes.fetch(:name)}. Check its reviewers and protection rules in GitHub, then rerun gem:github:setup:plan."
+						end
 					end
 
 					return rules
@@ -209,19 +214,26 @@ def environment_changes
 					return nil unless @config.key?("reviewers")
 					Setup.validate_reviewers(@config["reviewers"])
 
+					current = environment_settings
+					reviewers = @config.fetch("reviewers").map{|name| resolve_reviewer(name)}.uniq.sort_by{|reviewer| reviewer.values_at(:type, :id)}
+
+					return {name: @config.fetch("environment"), current: current, desired: current.merge(reviewers: reviewers)}
+				end
+
+				# Read the environment settings, comparing reviewer identities independently of their order.
+				def environment_settings
 					environment = api(environment_path)
 					protections = environment.fetch("protection_rules").to_h{|rule| [rule.fetch("type"), rule]}
 					reviews = protections.fetch("required_reviewers", {})
-					current = {
+					reviewers = reviews.fetch("reviewers", []).map{|entry| {type: entry.fetch("type"), id: entry.fetch("reviewer").fetch("id")}}
+
+					return {
 						wait_timer: protections.fetch("wait_timer", {}).fetch("wait_timer", 0),
 						prevent_self_review: reviews.fetch("prevent_self_review", false),
 						can_admins_bypass: environment.fetch("can_admins_bypass"),
 						deployment_branch_policy: environment.fetch("deployment_branch_policy"),
-						reviewers: reviews.fetch("reviewers", []).map{|entry| {type: entry.fetch("type"), id: entry.fetch("reviewer").fetch("id")}},
+						reviewers: reviewers.uniq.sort_by{|reviewer| reviewer.values_at(:type, :id)},
 					}
-					reviewers = @config.fetch("reviewers").map{|name| resolve_reviewer(name)}
-
-					return {name: @config.fetch("environment"), current: current, desired: current.merge(reviewers: reviewers)}
 				end
 
 				def resolve_reviewer(name)
@@ -235,6 +247,9 @@ def resolve_reviewer(name)
 						type = "User"
 					end
 					response = JSON.parse(readlines("gh", "api", path, chdir: @root).join)
+					if type == "Team" && response.fetch("privacy") == "secret"
+						raise "Reviewer team #{name} is Secret. Change its visibility to Visible in GitHub team settings before running setup."
+					end
 
 					return {type: type, id: response.fetch("id")}
 				end

releases.md

@@ -1,5 +1,9 @@
 # Releases
 
+## Unreleased
+
+  - Reject Secret environment reviewer teams before applying release settings, and verify that GitHub retained the requested reviewers and existing protections after updating the environment.
+
 ## v0.3.1
 
   - Publish validated release PRs from pushes to the default branch, retaining the exact merged commit and environment approval without requiring `pull_request_target`.

test/bake/gem/github/project/environment.rb

@@ -18,12 +18,26 @@
 		}
 	end
 
+	let(:saved_reviewers) {[{"type" => "Team", "reviewer" => {"id" => 123, "slug" => "managers"}}]}
+	let(:saved_environment) do
+		{
+			"can_admins_bypass" => true,
+			"deployment_branch_policy" => {"protected_branches" => false, "custom_branch_policies" => true},
+			"protection_rules" => [
+				{"type" => "branch_policy"},
+				{"type" => "required_reviewers", "prevent_self_review" => false, "reviewers" => saved_reviewers},
+			],
+		}
+	end
+
 	before do
 		Bake::Gem::GitHub::Setup.new(root).generate(repository: "socketry/example", checks: ["Tests"], reviewers: ["socketry/managers"])
 		project.responses["repos/socketry/example/rulesets?per_page=100"] = []
 		project.responses["repos/socketry/example/environments"] = {"environments" => [environment]}
-		project.responses["repos/socketry/example/environments/rubygems"] = environment
-		project.responses["orgs/socketry/teams/managers"] = {"id" => 123}
+		project.responses["repos/socketry/example/environments/rubygems"] = -> do
+			project.writes.any?{|payload| payload.key?("reviewers")} ? saved_environment : environment
+		end
+		project.responses["orgs/socketry/teams/managers"] = {"id" => 123, "privacy" => "closed"}
 	end
 
 	with "#doctor" do
@@ -53,6 +67,9 @@
 	with "#apply" do
 		it "updates reviewers without weakening existing environment protections" do
 			environment["can_admins_bypass"] = false
+			saved_environment["can_admins_bypass"] = false
+			saved_environment["protection_rules"] << {"type" => "wait_timer", "wait_timer" => 30}
+			saved_environment["protection_rules"].find{|rule| rule["type"] == "required_reviewers"}["prevent_self_review"] = true
 			environment["protection_rules"] += [
 				{"type" => "wait_timer", "wait_timer" => 30},
 				{"type" => "required_reviewers", "prevent_self_review" => true, "reviewers" => [{"type" => "User", "reviewer" => {"id" => 456}}]},
@@ -66,12 +83,13 @@
 				"deployment_branch_policy" => environment.fetch("deployment_branch_policy"),
 				"reviewers" => [{"type" => "Team", "id" => 123}],
 			}
-			expect(project.requests.last[0, 5]).to be == ["gh", "api", "repos/socketry/example/environments/rubygems", "--method", "PUT"]
+			expect(project.requests.last).to be == ["gh", "api", "repos/socketry/example/environments/rubygems"]
 			expect(project.writes.size).to be == 5
 		end
 
 		it "preserves an unrestricted branch policy and enabled administrator bypass" do
 			environment["deployment_branch_policy"] = nil
+			saved_environment["deployment_branch_policy"] = nil
 			project.apply
 
 			expect(project.writes.last).to have_keys("deployment_branch_policy" => be_nil, "can_admins_bypass" => be == true)
@@ -85,6 +103,67 @@
 			expect(project.writes.all?{|payload| payload.key?("rules")}).to be == true
 		end
 
+		it "accepts reordered reviewers and ignores duplicate configured identities" do
+			project.config["reviewers"] = ["ioquatix", "socketry/managers", "ioquatix"]
+			project.responses["users/ioquatix"] = {"id" => 456}
+			saved_reviewers.unshift({"type" => "User", "reviewer" => {"id" => 456}})
+
+			project.apply
+			expect(project.writes.last.fetch("reviewers")).to be == [{"type" => "Team", "id" => 123}, {"type" => "User", "id" => 456}]
+
+			project.apply
+			expect(project.writes.count{|payload| payload.key?("reviewers")}).to be == 1
+		end
+
+		it "rejects a successful write which silently drops every reviewer" do
+			saved_reviewers.clear
+
+			expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain.*rubygems.*gem:github:setup:plan/)
+			expect(project.writes.last.fetch("reviewers")).to be == [{"type" => "Team", "id" => 123}]
+		end
+
+		it "rejects a successful write which drops one of the requested reviewers" do
+			project.config["reviewers"] << "ioquatix"
+			project.responses["users/ioquatix"] = {"id" => 456}
+
+			expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
+		end
+
+		it "rejects an unexpected reviewer who could approve publication" do
+			saved_reviewers << {"type" => "User", "reviewer" => {"id" => 456}}
+
+			expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
+		end
+
+		it "distinguishes user and team reviewers with the same numeric ID" do
+			saved_reviewers.first["type"] = "User"
+
+			expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
+		end
+
+		it "rejects a successful write which removes the reviewer protection rule" do
+			saved_environment["protection_rules"].reject!{|rule| rule["type"] == "required_reviewers"}
+
+			expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
+		end
+
+		it "rejects a successful write which changes an existing protection" do
+			environment["can_admins_bypass"] = false
+
+			expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/)
+			expect(project.writes.last.fetch("can_admins_bypass")).to be == false
+		end
+
+		it "propagates failure to read back the applied environment" do
+			project.responses["repos/socketry/example/environments/rubygems"] = -> do
+				raise "GitHub readback failed" unless project.writes.empty?
+				environment
+			end
+
+			expect{project.apply}.to raise_exception(RuntimeError, message: be == "GitHub readback failed")
+			expect(project.writes.size).to be == 5
+		end
+
 		it "leaves environment settings unmanaged when reviewers are omitted" do
 			project.config.delete("reviewers")
 			project.apply
@@ -125,10 +204,21 @@
 
 		it "escapes the configured environment name in API requests" do
 			project.config["environment"] = "release / gems"
-			project.responses["repos/socketry/example/environments/release%20%2F%20gems"] = environment
+			project.responses["repos/socketry/example/environments/release%20%2F%20gems"] = project.responses.fetch("repos/socketry/example/environments/rubygems")
 			project.apply
 
 			expect(project.requests.last.fetch(2)).to be == "repos/socketry/example/environments/release%20%2F%20gems"
 		end
 	end
+
+	[:doctor, :apply].each do |operation|
+		with "##{operation}" do
+			it "rejects secret reviewer teams before changing any settings" do
+				project.responses["orgs/socketry/teams/managers"]["privacy"] = "secret"
+
+				expect{project.public_send(operation)}.to raise_exception(RuntimeError, message: be =~ /socketry\/managers is Secret.*Visible/)
+				expect(project.writes).to be == []
+			end
+		end
+	end
 end