From dfbf6dd030f2ffdfea66921aadfe307ff2cace8e Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 21:54:28 -0500 Subject: [PATCH 1/3] fix(branches): refuse a non-boolean control at parse, and let a null "off" converge Every boolean of the protection PUT body is now typed in the schema: the eight controls and the three review settings. A quoted value such as "yes" is refused when the file is parsed, on a literal branch and on a wildcard rule alike, in the wording required_signatures already had. Before, the PUT answered 422 at apply, and a wildcard rule's mutation was sent the string. A null is refused on a literal branch wherever the PUT does not mark the boolean nullable, as "has no empty state; write true or false". A wildcard rule still takes that null: the rule mutation types every twin as a nullable Boolean, so GitHub can accept the file. Under enforce_admins and allow_force_pushes null is the PUT's own spelling of off, so the section now reads it as false in one place. A check against GitHub's {enabled: false} is clean, where it reported "expected empty, live has false" on every run. A wildcard rule's mutation is sent false, where a null it stored failed the next rules read. A copied {url, enabled} wrapper is refused at the control's own path with the same advice; the key sweep used to report it one step deeper, at .enabled. The published JSON schema types the new booleans, so the known-divergence entry for the copied-GET scenario is deleted: both validators refuse it now. The PUT's 422 hint names only what GitHub's descriptor says the PUT judges against the repository, since the parser refuses a malformed shape first. The missing-branch refusal no longer leads with the section key twice. --- docs/reference/coverage.md | 3 + docs/sections/branches.docs.yml | 47 ++++++- docs/upgrading/v2-to-v3/refusals-lists.md | 2 +- src/sections/branches/endpoints.ts | 10 +- src/sections/branches/graphql-vocabulary.ts | 3 +- src/sections/branches/index.ts | 89 +++++++++---- src/sections/branches/keys.ts | 2 +- src/sections/branches/schema.ts | 95 ++++++++++---- test/schema-corpus.test.ts | 2 - test/sections/branches/branches.test.ts | 72 ++++++++++- .../branches-get-response-copied-rejected.yml | 3 +- .../branches-non-boolean-control-rejected.yml | 32 +++++ .../branches-null-off-control-converges.yml | 39 ++++++ test/sections/branches/schema.test.ts | 120 +++++++++++++++++- test/sections/refusal-messages.test.ts | 2 + 15 files changed, 446 insertions(+), 75 deletions(-) create mode 100644 test/sections/branches/scenarios/branches-non-boolean-control-rejected.yml create mode 100644 test/sections/branches/scenarios/branches-null-off-control-converges.yml diff --git a/docs/reference/coverage.md b/docs/reference/coverage.md index b9148f1e1..9ce85e417 100644 --- a/docs/reference/coverage.md +++ b/docs/reference/coverage.md @@ -180,6 +180,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 b09b1598f..71a058cf9 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 36b1d9ec8..9a2c8c3b4 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 488bd2ade..aa70c43a0 100644 --- a/src/sections/branches/endpoints.ts +++ b/src/sections/branches/endpoints.ts @@ -21,12 +21,14 @@ 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 only what GitHub's + // descriptor says the PUT judges against the repository itself. 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 a state only GitHub can judge, since the file's shape is checked before any " + + 'request: 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 d238a67e3..8c5efdb19 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 d0c6237d1..1a9778d7e 100644 --- a/src/sections/branches/index.ts +++ b/src/sections/branches/index.ts @@ -17,9 +17,10 @@ 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 { valueAt } from "../shared/list/write.js"; import { layeredList, routed, rule } from "../shared/schema-helpers.js"; import { ENDPOINTS, MISSING_BRANCH } from "./endpoints.js"; import { @@ -51,13 +52,15 @@ 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, + PUT_CONTROLS, + REVIEW_TOGGLES, } from "./schema.js"; /** The classic PUT rejects a payload missing any of these; null is a valid value for each. */ @@ -72,14 +75,25 @@ 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)]), ); +/** + * The booleans a literal entry may not declare null, each as its steps under protection: GitHub's protection PUT + * answers 422 to a null it does not mark nullable. The shape takes the null, since a wildcard entry may carry it + * (schema.ts states the two contracts). + */ +const LITERAL_BOOLEANS_WITHOUT_NULL: readonly (readonly string[])[] = [ + ...PUT_CONTROLS.filter((key) => !NULLABLE_CONTROLS.has(key)).map((key) => [key]), + ...REVIEW_TOGGLES.map((key) => ["required_pull_request_reviews", key]), +]; + +/** The steps under protection of each null a literal entry declares where the protection PUT takes none. */ +function nullsThePutRefuses(protection: Record): (readonly string[])[] { + return LITERAL_BOOLEANS_WITHOUT_NULL.filter((steps) => valueAt(protection, steps) === null); +} + /** * Whether every step of a phantom's path names a key the PUT documents under its holder. The steps * are checked one at a time, so a literal key holding a dot never reads as documented nesting. @@ -506,7 +520,6 @@ function replacingPutDrift( */ async function planProtectionPut( ctx: BranchesContext, - section: SectionMeta, plan: BranchesPlan, put: ProtectionPut, probe: ProtectionProbe, @@ -520,9 +533,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 +613,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 +645,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 +670,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 +704,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 }); @@ -788,8 +819,9 @@ export const branchesSection = { permission, endpoints: ENDPOINTS, graphql: GRAPHQL, - // The wildcard key sweep composes HERE, not in schema.ts: it reads the GraphQL translation tables, - // which are this section's own machinery, and nothing outside them can reach a wildcard rule. + // The two per-surface rules compose HERE, not in schema.ts, which holds one shape for both kinds of entry: + // the wildcard key sweep reads the GraphQL translation tables, this section's own machinery, and the + // literal null refusal is the protection PUT's contract alone. shape: routed(layeredList(BranchesConfig)).check( rule((declared, refineCtx) => { // The routed shape parsed one of the two forms; under the wrapper an issue's path starts at `entries`. @@ -803,17 +835,27 @@ export const branchesSection = { entries.forEach((entry: BranchConfig, index) => { // The entry, its name, or its protection may be raw or missing beside its own shape issue; a regex would // coerce the name (`[object Object]` wildcards), and a non-mapping protection holds no keys to sweep. - if ( - !isPlainObject(entry) || - typeof entry.name !== "string" || - !isWildcardPattern(entry.name) - ) { + if (!isPlainObject(entry) || typeof entry.name !== "string") { return; } const protection = entry.protection; if (!isPlainObject(protection)) { return; } + + if (!isWildcardPattern(entry.name)) { + for (const steps of nullsThePutRefuses(protection)) { + // The type issue it is on this surface, so validation words it like any null with no empty state. + refineCtx.addIssue({ + code: "invalid_type", + expected: "boolean", + input: null, + path: at(index, "protection", ...steps), + }); + } + return; + } + for (const steps of wildcardKeysOutsideVocabulary(protection)) { refineCtx.addIssue({ code: "custom", @@ -825,7 +867,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 +879,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 f5fc40346..9fda23f10 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 1b0590433..71d996dae 100644 --- a/src/sections/branches/schema.ts +++ b/src/sections/branches/schema.ts @@ -4,7 +4,7 @@ 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 { type BooleanControl, isGetOnlyKey, isUrlKey } from "./keys.js"; export type BypassActor = | { kind: "user"; login: string } @@ -186,6 +186,69 @@ 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. GitHub states two contracts for a null under one, and this one shape serves both + * kinds of entry, so it takes the null and index.ts 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 + */ +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 keys of the block above, for the literal entry's null refusal (index.ts). */ +export const PUT_CONTROLS = Object.keys(PutControls); + +/** 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, for the same refusal. */ +export const REVIEW_TOGGLES = Object.keys(ReviewToggles); + const RequiredPullRequestReviews = z.looseObject( { required_approving_review_count: z @@ -199,6 +262,7 @@ const RequiredPullRequestReviews = z.looseObject( bypass_pull_request_allowances: reviewActorHolder( "required_pull_request_reviews.bypass_pull_request_allowances", ).optional(), + ...ReviewToggles, }, { error: @@ -212,22 +276,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 +313,7 @@ function refuseGetOnlyKeys( refineCtx.addIssue({ code: "custom", path: [...path, key], - message: getOnlyKeyError(path, key, inner), + message: getOnlyKeyError(path, key), }); continue; } @@ -272,7 +323,7 @@ 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. */ +/** 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 +338,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( diff --git a/test/schema-corpus.test.ts b/test/schema-corpus.test.ts index 6d1723b0c..540306afd 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 bfd5887e3..9b42f9699 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. */ @@ -1342,6 +1375,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 2fde64cbb..cf3d06435 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-non-boolean-control-rejected.yml b/test/sections/branches/scenarios/branches-non-boolean-control-rejected.yml new file mode 100644 index 000000000..865f9d095 --- /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 000000000..19eb6c166 --- /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 d2e375666..e25631ec9 100644 --- a/test/sections/branches/schema.test.ts +++ b/test/sections/branches/schema.test.ts @@ -1,5 +1,7 @@ import { describe, expect, test } from "bun:test"; +import { validateSectionShapes } from "../../../src/engine/validate.js"; import { branchesSection, protectionSnapshot } from "../../../src/sections/branches/index.js"; +import { loadSpec } from "../../e2e/openapi/validate.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 +11,53 @@ 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 : []; +} + +/** 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 is held to the same refusals. `strict` keeps its + * own rows: it was 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 +246,58 @@ 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: the PUT answers 422 to it and the rule mutation's Boolean input cannot take a string", + ({ site, name }) => { + expect(problems([{ name, protection: declaring(site, "yes") }])).toEqual([ + `branches[0].protection.${site}: ${site}${QUOTED_ADVICE}`, + ]); + expect(problems([{ name, protection: declaring(site, true) }])).toEqual([]); + expect(problems([{ name, protection: declaring(site, false) }])).toEqual([]); + }, + ); + + 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 }) => { + expect(problems([{ name: "main", protection: declaring(site, null) }])).toEqual( + nullable ? [] : [`branches[0].protection.${site} has no empty state; write true or false`], + ); + }, + ); + + test.each(PUT_BOOLEANS)( + "$site: null still passes on a wildcard rule, whose mutation input types every twin as a nullable Boolean, so GitHub can accept the file's first apply", + ({ site }) => { + expect(problems([{ name: "release/*", protection: declaring(site, null) }])).toEqual([]); + }, + ); + + test("required_signatures and strict refuse null on every entry, wildcard included", () => { + // The two toggles typed before the rows above: their wildcard refusals stand beside the nulls a wildcard rule may carry. + for (const name of ["main", "release/*"]) { + expect(problems([{ name, protection: { required_signatures: null } }])).toEqual([ + "branches[0].protection.required_signatures has no empty state; write true or false", + ]); + const checks = { strict: null, contexts: [] }; + expect(problems([{ name, protection: { required_status_checks: checks } }])).toEqual([ + "branches[0].protection.required_status_checks.strict has no empty state; write true or false", + ]); + } + }); + // 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 +388,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 +410,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 66aa34b5c..b2090438b 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], From 2bb4fabb314b50e8e7665886111532b1ca7fb219 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 22:46:49 -0500 Subject: [PATCH 2/3] fix(branches): publish the literal-branch null refusal in the JSON schema The published schema accepted `protection: {allow_deletions: null}` on a literal branch while the runtime refused it. The refusal now lives on BranchConfig in schema.ts beside its published twin: one conditional that types the same nine keys boolean when the name is not a wildcard pattern, over the regex the runtime tests. index.ts keeps the wildcard key sweep alone. The rows in schema.test.ts now judge every PUT boolean with both validators, and a scenario whose settings hold only that null puts the document in front of the corpus gate. The PUT's 422 hint says what it is: causes the parser does not refuse. A list of 101 actors can be counted from the file, so "only GitHub can judge" was wrong. The test that restated two refusals main already had is deleted, and three comments on constants are one line each. --- src/sections/branches/endpoints.ts | 9 +- src/sections/branches/index.ts | 48 +++------- src/sections/branches/schema.ts | 90 ++++++++++++++++--- ...branches-literal-null-control-rejected.yml | 21 +++++ test/sections/branches/schema.test.ts | 54 +++++------ 5 files changed, 139 insertions(+), 83 deletions(-) create mode 100644 test/sections/branches/scenarios/branches-literal-null-control-rejected.yml diff --git a/src/sections/branches/endpoints.ts b/src/sections/branches/endpoints.ts index aa70c43a0..db7f8400d 100644 --- a/src/sections/branches/endpoints.ts +++ b/src/sections/branches/endpoints.ts @@ -21,13 +21,12 @@ 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 only what GitHub's - // descriptor says the PUT judges against the repository itself. + // A malformed shape is refused when the file is parsed, so the hint names causes the parser does not refuse. hints: { 422: - "Usually a state only GitHub can judge, since the file's shape is checked before any " + - 'request: users or teams under "restrictions" or "dismissal_restrictions" on a ' + - "repository a personal account owns, more than 100 actors across the actor lists, or " + + "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', }, }, diff --git a/src/sections/branches/index.ts b/src/sections/branches/index.ts index 1a9778d7e..317a8684b 100644 --- a/src/sections/branches/index.ts +++ b/src/sections/branches/index.ts @@ -20,7 +20,6 @@ import { liveByIdentity, liveIdentity } from "../contract/live.js"; import type { SectionModule } from "../contract/module.js"; import type { SectionPermission } from "../contract/permissions.js"; import type { PlainData, Read } from "../contract/plan.js"; -import { valueAt } from "../shared/list/write.js"; import { layeredList, routed, rule } from "../shared/schema-helpers.js"; import { ENDPOINTS, MISSING_BRANCH } from "./endpoints.js"; import { @@ -59,8 +58,7 @@ import { BranchProtectionConfig, copiedActorName, PROTECTION_MAPPING_KEYS, - PUT_CONTROLS, - REVIEW_TOGGLES, + WILDCARD_NAME, } from "./schema.js"; /** The classic PUT rejects a payload missing any of these; null is a valid value for each. */ @@ -79,21 +77,6 @@ const PROTECTION_VOCABULARY: ReadonlyMap> = new Map( }).map(([holder, keys]) => [holder, new Set(keys)]), ); -/** - * The booleans a literal entry may not declare null, each as its steps under protection: GitHub's protection PUT - * answers 422 to a null it does not mark nullable. The shape takes the null, since a wildcard entry may carry it - * (schema.ts states the two contracts). - */ -const LITERAL_BOOLEANS_WITHOUT_NULL: readonly (readonly string[])[] = [ - ...PUT_CONTROLS.filter((key) => !NULLABLE_CONTROLS.has(key)).map((key) => [key]), - ...REVIEW_TOGGLES.map((key) => ["required_pull_request_reviews", key]), -]; - -/** The steps under protection of each null a literal entry declares where the protection PUT takes none. */ -function nullsThePutRefuses(protection: Record): (readonly string[])[] { - return LITERAL_BOOLEANS_WITHOUT_NULL.filter((steps) => valueAt(protection, steps) === null); -} - /** * Whether every step of a phantom's path names a key the PUT documents under its holder. The steps * are checked one at a time, so a literal key holding a dot never reads as documented nesting. @@ -157,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. @@ -819,9 +802,8 @@ export const branchesSection = { permission, endpoints: ENDPOINTS, graphql: GRAPHQL, - // The two per-surface rules compose HERE, not in schema.ts, which holds one shape for both kinds of entry: - // the wildcard key sweep reads the GraphQL translation tables, this section's own machinery, and the - // literal null refusal is the protection PUT's contract alone. + // The wildcard key sweep composes HERE, not in schema.ts: it reads the GraphQL translation tables, + // which are this section's own machinery, and nothing outside them can reach a wildcard rule. shape: routed(layeredList(BranchesConfig)).check( rule((declared, refineCtx) => { // The routed shape parsed one of the two forms; under the wrapper an issue's path starts at `entries`. @@ -835,27 +817,17 @@ export const branchesSection = { entries.forEach((entry: BranchConfig, index) => { // The entry, its name, or its protection may be raw or missing beside its own shape issue; a regex would // coerce the name (`[object Object]` wildcards), and a non-mapping protection holds no keys to sweep. - if (!isPlainObject(entry) || typeof entry.name !== "string") { + if ( + !isPlainObject(entry) || + typeof entry.name !== "string" || + !isWildcardPattern(entry.name) + ) { return; } const protection = entry.protection; if (!isPlainObject(protection)) { return; } - - if (!isWildcardPattern(entry.name)) { - for (const steps of nullsThePutRefuses(protection)) { - // The type issue it is on this surface, so validation words it like any null with no empty state. - refineCtx.addIssue({ - code: "invalid_type", - expected: "boolean", - input: null, - path: at(index, "protection", ...steps), - }); - } - return; - } - for (const steps of wildcardKeysOutsideVocabulary(protection)) { refineCtx.addIssue({ code: "custom", diff --git a/src/sections/branches/schema.ts b/src/sections/branches/schema.ts index 71d996dae..c035e5f8e 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 { type BooleanControl, 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 } @@ -210,13 +210,7 @@ function control(key: PutControl) { }); } -/** - * The booleans of the PUT body. GitHub states two contracts for a null under one, and this one shape serves both - * kinds of entry, so it takes the null and index.ts 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 - */ +/** 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(), @@ -230,8 +224,8 @@ const PutControls = { allow_fork_syncing: control("allow_fork_syncing").nullable().optional(), } satisfies Record; -/** The keys of the block above, for the literal entry's null refusal (index.ts). */ -export const PUT_CONTROLS = Object.keys(PutControls); +/** 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 = { @@ -246,8 +240,8 @@ const ReviewToggles = { .optional(), }; -/** The keys of the block above, for the same refusal. */ -export const REVIEW_TOGGLES = Object.keys(ReviewToggles); +/** 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( { @@ -323,6 +317,46 @@ function refuseGetOnlyKeys( ancestors.delete(value); } +/** 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), @@ -365,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(), @@ -395,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); + } + }), + ) + .meta({ + id: "BranchConfig", + // The null refusal's published twin, read from the same two lists: a refinement does not reach z.toJSONSchema. + ...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/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 000000000..4809bedfb --- /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/schema.test.ts b/test/sections/branches/schema.test.ts index e25631ec9..ba6a4974f 100644 --- a/test/sections/branches/schema.test.ts +++ b/test/sections/branches/schema.test.ts @@ -1,7 +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[] { @@ -17,6 +19,14 @@ function problems(entries: unknown[]): readonly string[] { 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; @@ -37,10 +47,7 @@ function booleansUnder(node: DescriptorNode, path: string[]): PutBoolean[] { ); } -/** - * Read off GitHub's own descriptor, so a boolean the PUT gains is held to the same refusals. `strict` keeps its - * own rows: it was typed before, under a wording that also covers its absence. - */ +/** 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, @@ -259,45 +266,40 @@ describe("branches protection parse rules", () => { test.each( PUT_BOOLEANS.flatMap(({ site }) => ["main", "release/*"].map((name) => ({ site, name }))), )( - "a quoted $site is refused on $name, naming the YAML gotcha: the PUT answers 422 to it and the rule mutation's Boolean input cannot take a string", + "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 }) => { - expect(problems([{ name, protection: declaring(site, "yes") }])).toEqual([ - `branches[0].protection.${site}: ${site}${QUOTED_ADVICE}`, - ]); - expect(problems([{ name, protection: declaring(site, true) }])).toEqual([]); - expect(problems([{ name, protection: declaring(site, false) }])).toEqual([]); + 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 }) => { - expect(problems([{ name: "main", protection: declaring(site, null) }])).toEqual( + 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, whose mutation input types every twin as a nullable Boolean, so GitHub can accept the file's first apply", + "$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 }) => { - expect(problems([{ name: "release/*", protection: declaring(site, null) }])).toEqual([]); + const entries = [{ name: "release/*", protection: declaring(site, null) }]; + expect(problems(entries)).toEqual([]); + expect(publishedAccepts(entries)).toBe(true); }, ); - test("required_signatures and strict refuse null on every entry, wildcard included", () => { - // The two toggles typed before the rows above: their wildcard refusals stand beside the nulls a wildcard rule may carry. - for (const name of ["main", "release/*"]) { - expect(problems([{ name, protection: { required_signatures: null } }])).toEqual([ - "branches[0].protection.required_signatures has no empty state; write true or false", - ]); - const checks = { strict: null, contexts: [] }; - expect(problems([{ name, protection: { required_status_checks: checks } }])).toEqual([ - "branches[0].protection.required_status_checks.strict has no empty state; write true or false", - ]); - } - }); - // 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 = [ From 4297360c68db518ae8d614aee24ece948c0f83ae Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Thu, 12 Nov 2026 00:04:58 -0500 Subject: [PATCH 3/3] test(branches): pin the missing-branch line as a run prints it With the engine now leaving alone a line that leads with its section key, the missing-branch refusal prints as `branches[]: ...`, where a run printed `branches: branches[]: ...`. The scenario lines held only a substring both forms contain, so one stdout line now pins the printed error. The comment about the published twin moves above `.meta(`, so the members of that object are uncommented alike. --- src/sections/branches/schema.ts | 2 +- .../scenarios/branches-missing-branch-advisory-apply-fails.yml | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sections/branches/schema.ts b/src/sections/branches/schema.ts index c035e5f8e..49046c3fc 100644 --- a/src/sections/branches/schema.ts +++ b/src/sections/branches/schema.ts @@ -441,9 +441,9 @@ export const BranchConfig = open({ } }), ) + // 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", - // The null refusal's published twin, read from the same two lists: a refinement does not reach z.toJSONSchema. ...conditional( { properties: { name: { not: { pattern: WILDCARD_NAME.source } } } }, { 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 d76af59a5..f47d6891d 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"