feat(yo61): allow per-repo dismiss_stale_reviews_on_push, off for homebrew-tap

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

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`.

modules/github-repo/README.md

@@ -308,6 +308,7 @@ No modules.
 | <a name="input_create_default_branch"></a> [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 |
 | <a name="input_default_branch"></a> [default\_branch](#input\_default\_branch) | The name of the default branch of the repository | `string` | `"main"` | no |
 | <a name="input_default_branch_ruleset_bypass_actors"></a> [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. | <pre>list(object({<br/>    actor_id    = number<br/>    actor_type  = string<br/>    bypass_mode = string<br/>  }))</pre> | `[]` | no |
+| <a name="input_default_branch_ruleset_dismiss_stale_reviews_on_push"></a> [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 |
 | <a name="input_default_branch_ruleset_require_last_push_approval"></a> [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 |
 | <a name="input_default_branch_ruleset_required_approving_review_count"></a> [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 |
 | <a name="input_delete_branch_on_merge"></a> [delete\_branch\_on\_merge](#input\_delete\_branch\_on\_merge) | Automatically delete head branch after a pull request is merged. | `bool` | `true` | no |

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
           })

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

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

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
   )

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