fix(branches): refuse a non-boolean control at parse, and let a null "off" converge

docs/reference/coverage.md

@@ -182,6 +182,9 @@ The side-by-side comparison with the Probot Settings app lives in the migration
 - GET /repos/{owner}/{repo}/branches/{branch} probes whether a literal branch exists, so check mode can tell a missing branch from an unprotected one; the probe needs Contents: read.
 - What the settings file alone shows wrong is refused before any request, naming the key and the fix: required_status_checks without strict or without a check list (contexts or checks), required_approving_review_count outside 0-6, and the GET-only vocabulary a copied GET response carries (name, enabled, enforcement_level, url and *_url keys, at any depth).
 - Also refused at parse: an actor object copied from the GET into restrictions, dismissal_restrictions, or bypass_pull_request_allowances, where the PUT takes login/slug strings (the refusal names the string to write).
+- Every boolean of the PUT body is typed: the eight controls (enforce_admins through allow_fork_syncing) and the review settings dismiss_stale_reviews, require_code_owner_reviews, and require_last_push_approval. A quoted value ("yes", "no") is refused at parse on a literal branch and a wildcard rule alike, and a copied {url, enabled} wrapper at its control, naming the boolean to declare.
+- null means off under enforce_admins and allow_force_pushes, the two controls whose PUT schema spells off that way: check compares it as false and both the PUT and a wildcard rule's mutation are sent false, so a check right after an apply is clean.
+- Under any other of those booleans a literal branch refuses null at parse (the PUT answers 422 to it). A wildcard rule still takes it, since the mutation's Boolean inputs are nullable; write false there to turn a control off. required_signatures and required_status_checks.strict refuse null on every entry.
 - Check mode flattens the GET shape ({enabled} wrappers, actor objects -> login/slug strings, *_url dropped) to compare like with like (src/sections/branches/index.ts flattenProtection).
 - Check mode notes a declared key outside the PUT vocabulary that the GET never echoes as never converging; a documented key the GET omits, such as an off control, is drift the PUT resolves.
 - Actor logins and slugs compare case-insensitively (GitHub matches them in any case and reads back its own spelling), and a live review block without dismissal_restrictions or bypass_pull_request_allowances compares as the all-empty holder, which is how GitHub reads an empty one back.

docs/sections/branches.docs.yml

@@ -34,6 +34,21 @@ coverage:
         Also refused at parse: an actor object copied from the GET into restrictions,
         dismissal_restrictions, or bypass_pull_request_allowances, where the PUT takes login/slug
         strings (the refusal names the string to write).
+      - >-
+        Every boolean of the PUT body is typed: the eight controls (enforce_admins through
+        allow_fork_syncing) and the review settings dismiss_stale_reviews,
+        require_code_owner_reviews, and require_last_push_approval. A quoted value ("yes", "no") is
+        refused at parse on a literal branch and a wildcard rule alike, and a copied {url, enabled}
+        wrapper at its control, naming the boolean to declare.
+      - >-
+        null means off under enforce_admins and allow_force_pushes, the two controls whose PUT
+        schema spells off that way: check compares it as false and both the PUT and a wildcard
+        rule's mutation are sent false, so a check right after an apply is clean.
+      - >-
+        Under any other of those booleans a literal branch refuses null at parse (the PUT answers 422
+        to it). A wildcard rule still takes it, since the mutation's Boolean inputs are nullable;
+        write false there to turn a control off. required_signatures and
+        required_status_checks.strict refuse null on every entry.
       - >-
         Check mode flattens the GET shape ({enabled} wrappers, actor objects -> login/slug strings,
         *_url dropped) to compare like with like (src/sections/branches/index.ts flattenProtection).
@@ -185,10 +200,25 @@ schema:
   BranchConfig.protection: PUT .../protection payload; null removes protection (Probot parity).
   BranchProtectionConfig: >-
     The protection PUT payload, passed through verbatim except its routed keys; a key the GET never
-    echoes is noted at check time as never converging. What the file alone shows wrong is refused
-    upfront, naming the fix: the GET-only keys a copied GET response carries
-    (`name`, `enabled`, `enforcement_level`, `url` and `*_url`, at any depth), a status-check
-    requirement without `strict` or a check list, and a review count outside 0-6.
+    echoes is noted as never converging. Refused upfront, naming the fix: a copied GET response's
+    keys (`name`, `enabled`, `enforcement_level`, `url` and `*_url`, at any depth), a status-check
+    requirement without `strict` or a check list, a review count outside 0-6, a quoted boolean,
+    and, on a literal branch, a `null` the PUT does not take.
+  BranchProtectionConfig.enforce_admins: >-
+    Enforce the protection for administrators too. `null` is the PUT's own spelling of off here: it
+    reads as `false` on a literal branch and a wildcard rule alike, and is sent as `false`.
+  BranchProtectionConfig.required_linear_history: >-
+    Require a linear commit history, so no merge commit can be pushed. An unquoted `true` or
+    `false`, like every boolean here; a literal branch refuses `null`.
+  BranchProtectionConfig.allow_force_pushes: >-
+    Allow force pushes by anyone with write access. `null` is the PUT's own spelling of off here: it
+    reads as `false` on a literal branch and a wildcard rule alike, and is sent as `false`.
+  BranchProtectionConfig.allow_deletions: Allow anyone with write access to delete the branch.
+  BranchProtectionConfig.block_creations: >-
+    With `restrictions`, also block a push that creates a matching branch unless its actor may push.
+  BranchProtectionConfig.required_conversation_resolution: Require every review conversation to be resolved before merging.
+  BranchProtectionConfig.lock_branch: Make the branch read-only, so nobody can push to it.
+  BranchProtectionConfig.allow_fork_syncing: Whether a locked branch still lets users pull changes from upstream.
   BranchProtectionConfig.required_status_checks: >-
     Require status checks to pass before merging; `null` turns the requirement off. A mapping
     carries `strict` and one of `contexts` or `checks`, since GitHub's protection PUT rejects the
@@ -208,9 +238,12 @@ schema:
     GitHub's read-back spelling of the same, is sent as `-1`); an omitted id pins whichever App
     reported it last.
   BranchProtectionConfig.required_pull_request_reviews: >-
-    Require pull request reviews before merging; `null` turns the requirement off. Every review
-    setting beside the count and the two actor holders (`dismiss_stale_reviews`,
-    `require_code_owner_reviews`, ...) passes through to the PUT.
+    Require pull request reviews before merging; `null` turns the requirement off. A key beside the
+    ones described here passes through to the PUT.
+  BranchProtectionConfig.required_pull_request_reviews|0.dismiss_stale_reviews: Dismiss approving reviews when someone pushes a new commit.
+  BranchProtectionConfig.required_pull_request_reviews|0.require_code_owner_reviews: Require a review from a code owner before merging.
+  BranchProtectionConfig.required_pull_request_reviews|0.require_last_push_approval: >-
+    Require the most recent push to be approved by someone other than the person who pushed it.
   BranchProtectionConfig.required_pull_request_reviews|0.required_approving_review_count: >-
     Approving reviews required before merging: 1 to 6, or 0 to require no approvals. GitHub accepts
     no other value, so the file is refused upfront instead of the PUT failing at apply.

docs/upgrading/v2-to-v3/refusals-lists.md

@@ -38,7 +38,7 @@ v2   branches:
 
 v3   branches[0].protection.required_status_checks.enforcement_level: ... is GitHub's GET-only echo, which the protection PUT has no word for; remove it (strict and the check list carry the requirement)
      branches[0].protection.url: ... is a link GitHub's GET response carries and the protection PUT has no word for; remove it
-     branches[0].protection.enforce_admins.enabled: ... is GitHub's GET wrapper around the toggle, which the protection PUT takes as a bare boolean; declare enforce_admins: true instead
+     branches[0].protection.enforce_admins: ... is GitHub's GET wrapper around the toggle, which the protection PUT takes as a bare boolean; declare enforce_admins: true instead
      branches[0].protection.restrictions.users[0]: ... carries an actor object copied from GitHub's GET response, which the protection PUT takes as the login string; write "octocat" instead
      (exit 1, zero requests)
 ```

src/sections/branches/endpoints.ts

@@ -21,12 +21,13 @@ export const ENDPOINTS = {
     statuses: { 200: "protection replaced" },
     // Reached for a missing branch only when the advisory probe below was denied.
     rejections: [MISSING_BRANCH],
+    // A malformed shape is refused when the file is parsed, so the hint names causes the parser does not refuse.
     hints: {
       422:
-        'Usually a sub-object is missing a required half: "required_status_checks" needs both ' +
-        '"strict" and "contexts", "required_pull_request_reviews" values must fit their ' +
-        'documented shapes, and "restrictions" needs "users" and "teams" lists (or declare the ' +
-        "whole key as null)",
+        "Usually one of the causes the settings file's own checks do not refuse: users or teams " +
+        'under "restrictions" or "dismissal_restrictions" on a repository a personal account ' +
+        "owns, more than 100 actors across the actor lists, or " +
+        '"required_linear_history" where the repository allows neither squash nor rebase merging',
     },
   },
   removeProtection: {

src/sections/branches/graphql-vocabulary.ts

@@ -57,7 +57,8 @@ export type ExplicitKeys<T> = keyof {
 export type RestCarriedKey =
   | "required_status_checks"
   | "required_pull_request_reviews"
-  | "restrictions";
+  | "restrictions"
+  | Exclude<BooleanControl, "required_signatures">;
 
 type _RoutedKeysCoverSchema = MustBeNever<
   Exclude<ExplicitKeys<BranchProtectionConfig>, RoutedKey | RestCarriedKey | "required_signatures">

src/sections/branches/index.ts

@@ -17,7 +17,7 @@ import { matchesRejection } from "../contract/endpoints.js";
 import { type SectionFailure, sectionFailure } from "../contract/errors.js";
 import { identifiedBy } from "../contract/keyed-list.js";
 import { liveByIdentity, liveIdentity } from "../contract/live.js";
-import type { SectionMeta, SectionModule } from "../contract/module.js";
+import type { SectionModule } from "../contract/module.js";
 import type { SectionPermission } from "../contract/permissions.js";
 import type { PlainData, Read } from "../contract/plan.js";
 import { layeredList, routed, rule } from "../shared/schema-helpers.js";
@@ -51,13 +51,14 @@ import {
   planWildcardEntry,
   resolveActorIds,
 } from "./graphql-writes.js";
-import { BOOLEAN_CONTROL_SET, isGetOnlyKey } from "./keys.js";
+import { isGetOnlyKey, NULLABLE_CONTROLS } from "./keys.js";
 import {
   type BranchConfig,
   BranchesConfig,
   BranchProtectionConfig,
   copiedActorName,
   PROTECTION_MAPPING_KEYS,
+  WILDCARD_NAME,
 } from "./schema.js";
 
 /** The classic PUT rejects a payload missing any of these; null is a valid value for each. */
@@ -72,11 +73,7 @@ export const REQUIRED_PROTECTION_KEYS = [
 const PROTECTION_VOCABULARY: ReadonlyMap<string, ReadonlySet<string>> = new Map(
   Object.entries({
     ...PROTECTION_MAPPING_KEYS,
-    "": [...Object.keys(BranchProtectionConfig.shape), ...BOOLEAN_CONTROL_SET],
-    required_pull_request_reviews: [
-      ...PROTECTION_MAPPING_KEYS.required_pull_request_reviews,
-      ...Object.keys(GRAPHQL_REVIEW_TWINS),
-    ],
+    "": Object.keys(BranchProtectionConfig.shape),
   }).map(([holder, keys]) => [holder, new Set(keys)]),
 );
 
@@ -143,9 +140,9 @@ function undocumentedPhantoms(declared: Record<string, unknown>, live: unknown):
   });
 }
 
-/** Git refnames forbid `*`, `?`, and `[`, so a wildcard entry can never collide with a literal branch. */
+/** Whether an entry's name is a wildcard pattern, which only the rule mutations reach. */
 export function isWildcardPattern(name: string): boolean {
-  return /[*?[]/.test(name);
+  return WILDCARD_NAME.test(name);
 }
 
 // GitHub spells this one list two ways inside required_status_checks and the GET returns both, so a declaration carrying either covers the other.
@@ -506,7 +503,6 @@ function replacingPutDrift(
  */
 async function planProtectionPut(
   ctx: BranchesContext,
-  section: SectionMeta,
   plan: BranchesPlan,
   put: ProtectionPut,
   probe: ProtectionProbe,
@@ -520,9 +516,7 @@ async function planProtectionPut(
       const branchProbe = yield* ctx.read.branchProbe.tryCall(z.unknown(), { params });
       if ("error" in branchProbe && matchesRejection(MISSING_BRANCH, branchProbe.error)) {
         // The same failure the PUT raises without the Contents grant, so the outcome does not depend on it.
-        return err(
-          sectionFailure("refused", `${section.key}: branches[${name}]: ${MISSING_BRANCH.advice}`),
-        );
+        return err(sectionFailure("refused", `branches[${name}]: ${MISSING_BRANCH.advice}`));
       }
       noteUndocumentedKeys(plan, prefix, payload, {});
       plan.ops.push({
@@ -602,7 +596,6 @@ function planSignatureToggle(plan: BranchesPlan, toggle: SignatureToggle): void
 /** A literal entry's operations in order: the PUT (or the removal), the signatures toggle, the routed update. */
 async function planLiteralEntry(
   ctx: BranchesContext,
-  section: SectionMeta,
   entry: Exclude<ClassifiedEntry, { kind: "wildcard" }>,
   plan: BranchesPlan,
 ): Promise<Result<void, SectionFailure>> {
@@ -635,7 +628,7 @@ async function planLiteralEntry(
       payload: restPayload(branch.protection),
       declared: branch.protection,
     };
-    const { live, putPlanned } = yield* await planProtectionPut(ctx, section, plan, put, probe);
+    const { live, putPlanned } = yield* await planProtectionPut(ctx, plan, put, probe);
     const requiredSignatures = branch.protection.required_signatures;
     // An undeclared toggle leaves the live requirement alone.
     if (requiredSignatures !== undefined) {
@@ -660,6 +653,27 @@ async function planLiteralEntry(
   });
 }
 
+/**
+ * A declared protection as every surface reads it: null under a control whose PUT spelling of off it is
+ * (keys.ts NULLABLE_CONTROLS) is that control off. GitHub's GET answers `false` there and the rule mutation
+ * has no null for off, so the one reading is `false`; the settings file keeps what its author wrote.
+ */
+function nullReadAsOff(
+  protection: PlainTyped<SplitProtection> | null,
+): PlainTyped<SplitProtection> | null {
+  if (protection === null) {
+    return null;
+  }
+
+  const read = { ...protection };
+  for (const control of NULLABLE_CONTROLS) {
+    if (read[control] === null) {
+      read[control] = false;
+    }
+  }
+  return read;
+}
+
 /**
  * The entries by the surface they apply through. The first entry that needs the GraphQL surface
  * starts the one rules read, ahead of every REST probe; a pure-REST declaration never starts it,
@@ -673,10 +687,10 @@ async function classifyEntries(
     let graphqlRun: GraphqlRun | null = null;
     const entries: ClassifiedEntry[] = [];
     for (const branch of branches) {
-      const protection: PlainTyped<SplitProtection> | null = branch.protection;
+      const protection = nullReadAsOff(branch.protection);
       if (isWildcardPattern(branch.name)) {
         graphqlRun ??= yield* startGraphqlRun(ctx);
-        entries.push({ kind: "wildcard", branch, graphqlRun });
+        entries.push({ kind: "wildcard", branch: { name: branch.name, protection }, graphqlRun });
       } else if (hasRoutedGraphqlKeys(protection)) {
         graphqlRun ??= yield* startGraphqlRun(ctx);
         entries.push({ kind: "routed", branch: { name: branch.name, protection }, graphqlRun });
@@ -825,7 +839,6 @@ export const branchesSection = {
     }),
   ),
   async plan(ctx, desired): Promise<Result<BranchesPlan, SectionFailure>> {
-    const section = this;
     return safeTry(async function* () {
       const branches = listEntries(desired);
       const plan: BranchesPlan = { ops: [], notes: [], drift: [] };
@@ -838,7 +851,7 @@ export const branchesSection = {
           await planWildcardEntry(ctx, entry.graphqlRun, entry.branch, plan);
           continue;
         }
-        yield* await planLiteralEntry(ctx, section, entry, plan);
+        yield* await planLiteralEntry(ctx, entry, plan);
       }
       if (graphqlRun !== null) {
         plan.ops = withActorsResolvedFirst(ctx, graphqlRun, plan.ops);

src/sections/branches/keys.ts

@@ -7,7 +7,7 @@
 /** The echoes by name; the links (url, *_url) match by suffix in isUrlKey. */
 export const GET_ONLY_KEYS: ReadonlySet<string> = new Set(["name", "enabled", "enforcement_level"]);
 
-// The controls the PUT takes as a bare boolean and the GET wraps as {url, enabled}; graphql-vocabulary.ts pins its twin table to this list, so a control added there is added here first.
+// The controls the PUT takes as a bare boolean and the GET wraps as {url, enabled}; graphql-vocabulary.ts pins its twin table to this list and schema.ts its typed controls, so a control is added here first.
 const BOOLEAN_CONTROLS = [
   "enforce_admins",
   "required_linear_history",