diff --git a/data/yo61/homebrew-tap.yaml b/data/yo61/homebrew-tap.yaml index e1c2e07..ce63d0c 100644 --- a/data/yo61/homebrew-tap.yaml +++ b/data/yo61/homebrew-tap.yaml @@ -2,6 +2,13 @@ allow_auto_merge: true allow_update_branch: false default_branch_ruleset_bypass_actors: [] +# publish-bottles.yaml pushes the bottle commit to the bump PR branch and then +# squash-merges it. With dismissal on, that push dismisses lastlight's approval +# — the very approval the merge needs — and the merge fails on "At least 1 +# approving review is required". Re-approval takes ~2min, well past the +# workflow's retry window, and its retry path is not idempotent (bottle assets +# already exist on the release). See homebrew-tap#104. +default_branch_ruleset_dismiss_stale_reviews_on_push: false default_branch_ruleset_required_approving_review_count: 1 delete_branch_on_merge: true dependabot_security_updates: true diff --git a/decisions/2026-08-12-homebrew-tap-dismiss-stale-override.md b/decisions/2026-08-12-homebrew-tap-dismiss-stale-override.md new file mode 100644 index 0000000..82f24a3 --- /dev/null +++ b/decisions/2026-08-12-homebrew-tap-dismiss-stale-override.md @@ -0,0 +1,89 @@ +## Decision: Make `dismiss_stale_reviews_on_push` a per-repo variable (module default unchanged at `true`) and set it `false` on `homebrew-tap` alone + +`homebrew-tap`'s release automation pushes a commit to the PR branch *after* the +approving review and then merges that branch. With dismissal on, the push +dismisses the very approval the merge depends on, and the release cannot land. +The knob was hardcoded in `modules/github-repo/data/rulesets.yaml`, so there was +no way to change it for one repo. It is now variable-driven — the same treatment +`required_approving_review_count` and `require_last_push_approval` already get — +defaulting to `true`, so every other repo is unaffected. + +## Context + +`eabc303` ("bring homebrew-tap and reportlab-pdf back under the policy", #62) +set `default_branch_ruleset_required_approving_review_count: 1` on the tap at +2026-08-11 22:05 UTC; the ruleset applied at 22:37:56 UTC. `homebrew-tap#104` +(an automated jobhound 0.18.0 bump) was opened at 23:38 UTC — the first release +bump to run under the new policy. It failed: + +1. `publish-bottles.yaml` builds bottles, uploads them to the release, then + commits the `bottle do` block to the bump branch via `createCommitOnBranch`. +2. That push dismissed `yo61-lastlight[bot]`'s approval + (`dismiss_stale_reviews_on_push: true`). +3. The workflow's squash-merge then failed five times over ~35s with + `Repository rule violations found — At least 1 approving review is required + by reviewers with write access` (HTTP 405). +4. lastlight re-approved at 01:23:20 UTC, ~40s after the workflow gave up. +5. The automatic retry (a second `workflow_run` from the push) failed *earlier*, + at `brew pr-upload`: `Validation Failed: ReleaseAsset already_exists`. The + upload step is not idempotent, so the pipeline cannot recover on its own. + +The bottle assets, the bottle commit, and green CI all exist; only the merge is +blocked. This is structural, not a one-off: every release bump pushes after +review, so every release would fail the same way. + +## Alternatives considered + +- **Set `dismiss_stale_reviews_on_push: false` in the module default.** Rejected: + `[[2026-08-06-unifi-mcp-ci-only-gate]]` already rejected exactly this ("Fixing + release-PR friction by weakening the approval-to-code binding was the wrong + trade") and recorded that the `true` default "stays as the module default + fleet-wide". Nothing has changed to invalidate that reasoning for the other + repos — the tap is distinctive in that its automation pushes after review. +- **Drop the tap's `required_approving_review_count` to 0**, the + `[[2026-08-06-unifi-mcp-ci-only-gate]]` shape: with no approval to dismiss, + dismissal becomes a no-op. Rejected here: #62 deliberately brought this repo + *under* the review policy a day earlier, and reverting that within 24 hours + discards the intent rather than accommodating it. Also worth noting the tap's + own `2026-07-13-signed-commits-via-api-drop-bypass` recorded 0 approvals, so + this would have been a return to the prior state, not a novel relaxation. +- **Leave Terraform alone; fix the pipeline** — widen `publish-bottles.yaml`'s + merge retry past lastlight's ~2min re-review and make the asset upload + idempotent. Not chosen as the primary fix (it leaves the workflow racing a + dismissal it causes itself), but the idempotency half remains worth doing + independently; see Trade-offs. + +## Reasoning + +- **The dismissal rule and this pipeline are incompatible by construction.** The + rule exists to stop code changing after a human approved it. Here the approver + is a bot, and the post-approval push is a machine-generated bottle block whose + content is derived from the very CI run that approved the branch. The property + the rule protects is not the property at risk. +- **Per-repo, not fleet-wide, keeps the earlier decision intact.** Adding the + variable with `default = true` means the other nine repos produce an identical + plan; only `homebrew-tap` deviates, and it says so in its own YAML with a + comment pointing at #104. +- **Follows the established plumbing.** `bypass_actors`, + `required_approving_review_count` and `require_last_push_approval` are already + injected from variables rather than hardcoded in the ruleset catalog; the + in-code comment states the reason ("so each org / repo can supply its own + without forking the ruleset catalog"). Moving `dismiss_stale_reviews_on_push` + out of the YAML into a variable extends that pattern rather than inventing one. + +## Trade-offs accepted + +- On `homebrew-tap`, an approval now survives *any* later push to the branch, + including a hand-pushed one, not just the bottle commit. Accepted: the repo's + approvals come from lastlight on every push anyway, and its status-check gate + is unchanged. +- `publish-bottles.yaml` keeps a non-idempotent upload step, so a failed run + still cannot be re-run cleanly. This change removes the cause of the failure + rather than making recovery work; the retry path is a separate fix. +- One more knob on the module surface. Bounded — it mirrors three existing ones + and defaults to current behaviour. + +## Supersedes + +None. `[[2026-08-06-unifi-mcp-ci-only-gate]]` stands unchanged: its rejection was +of altering the *module default*, which this preserves at `true`. diff --git a/modules/github-repo/README.md b/modules/github-repo/README.md index 234d596..fdbe5d8 100644 --- a/modules/github-repo/README.md +++ b/modules/github-repo/README.md @@ -308,6 +308,7 @@ No modules. | [create\_default\_branch](#input\_create\_default\_branch) | Have terraform create the default branch resource. Defaults to false; the default branch on existing repos is left unmanaged unless explicitly opted in. | `bool` | `false` | no | | [default\_branch](#input\_default\_branch) | The name of the default branch of the repository | `string` | `"main"` | no | | [default\_branch\_ruleset\_bypass\_actors](#input\_default\_branch\_ruleset\_bypass\_actors) | Actors permitted to bypass the default\_branch built-in ruleset. Empty means no bypass. |
list(object({
actor_id = number
actor_type = string
bypass_mode = string
})) | `[]` | no |
+| [default\_branch\_ruleset\_dismiss\_stale\_reviews\_on\_push](#input\_default\_branch\_ruleset\_dismiss\_stale\_reviews\_on\_push) | Whether pushing to a PR branch dismisses existing approvals. Defaults to true. Set false on repos whose release automation pushes a commit after review (e.g. a bottle commit), where the rule would otherwise dismiss the approval the automation is about to merge on. | `bool` | `true` | no |
| [default\_branch\_ruleset\_require\_last\_push\_approval](#input\_default\_branch\_ruleset\_require\_last\_push\_approval) | Whether the most recent reviewable push must be approved by someone other than the pusher. With required\_approving\_review\_count = 0 and this = true, solo authors are still blocked. Defaults to false. | `bool` | `false` | no |
| [default\_branch\_ruleset\_required\_approving\_review\_count](#input\_default\_branch\_ruleset\_required\_approving\_review\_count) | Number of approving reviews required on PRs targeting the default branch when the default\_branch built-in ruleset is enabled. | `number` | `0` | no |
| [delete\_branch\_on\_merge](#input\_delete\_branch\_on\_merge) | Automatically delete head branch after a pull request is merged. | `bool` | `true` | no |
diff --git a/modules/github-repo/data.tf b/modules/github-repo/data.tf
index 9147e7e..f5deaa5 100644
--- a/modules/github-repo/data.tf
+++ b/modules/github-repo/data.tf
@@ -40,7 +40,8 @@ locals {
builtin_rulesets_raw = yamldecode(file(local.rulesets_file))
# Inject variable-driven fields into the default_branch built-in ruleset
- # (bypass_actors, required_approving_review_count). Kept out of the YAML so
+ # (bypass_actors, required_approving_review_count,
+ # dismiss_stale_reviews_on_push). Kept out of the YAML so
# each org / repo can supply its own without forking the ruleset catalog.
# Other built-ins (when added) pass through unchanged via the outer merge.
# Assumes every rule in the default_branch ruleset has a pull_request block.
@@ -50,6 +51,7 @@ locals {
rules = [
for rule in local.builtin_rulesets_raw["default_branch"].rules : merge(rule, {
pull_request = merge(rule.pull_request, {
+ dismiss_stale_reviews_on_push = var.default_branch_ruleset_dismiss_stale_reviews_on_push
require_last_push_approval = var.default_branch_ruleset_require_last_push_approval
required_approving_review_count = var.default_branch_ruleset_required_approving_review_count
})
diff --git a/modules/github-repo/data/rulesets.yaml b/modules/github-repo/data/rulesets.yaml
index 476f889..5eed787 100644
--- a/modules/github-repo/data/rulesets.yaml
+++ b/modules/github-repo/data/rulesets.yaml
@@ -13,7 +13,6 @@ default_branch:
deletion: true
non_fast_forward: true
pull_request:
- dismiss_stale_reviews_on_push: true
require_code_owner_review: false
required_review_thread_resolution: false
required_signatures: true
diff --git a/modules/github-repo/variables.tf b/modules/github-repo/variables.tf
index 5dfe76a..b3359f1 100644
--- a/modules/github-repo/variables.tf
+++ b/modules/github-repo/variables.tf
@@ -166,6 +166,13 @@ variable "default_branch_ruleset_bypass_actors" {
nullable = false
}
+variable "default_branch_ruleset_dismiss_stale_reviews_on_push" {
+ description = "Whether pushing to a PR branch dismisses existing approvals. Defaults to true. Set false on repos whose release automation pushes a commit after review (e.g. a bottle commit), where the rule would otherwise dismiss the approval the automation is about to merge on."
+ type = bool
+ default = true
+ nullable = false
+}
+
variable "default_branch_ruleset_require_last_push_approval" {
description = "Whether the most recent reviewable push must be approved by someone other than the pusher. With required_approving_review_count = 0 and this = true, solo authors are still blocked. Defaults to false."
type = bool
diff --git a/modules/org/main.tf b/modules/org/main.tf
index 5fa76b5..6767beb 100644
--- a/modules/org/main.tf
+++ b/modules/org/main.tf
@@ -40,6 +40,9 @@ module "repo" {
contains(local.non_fork_names, each.key) ? var.default_branch_ruleset_non_fork_bypass_actors : []
)
)
+ default_branch_ruleset_dismiss_stale_reviews_on_push = lookup(
+ each.value, "default_branch_ruleset_dismiss_stale_reviews_on_push", var.default_branch_ruleset_dismiss_stale_reviews_on_push
+ )
default_branch_ruleset_require_last_push_approval = lookup(
each.value, "default_branch_ruleset_require_last_push_approval", var.default_branch_ruleset_require_last_push_approval
)
diff --git a/modules/org/variables.tf b/modules/org/variables.tf
index 61653cf..a43f38a 100644
--- a/modules/org/variables.tf
+++ b/modules/org/variables.tf
@@ -26,6 +26,13 @@ variable "default_branch_ruleset_non_fork_bypass_actors" {
nullable = false
}
+variable "default_branch_ruleset_dismiss_stale_reviews_on_push" {
+ description = "Org-wide default for whether pushing to a PR branch dismisses existing approvals. Defaults to true. Per-repo YAML can override, for repos whose release automation pushes after review."
+ type = bool
+ default = true
+ nullable = false
+}
+
variable "default_branch_ruleset_require_last_push_approval" {
description = "Org-wide default for whether the most recent reviewable push must be approved by someone other than the pusher. Defaults to false so solo authors aren't blocked even when the pull_request rule is active. Per-repo YAML can override."
type = bool