diff --git a/docs/reference/coverage.md b/docs/reference/coverage.md index 654b8fb1..efbdc389 100644 --- a/docs/reference/coverage.md +++ b/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. diff --git a/docs/sections/branches.docs.yml b/docs/sections/branches.docs.yml index b09b1598..71a058cf 100644 --- a/docs/sections/branches.docs.yml +++ b/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. diff --git a/docs/upgrading/v2-to-v3/refusals-lists.md b/docs/upgrading/v2-to-v3/refusals-lists.md index 36b1d9ec..9a2c8c3b 100644 --- a/docs/upgrading/v2-to-v3/refusals-lists.md +++ b/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) ``` diff --git a/src/sections/branches/endpoints.ts b/src/sections/branches/endpoints.ts index 488bd2ad..db7f8400 100644 --- a/src/sections/branches/endpoints.ts +++ b/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: { diff --git a/src/sections/branches/graphql-vocabulary.ts b/src/sections/branches/graphql-vocabulary.ts index d238a67e..8c5efdb1 100644 --- a/src/sections/branches/graphql-vocabulary.ts +++ b/src/sections/branches/graphql-vocabulary.ts @@ -57,7 +57,8 @@ export type ExplicitKeys = keyof { export type RestCarriedKey = | "required_status_checks" | "required_pull_request_reviews" - | "restrictions"; + | "restrictions" + | Exclude; type _RoutedKeysCoverSchema = MustBeNever< Exclude, RoutedKey | RestCarriedKey | "required_signatures"> diff --git a/src/sections/branches/index.ts b/src/sections/branches/index.ts index d0c6237d..317a8684 100644 --- a/src/sections/branches/index.ts +++ b/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> = 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, 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, plan: BranchesPlan, ): Promise> { @@ -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 | null, +): PlainTyped | 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 | 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> { - 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); diff --git a/src/sections/branches/keys.ts b/src/sections/branches/keys.ts index f5fc4034..9fda23f1 100644 --- a/src/sections/branches/keys.ts +++ b/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 = 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", diff --git a/src/sections/branches/schema.ts b/src/sections/branches/schema.ts index 1b059043..49046c3f 100644 --- a/src/sections/branches/schema.ts +++ b/src/sections/branches/schema.ts @@ -3,8 +3,8 @@ import { z } from "zod"; import { isPlainObject } from "../../plain-data.js"; import { stringItems } from "../shared/raw-values.js"; -import { open, rule } from "../shared/schema-helpers.js"; -import { BOOLEAN_CONTROL_SET, isGetOnlyKey, isUrlKey } from "./keys.js"; +import { conditional, open, rule } from "../shared/schema-helpers.js"; +import { type BooleanControl, isGetOnlyKey, isUrlKey, NULLABLE_CONTROLS } from "./keys.js"; export type BypassActor = | { kind: "user"; login: string } @@ -186,6 +186,63 @@ const RequiredStatusChecks = z anyOf: [{ required: ["contexts"] }, { required: ["checks"] }], }); +/** The one wording for a toggle that is not a boolean; `site` is its path under protection. */ +function toggleError(site: string): string { + return `${site} must be an unquoted true or false (YAML parses "no"/"off"/"yes" as strings, not booleans), so the toggle direction is unambiguous`; +} + +function toggle(site: string) { + return z.boolean({ error: toggleError(site) }); +} + +/** The controls the protection PUT carries; required_signatures rides its own sub-endpoint. */ +type PutControl = Exclude; + +/** The GET wraps each of these as {url, enabled}, so a copied wrapper is refused naming the boolean to declare. */ +function control(key: PutControl) { + return z.boolean({ + error: (issue) => { + const enabled = isPlainObject(issue.input) ? issue.input.enabled : undefined; + return typeof enabled === "boolean" + ? `protection.${key} is GitHub's GET wrapper around the toggle, which the protection PUT takes as a bare boolean; declare ${key}: ${enabled} instead` + : toggleError(key); + }, + }); +} + +/** The booleans of the PUT body; each takes null here, since the one shape serves both kinds of entry (refuseNullsThePutRefuses states the two contracts). */ +const PutControls = { + enforce_admins: control("enforce_admins").nullable().optional(), + required_linear_history: control("required_linear_history").nullable().optional(), + allow_force_pushes: control("allow_force_pushes").nullable().optional(), + allow_deletions: control("allow_deletions").nullable().optional(), + block_creations: control("block_creations").nullable().optional(), + required_conversation_resolution: control("required_conversation_resolution") + .nullable() + .optional(), + lock_branch: control("lock_branch").nullable().optional(), + allow_fork_syncing: control("allow_fork_syncing").nullable().optional(), +} satisfies Record; + +/** The controls of the block above that the PUT answers 422 to a null under: all but the two it marks nullable. */ +const NON_NULLABLE_CONTROLS = Object.keys(PutControls).filter((key) => !NULLABLE_CONTROLS.has(key)); + +/** The review settings the PUT takes as a bare boolean, under the same two contracts; the GET serves them bare too. */ +const ReviewToggles = { + dismiss_stale_reviews: toggle("required_pull_request_reviews.dismiss_stale_reviews") + .nullable() + .optional(), + require_code_owner_reviews: toggle("required_pull_request_reviews.require_code_owner_reviews") + .nullable() + .optional(), + require_last_push_approval: toggle("required_pull_request_reviews.require_last_push_approval") + .nullable() + .optional(), +}; + +/** The keys of the block above; the PUT answers 422 to a null under each. */ +const REVIEW_TOGGLES = Object.keys(ReviewToggles); + const RequiredPullRequestReviews = z.looseObject( { required_approving_review_count: z @@ -199,6 +256,7 @@ const RequiredPullRequestReviews = z.looseObject( bypass_pull_request_allowances: reviewActorHolder( "required_pull_request_reviews.bypass_pull_request_allowances", ).optional(), + ...ReviewToggles, }, { error: @@ -212,22 +270,9 @@ const ECHO_CARRIER: Readonly> = { enforcement_level: "strict and the check list carry the requirement", }; -/** - * The fix for each GET-only key a copied GET response carries. The bare-boolean advice is offered - * only under a control the PUT takes as a boolean: under a mapping-valued one such as - * required_pull_request_reviews, "declare required_pull_request_reviews: true" would itself be refused. - */ -function getOnlyKeyError(path: readonly (string | number)[], key: string, value: unknown): string { +/** The fix for a GET-only key; a wrapper under a control never reaches here, since the control's own type refuses it (control(), above). */ +function getOnlyKeyError(path: readonly (string | number)[], key: string): string { const site = ["protection", ...path, key].join("."); - const parent = path.at(-1); - if ( - key === "enabled" && - typeof parent === "string" && - BOOLEAN_CONTROL_SET.has(parent) && - typeof value === "boolean" - ) { - return `${site} is GitHub's GET wrapper around the toggle, which the protection PUT takes as a bare boolean; declare ${parent}: ${value} instead`; - } if (isUrlKey(key)) { return `${site} is a link GitHub's GET response carries and the protection PUT has no word for; remove it`; } @@ -262,7 +307,7 @@ function refuseGetOnlyKeys( refineCtx.addIssue({ code: "custom", path: [...path, key], - message: getOnlyKeyError(path, key, inner), + message: getOnlyKeyError(path, key), }); continue; } @@ -272,7 +317,47 @@ function refuseGetOnlyKeys( ancestors.delete(value); } -/** The schema's keys under each open protection mapping, by dotted path; index.ts adds the pass-through controls to complete the PUT vocabulary and notes a declared key outside it that the GET never echoes. */ +/** zod's own issue for a null where a boolean goes, so validation words the refusal like any null with no empty state. */ +const NULL_FOR_BOOLEAN = { code: "invalid_type", expected: "boolean", input: null } as const; + +/** + * GitHub states two contracts for a null under a boolean of the PUT body. One shape serves both kinds of entry, so + * the shape takes the null and this refuses it on a literal entry: + * + * the protection PUT (a literal entry) -> 422, except under the two controls it marks nullable, where null is off + * the rule mutation (a wildcard entry) -> every twin is a nullable Boolean input, so GitHub can accept the null + */ +function refuseNullsThePutRefuses( + protection: Record, + refineCtx: z.RefinementCtx, +): void { + for (const key of NON_NULLABLE_CONTROLS) { + if (protection[key] === null) { + refineCtx.addIssue({ ...NULL_FOR_BOOLEAN, path: ["protection", key] }); + } + } + + const reviews = protection.required_pull_request_reviews; + if (!isPlainObject(reviews)) { + return; + } + + for (const key of REVIEW_TOGGLES) { + if (reviews[key] === null) { + refineCtx.addIssue({ + ...NULL_FOR_BOOLEAN, + path: ["protection", "required_pull_request_reviews", key], + }); + } + } +} + +/** A JSON Schema `properties` map typing each key a boolean. */ +function booleans(keys: readonly string[]): Record { + return Object.fromEntries(keys.map((key) => [key, { type: "boolean" }])); +} + +/** The schema's keys under each open protection mapping, by dotted path: the PUT's vocabulary there. index.ts notes a declared key outside it that the GET never echoes. */ export const PROTECTION_MAPPING_KEYS = { required_status_checks: Object.keys(RequiredStatusChecks.shape), required_pull_request_reviews: Object.keys(RequiredPullRequestReviews.shape), @@ -287,12 +372,8 @@ export const BranchProtectionConfig = z required_status_checks: RequiredStatusChecks.nullable().optional(), required_pull_request_reviews: RequiredPullRequestReviews.nullable().optional(), restrictions: Restrictions.nullable().optional(), - required_signatures: z - .boolean({ - error: - 'required_signatures must be an unquoted true or false (YAML parses "no"/"off"/"yes" as strings, not booleans), so the toggle direction is unambiguous', - }) - .optional(), + ...PutControls, + required_signatures: toggle("required_signatures").optional(), force_push_bypassers: z .array( z.string().check( @@ -318,6 +399,9 @@ export const BranchProtectionConfig = z export type BranchProtectionConfig = z.infer; +/** What makes a name a wildcard pattern: git refnames forbid `*`, `?`, and `[`, so one can never collide with a literal branch. */ +export const WILDCARD_NAME = /[*?[]/; + export const BranchConfig = open({ name: z.string(), protection: BranchProtectionConfig.nullable(), @@ -348,7 +432,32 @@ export const BranchConfig = open({ } }), ) - .meta({ id: "BranchConfig" }); + .check( + rule((entry, refineCtx) => { + // The name or the protection may be raw beside its own shape issue; a regex would coerce a raw name. + const literal = typeof entry.name === "string" && !WILDCARD_NAME.test(entry.name); + if (literal && isPlainObject(entry.protection)) { + refuseNullsThePutRefuses(entry.protection, refineCtx); + } + }), + ) + // The conditional is the null refusal's published twin, read from the same two lists: a refinement does not reach z.toJSONSchema. + .meta({ + id: "BranchConfig", + ...conditional( + { properties: { name: { not: { pattern: WILDCARD_NAME.source } } } }, + { + properties: { + protection: { + properties: { + ...booleans(NON_NULLABLE_CONTROLS), + required_pull_request_reviews: { properties: booleans(REVIEW_TOGGLES) }, + }, + }, + }, + }, + ), + }); export type BranchConfig = z.infer; export const BranchesConfig = z.array(BranchConfig); diff --git a/test/schema-corpus.test.ts b/test/schema-corpus.test.ts index 6d1723b0..540306af 100644 --- a/test/schema-corpus.test.ts +++ b/test/schema-corpus.test.ts @@ -181,8 +181,6 @@ const KNOWN_DIVERGENCES: Record = { "secret-scanning-patterns-uncompilable-regex-rejected.yml settings": "schema-looser: the regex fields' syntax check is a superRefine that first translates the PCRE-only forms Hyperscan accepts (compilable-form.ts), which no JSON Schema keyword does; " + "`format: regex` would be a flagless new RegExp that also rejects a `\\Z` anchor after another character, refusing a delimiter Hyperscan holds, so the fields stay plain strings in the schema", - "branches-get-response-copied-rejected.yml settings": - "schema-looser: the GET-only key sweep is a recursive superRefine over the passthrough protection mapping (branches/schema.ts), which JSON Schema cannot express; the published schema types the two structured controls but forbids no key", "repository-get-only-key-rejected.yml settings": "schema-looser: the GET-only key refusal is a superRefine over the repository passthrough mapping, which stays open in the schema for the PATCH fields GitHub adds later", "code-scanning-default-setup-get-only-key-rejected.yml settings": diff --git a/test/sections/branches/branches.test.ts b/test/sections/branches/branches.test.ts index c9649c24..9116c308 100644 --- a/test/sections/branches/branches.test.ts +++ b/test/sections/branches/branches.test.ts @@ -225,9 +225,8 @@ describe("branches", () => { const { status, message, advice } = MISSING_BRANCH; // The unrouted protection GET 404s; a Contents-granted probe answers GitHub's missing-branch body. const probed = new MockApi({ [PROBE]: { error: { status, message, body: "" } } }); - await expect(plan(probed, declared)).rejects.toThrow( - new Error(`branches: branches[main]: ${advice}`), - ); + // The engine prefixes the section key once, so the section's own line leads with the entry alone. + await expect(plan(probed, declared)).rejects.toThrow(new Error(`branches[main]: ${advice}`)); expect(probed.mutations()).toHaveLength(0); // A denied probe is a concealed 404, so the PUT is planned and GitHub answers it with the same body: a // hard failure under every on-missing-permission policy, never a denial carrying grant advice. @@ -789,6 +788,40 @@ describe("branches", () => { expect(changes).toEqual(['applied protection to "main"']); expect(second).toEqual({ ops: [], notes: [], drift: [] }); }); + + test("null on the two controls whose PUT spelling of off it is reads as off: clean against GitHub's {enabled: false}, and a control that is on diffs as false", async () => { + // GitHub's GET serves an off control as {enabled: false}; compared as "empty", a declared null drifted against it on every run. + const nullAsOff: Desired = [ + { name: "main", protection: { enforce_admins: null, allow_force_pushes: null } }, + ]; + const off = new MockApi({ + [PROTECTION]: { + data: { enforce_admins: { enabled: false }, allow_force_pushes: { enabled: false } }, + }, + }); + expect(await plan(off, nullAsOff)).toEqual({ ops: [], notes: [], drift: [] }); + const on = new MockApi({ + [PROTECTION]: { + data: { enforce_admins: { enabled: true }, allow_force_pushes: { enabled: false } }, + }, + }); + expect((await plan(on, nullAsOff)).ops).toEqual([ + { + role: "putProtection", + params: MAIN, + payload: { + enforce_admins: false, + allow_force_pushes: false, + required_status_checks: null, + required_pull_request_reviews: null, + restrictions: null, + }, + describe: 'replacing protection for branch "main"', + drift: ["branches[main].protection.enforce_admins: false != true"], + change: 'applied protection to "main"', + }, + ]); + }); }); /** A required-deployment list naming an environment no live state seeds. */ @@ -1346,6 +1379,39 @@ describe("branches wildcard entries", () => { await plan(api, [{ name: "main", protection: { enforce_admins: true } }]); expect(api.calls.filter((c) => c.method === "GRAPHQL")).toHaveLength(0); }); + + test("null on enforce_admins or allow_force_pushes turns a wildcard rule's control off: the mutation sends false, and the re-plan over the applied rule is empty", async () => { + // The rule's twins are non-null Booleans: a null sent as the off value read back as no boolean at all and failed the next rules read. + const api = liveRepo({ + branch_protection_rules: [ + { pattern: "release/*", isAdminEnforced: true, allowsForcePushes: true }, + ], + }); + const { first, second, changes } = await provePlanIdempotent(branchesSection, api, [ + { name: "release/*", protection: { enforce_admins: null, allow_force_pushes: null } }, + ]); + expect(first.ops.map((op) => op.drift)).toEqual([ + [ + "branches[release/*].protection.enforce_admins: false != true", + "branches[release/*].protection.allow_force_pushes: false != true", + ], + ]); + expect(api.writes).toEqual([ + { + method: "GRAPHQL", + path: "UpdateBranchProtectionRule", + payload: { + input: { + branchProtectionRuleId: expect.any(String), + isAdminEnforced: false, + allowsForcePushes: false, + }, + }, + }, + ]); + expect(changes).toEqual(['updated protection rule "release/*"']); + expect(second).toEqual({ ops: [], notes: [], drift: [] }); + }); }); describe("branches plan contract", () => { diff --git a/test/sections/branches/scenarios/branches-get-response-copied-rejected.yml b/test/sections/branches/scenarios/branches-get-response-copied-rejected.yml index 2fde64cb..cf3d0643 100644 --- a/test/sections/branches/scenarios/branches-get-response-copied-rejected.yml +++ b/test/sections/branches/scenarios/branches-get-response-copied-rejected.yml @@ -27,6 +27,5 @@ expect: zero_requests: true stdout_contains: - "branches[0].protection.url: protection.url is a link" - - "branches[0].protection.enforce_admins.enabled:" - - "declare enforce_admins: true instead" + - "branches[0].protection.enforce_admins: 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.required_status_checks.enforcement_level:" diff --git a/test/sections/branches/scenarios/branches-literal-null-control-rejected.yml b/test/sections/branches/scenarios/branches-literal-null-control-rejected.yml new file mode 100644 index 00000000..4809bedf --- /dev/null +++ b/test/sections/branches/scenarios/branches-literal-null-control-rejected.yml @@ -0,0 +1,21 @@ +# The settings hold nothing but a null on a control GitHub's protection PUT +# does not mark nullable, on a literal branch: the PUT answers 422 to it. The +# published schema and the runtime both refuse the file, so this is the +# document that would show the two disagreeing on the literal-only rule. +name: branches-literal-null-control-rejected +settings: + branches: + - name: main + protection: + allow_deletions: null +live_state: + branches: ["main"] +token_permissions: + administration: write + contents: read +expect: + exit_code: 1 + result: failed + zero_requests: true + stdout_contains: + - "branches[0].protection.allow_deletions has no empty state; write true or false" diff --git a/test/sections/branches/scenarios/branches-missing-branch-advisory-apply-fails.yml b/test/sections/branches/scenarios/branches-missing-branch-advisory-apply-fails.yml index d76af59a..f47d6891 100644 --- a/test/sections/branches/scenarios/branches-missing-branch-advisory-apply-fails.yml +++ b/test/sections/branches/scenarios/branches-missing-branch-advisory-apply-fails.yml @@ -23,5 +23,7 @@ expect: branches: failed summary_contains: - "branches[release]: the declared branch does not exist on the repo, so its protection cannot be applied; create the branch, or remove it from the settings file" + stdout_contains: + - "::error::branches[release]: the declared branch does not exist on the repo" never: - "PUT /repos/{repo}/branches/release/protection" diff --git a/test/sections/branches/scenarios/branches-non-boolean-control-rejected.yml b/test/sections/branches/scenarios/branches-non-boolean-control-rejected.yml new file mode 100644 index 00000000..865f9d09 --- /dev/null +++ b/test/sections/branches/scenarios/branches-non-boolean-control-rejected.yml @@ -0,0 +1,32 @@ +# GitHub's protection PUT types every control and review setting as a +# boolean, and the rule mutation's Boolean input cannot take a string, so a +# quoted value failed at apply: a 422 on a literal branch, a rejected +# mutation on a wildcard rule. A null answered 422 on a literal branch +# outside enforce_admins and allow_force_pushes. Each is refused before any +# request, naming the key. +name: branches-non-boolean-control-rejected +settings: + branches: + - name: main + protection: + required_linear_history: "yes" + allow_deletions: null + required_pull_request_reviews: + dismiss_stale_reviews: "no" + - name: "release/*" + protection: + lock_branch: "off" +live_state: + branches: ["main"] +token_permissions: + administration: write + contents: read +expect: + exit_code: 1 + result: failed + zero_requests: true + stdout_contains: + - "branches[0].protection.required_pull_request_reviews.dismiss_stale_reviews: required_pull_request_reviews.dismiss_stale_reviews must be an unquoted true or false" + - 'branches[0].protection.required_linear_history: required_linear_history must be an unquoted true or false (YAML parses "no"/"off"/"yes" as strings, not booleans), so the toggle direction is unambiguous' + - "branches[0].protection.allow_deletions has no empty state; write true or false" + - "branches[1].protection.lock_branch: lock_branch must be an unquoted true or false" diff --git a/test/sections/branches/scenarios/branches-null-off-control-converges.yml b/test/sections/branches/scenarios/branches-null-off-control-converges.yml new file mode 100644 index 00000000..19eb6c16 --- /dev/null +++ b/test/sections/branches/scenarios/branches-null-off-control-converges.yml @@ -0,0 +1,39 @@ +# null is the protection PUT's own spelling of off under enforce_admins and +# allow_force_pushes, so it reads as off on both surfaces. A literal branch +# already off the way GitHub's GET serves it ({enabled: false}) plans no PUT. +# A wildcard rule's mutation is sent false: it has no null for off, and a +# stored null failed the next rules read. +name: branches-null-off-control-converges +settings: + branches: + - name: main + protection: + enforce_admins: null + allow_force_pushes: null + - name: "release/*" + protection: + enforce_admins: null +live_state: + branches: ["main"] + branch_protection: + main: + enforce_admins: + enabled: false + allow_force_pushes: + enabled: false + branch_protection_rules: + - pattern: "release/*" + isAdminEnforced: true +token_permissions: + administration: write + contents: read +expect: + exit_code: 0 + result: applied + outcomes: + branches: applied + mutations: + - "GRAPHQL UpdateBranchProtectionRule" + never: + - "PUT /repos/{repo}/branches" + fixpoint: apply_idempotent diff --git a/test/sections/branches/schema.test.ts b/test/sections/branches/schema.test.ts index d2e37566..ba6a4974 100644 --- a/test/sections/branches/schema.test.ts +++ b/test/sections/branches/schema.test.ts @@ -1,5 +1,9 @@ import { describe, expect, test } from "bun:test"; +import { Ajv } from "ajv"; +import { validateSectionShapes } from "../../../src/engine/validate.js"; import { branchesSection, protectionSnapshot } from "../../../src/sections/branches/index.js"; +import { loadSpec } from "../../e2e/openapi/validate.js"; +import { readSettingsSchema } from "../../settings-schema.js"; /** Every issue as "path: message", so a case pins the whole verdict and not one line of it. */ function issues(entries: unknown[]): string[] { @@ -9,6 +13,58 @@ function issues(entries: unknown[]): string[] { : parsed.error.issues.map((issue) => `${issue.path.join(".")}: ${issue.message}`); } +/** The file's problems as a run prints them, a refused null reworded to the values that exist. */ +function problems(entries: unknown[]): readonly string[] { + const verdict = validateSectionShapes({ branches: entries }, "fixture"); + return verdict.isErr() ? verdict.error.issues : []; +} + +/** The built lib/settings.schema.json as an editor judges a file against it; strict off for its draft-07 idioms. */ +const published = new Ajv({ strict: false, allErrors: true }).compile(readSettingsSchema()); + +/** The published schema's verdict on the same entries. */ +function publishedAccepts(entries: unknown[]): boolean { + return published({ branches: entries }) === true; +} + +/** A bare boolean of GitHub's protection PUT body: its dotted path under protection, and whether the PUT takes null there. */ +interface PutBoolean { + site: string; + nullable: boolean; +} + +interface DescriptorNode { + type?: unknown; + nullable?: boolean; + properties?: Record; +} + +function booleansUnder(node: DescriptorNode, path: string[]): PutBoolean[] { + return Object.entries(node.properties ?? {}).flatMap(([key, inner]) => + inner.type === "boolean" + ? [{ site: [...path, key].join("."), nullable: inner.nullable === true }] + : booleansUnder(inner, [...path, key]), + ); +} + +/** Read off GitHub's own descriptor, so a boolean the PUT gains meets the same rows; `strict` keeps its own, typed before under a wording that also covers its absence. */ +const PUT_BOOLEANS = booleansUnder( + loadSpec().paths["/repos/{owner}/{repo}/branches/{branch}/protection"]?.put?.requestBody + ?.content?.["application/json"]?.schema as DescriptorNode, + [], +).filter(({ site }) => site !== "required_status_checks.strict"); + +/** A protection declaring `value` at a dotted site. */ +function declaring(site: string, value: unknown): Record { + return site.split(".").reduceRight((inner, key) => ({ [key]: inner }), value) as Record< + string, + unknown + >; +} + +const QUOTED_ADVICE = + ' must be an unquoted true or false (YAML parses "no"/"off"/"yes" as strings, not booleans), so the toggle direction is unambiguous'; + const WILDCARD_KEYS = "enforce_admins, required_linear_history, allow_force_pushes, allow_deletions, " + "block_creations, required_conversation_resolution, lock_branch, allow_fork_syncing, " + @@ -197,6 +253,53 @@ describe("branches protection parse rules", () => { ); }); + test("the descriptor walk reaches the three kinds of boolean the rows below loop over", () => { + // The control: a walk that found nothing would pass every loop below without pinning anything. + expect(PUT_BOOLEANS).toContainEqual({ site: "enforce_admins", nullable: true }); + expect(PUT_BOOLEANS).toContainEqual({ site: "allow_deletions", nullable: false }); + expect(PUT_BOOLEANS).toContainEqual({ + site: "required_pull_request_reviews.dismiss_stale_reviews", + nullable: false, + }); + }); + + test.each( + PUT_BOOLEANS.flatMap(({ site }) => ["main", "release/*"].map((name) => ({ site, name }))), + )( + "a quoted $site is refused on $name, naming the YAML gotcha, and the published schema refuses it too: the PUT answers 422 to it and the rule mutation's Boolean input cannot take a string", + ({ site, name }) => { + const quoted = [{ name, protection: declaring(site, "yes") }]; + expect(problems(quoted)).toEqual([`branches[0].protection.${site}: ${site}${QUOTED_ADVICE}`]); + expect(publishedAccepts(quoted)).toBe(false); + for (const value of [true, false]) { + const bare = [{ name, protection: declaring(site, value) }]; + expect(problems(bare)).toEqual([]); + expect(publishedAccepts(bare)).toBe(true); + } + }, + ); + + test.each(PUT_BOOLEANS)( + "$site: null on a literal branch is refused unless the PUT marks the boolean nullable (nullable: $nullable), since the PUT answers 422 to it at apply", + ({ site, nullable }) => { + const entries = [{ name: "main", protection: declaring(site, null) }]; + expect(problems(entries)).toEqual( + nullable ? [] : [`branches[0].protection.${site} has no empty state; write true or false`], + ); + // The refusal reads the entry's name, which no type carries: the published schema holds it as a conditional, so an editor reaches the same verdict. + expect(publishedAccepts(entries)).toBe(nullable); + }, + ); + + test.each(PUT_BOOLEANS)( + "$site: null still passes on a wildcard rule, at parse and in the published schema: its mutation input types every twin as a nullable Boolean, so GitHub can accept the file's first apply", + ({ site }) => { + const entries = [{ name: "release/*", protection: declaring(site, null) }]; + expect(problems(entries)).toEqual([]); + expect(publishedAccepts(entries)).toBe(true); + }, + ); + // The GET expands each actor into an object; the PUT takes the login/slug string, so a copied // list reached the PUT as objects and 422d there. const holders = [ @@ -287,18 +390,20 @@ describe("branches protection parse rules", () => { "0.protection.url: protection.url is a link GitHub's GET response carries and the protection PUT has no word for; remove it", "0.protection.name: protection.name is GitHub's GET-only echo, which the protection PUT has no word for; remove it (the entry's name already names the branch)", "0.protection.enabled: protection.enabled is GitHub's GET-only echo, which the protection PUT has no word for; remove it (the control's own key carries the toggle)", - "0.protection.enforce_admins.url: protection.enforce_admins.url is a link GitHub's GET response carries and the protection PUT has no word for; remove it", - "0.protection.enforce_admins.enabled: 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", ]; + // The wrapper is a typed control's failure too, refused at the control with the boolean to declare; its url and enabled keys fold into that line. + const wrapper = + "0.protection.enforce_admins: 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"; expect(issues([{ name: "main", protection: getBody }])).toEqual([ '0.protection.restrictions.users.0: protection.restrictions.users carries an actor object copied from GitHub\'s GET response, which the protection PUT takes as the login string; write "octocat" instead', + wrapper, ...keySweep, ]); const stringActors = { ...getBody, restrictions: { ...getBody.restrictions, users: ["octocat"] }, }; - expect(issues([{ name: "main", protection: stringActors }])).toEqual(keySweep); + expect(issues([{ name: "main", protection: stringActors }])).toEqual([wrapper, ...keySweep]); expect(issues([{ name: "main", protection: protectionSnapshot(getBody) }])).toEqual([]); }); @@ -307,12 +412,17 @@ describe("branches protection parse rules", () => { const self: Record = { users: [], teams: [] }; self.restrictions = self; expect(issues([{ name: "main", protection: self }])).toEqual([]); - const wrapper = { enabled: true }; + // Shared between the two review-side holders, which the key walk still reaches (a wrapper under a typed control stops at the control). + const holder = { users: [], users_url: "https://api.github.com/teams/1/members" }; + const reviews = { dismissal_restrictions: holder, bypass_pull_request_allowances: holder }; expect( - issues([{ name: "main", protection: { enforce_admins: wrapper, lock_branch: wrapper } }]).map( + issues([{ name: "main", protection: { required_pull_request_reviews: reviews } }]).map( (issue) => issue.slice(0, issue.indexOf(":")), ), - ).toEqual(["0.protection.enforce_admins.enabled", "0.protection.lock_branch.enabled"]); + ).toEqual([ + "0.protection.required_pull_request_reviews.dismissal_restrictions.users_url", + "0.protection.required_pull_request_reviews.bypass_pull_request_allowances.users_url", + ]); }); test("every shape the PUT and the rule mutation take parses clean, off controls and edge values included", () => { diff --git a/test/sections/refusal-messages.test.ts b/test/sections/refusal-messages.test.ts index 66aa34b5..b2090438 100644 --- a/test/sections/refusal-messages.test.ts +++ b/test/sections/refusal-messages.test.ts @@ -131,6 +131,8 @@ const DATA_FUNCTIONS: ReadonlySet = new Set(["render", "codePointEscape" /** Helpers whose string arguments at these indexes are data (a key name, a rule type, a wrapper kind, a noun); a key that renders inside a message is data too, since a containment pin would hold wherever a test spells it. */ const DATA_ARGUMENTS: Readonly> = { actorList: [0, 1], + control: [0], + toggle: [0], reviewActorHolder: [0], rule: [0], sectionFailure: [0],