diff --git a/docs/operate/layering.md b/docs/operate/layering.md index 9647e6a5..cd52ba7f 100644 --- a/docs/operate/layering.md +++ b/docs/operate/layering.md @@ -76,7 +76,7 @@ Beside these, `_remove: true` on a keyed entry drops the lower entry under that Any other underscore key at the top level or on a wrapper fails validation with an error naming which directive belongs where (a note belongs in a YAML comment). A misspelled directive can therefore never pass as a private note and quietly merge a layer meant to replace. -Inside a keyed entry, an underscore key is a field like any other: a closed entry shape refuses it as unrecognized, and an open one keeps it, as it keeps every unknown key. +Inside a keyed entry, an underscore key is a field like any other: a closed entry shape refuses it as unrecognized, and an open one keeps it, as it keeps every unknown key. A `_remove` on an entry of a list that is not keyed (a ruleset's `bypass_actors`) is such a field: it removes nothing, and a document that validates draws a warning naming its path. ## The rules diff --git a/src/engine/admit.ts b/src/engine/admit.ts index 2d40ded5..51c544cd 100644 --- a/src/engine/admit.ts +++ b/src/engine/admit.ts @@ -16,13 +16,16 @@ import { UNDECLARED_POLICIES, } from "../sections/shared/schema-helpers.js"; import type { DistributiveOmit, UndeclaredPolicy } from "../types.js"; -import { KNOBBED, LAYERING_KEY, nestedForm, REMOVE_KEY, UNDECLARED_KEY } from "./directives.js"; +import { + isLayering, + KNOBBED, + LAYERING_KEY, + nestedForm, + REMOVE_KEY, + UNDECLARED_KEY, +} from "./directives.js"; import type { Layer } from "./layers.js"; -function isLayering(value: unknown): value is Layering { - return LAYERINGS.some((layering) => layering === value); -} - function isUndeclaredPolicy(value: unknown): value is UndeclaredPolicy { return UNDECLARED_POLICIES.some((policy) => policy === value); } diff --git a/src/engine/directives.ts b/src/engine/directives.ts index aad78397..b93b7214 100644 --- a/src/engine/directives.ts +++ b/src/engine/directives.ts @@ -1,11 +1,13 @@ /** * The three directive keys a layer may write and the two forms of a keyed list they ride on, the one spelling every - * stage of the fold reads (admit.ts, undeclared.ts, separate-removals.ts, layers.ts). Which location takes which key - * is docs/operate/layering.md's. + * stage of the fold reads (admit.ts, undeclared.ts, separate-removals.ts, layers.ts), and what a run says of a + * directive written where nothing reads it. Which location takes which key is docs/operate/layering.md's. */ -import { isPlainObject } from "../plain-data.js"; +import { describeKind, isPlainObject } from "../plain-data.js"; import { UNDECLARED_POLICY_SECTIONS } from "../schema.js"; +import { LAYERINGS, type Layering } from "../sections/shared/schema-helpers.js"; +import { quote } from "../text.js"; /** The merge directive's key: at a file's top level, or on a list section's wrapper. */ export const LAYERING_KEY = "_layering"; @@ -35,3 +37,53 @@ export function nestedForm(value: unknown): NestedForm | null { } return null; } + +/** Whether a value is one the `_layering` directive takes. */ +export function isLayering(value: unknown): value is Layering { + return LAYERINGS.some((layering) => layering === value); +} + +/** The directives a document writes where nothing reads them, as validateSettingsDoc (orchestrate.ts) finds them. */ +export interface UnreadDirectives { + /** The document's top-level `_layering` as written, undefined when it sets none. */ + readonly layering: unknown; + + /** The path of each `_remove` on an entry no list keys (`SeparatedRemovals.unkeyed`). */ + readonly unkeyed: readonly string[]; + + /** A layer's way back from its standalone view to the file as written, applied to each path; a document judged as written passes none. */ + readonly asWritten: ((path: string) => string) | undefined; +} + +/** + * The warning for a top-level `_layering` outside the directive's values, or null where the key is absent or legal. + * Nothing folds a single document, so the value is never read there; the fold refuses it in a layer (admit.ts), whose + * standalone view never carries the key this far. The value stays out of the text, as it does in that refusal. + */ +function unreadLayeringNote(source: string, value: unknown): string | null { + if (value === undefined || isLayering(value)) { + return null; + } + const got = `${describeKind(value)}${typeof value === "string" ? " that is none of them" : ""}`; + return ( + `${source}: ${LAYERING_KEY} must be one of ${LAYERINGS.map(quote).join(", ")}; got ${got}. ` + + "A single document is not folded, so the directive is not read and the run goes on; mode: render refuses this " + + "file as a layer. Write one of the values, or remove the key; the next major version refuses it here too" + ); +} + +/** The warning for a `_remove` on an entry no list keys, by the marker's path (`rulesets[0].bypass_actors[0]._remove`). */ +export function unkeyedRemovalNote(source: string, path: string): string { + return ( + `${source}: ${path}: ${REMOVE_KEY} belongs on an entry of a keyed list, and this list is not keyed, so it ` + + "removes nothing and passes through as a field of the entry. Remove the key; the next major version refuses it" + ); +} + +/** One warning per directive the document writes where nothing reads it, each naming `source`: the `_layering` first, then every marker in the walk's order. */ +export function unreadDirectiveNotes(source: string, unread: UnreadDirectives): string[] { + const asWritten = unread.asWritten ?? ((path: string) => path); + const notes = unread.unkeyed.map((path) => unkeyedRemovalNote(source, asWritten(path))); + const layering = unreadLayeringNote(source, unread.layering); + return layering === null ? notes : [layering, ...notes]; +} diff --git a/src/engine/orchestrate.ts b/src/engine/orchestrate.ts index 15d372af..ce2e638f 100644 --- a/src/engine/orchestrate.ts +++ b/src/engine/orchestrate.ts @@ -34,6 +34,7 @@ import { import { SECTIONS } from "../sections/registry.js"; import { agree, countNoun } from "../text.js"; import type { MustBeNever, UndeclaredPolicy } from "../types.js"; +import { unreadDirectiveNotes } from "./directives.js"; import { executePlan } from "./execute.js"; import { UNDECLARED_POLICIES } from "./layers.js"; import type { RunOutcome } from "./outcome.js"; @@ -109,6 +110,9 @@ export interface ValidateOptions { /** Who authored the document, "operator" when omitted; the multi-repo flow passes "target" for a target's own settings.yml, so a secret reference in it is refused here, with the rest of the document's problems. */ readonly secretSource?: SettingsSource | undefined; + + /** A layer's way back from its standalone view to the file as written (`StandaloneView.asWritten`), for the path a warning names; a document judged as written passes none. */ + readonly asWritten?: ((path: string) => string) | undefined; } /** What one section's run ends in: the drift it found (check) or the changes it made (apply), with its notes. */ @@ -217,6 +221,16 @@ export function validateSettingsDoc( if (issues.length > 0) { return err({ code: "settings-malformed-sections", source: sourceLabel, issues }); } + // Only a document that runs is warned: in a refused one a closed shape may have named the marker itself. + // COMPAT(v4): a single document whose top-level _layering is outside its values still runs, under a warning; delete the warning and refuse the value as _undeclared's is refused above + // COMPAT(v4): a _remove on an entry no list keys still runs and passes through as a field, under a warning; delete the warning and refuse each path with the removal sites above + unreadDirectiveNotes(sourceLabel, { + layering: (settings as Record)._layering, + unkeyed: removals.unkeyed, + asWritten: options.asWritten, + }).forEach((note) => { + io.annotate("warning", note); + }); return shapes.map((parsed) => mintValidatedSettings(parsed, policy ?? options.undeclared)); } diff --git a/src/engine/separate-removals.ts b/src/engine/separate-removals.ts index 262b930e..80f84895 100644 --- a/src/engine/separate-removals.ts +++ b/src/engine/separate-removals.ts @@ -1,26 +1,47 @@ /** - * A document's removal entries set apart from it, and the way back from an issue over the rest to the document as - * written. validateSettingsDoc (orchestrate.ts) refuses a single document's sites and judges the rest; the fold - * (layers.ts) builds each layer's standalone view on it. + * A document's removal entries set apart from it, the markers no list keys beside them, and the way back from an + * issue over the rest to the document as written. validateSettingsDoc (orchestrate.ts) refuses a single document's + * sites and judges the rest; the fold (layers.ts) builds each layer's standalone view on it. */ +import { z } from "zod"; import { isPlainObject } from "../plain-data.js"; -import { LIST_SECTIONS } from "../schema.js"; +import { LIST_SECTIONS, SECTION_KEYS } from "../schema.js"; import type { KeyedListLayering } from "../sections/contract/keyed-list.js"; import { listLayering } from "../sections/registry.js"; import { nestedForm, REMOVE_KEY } from "./directives.js"; +/** The sections the keyed walk never enters: every list below one is a list nothing keys. */ +const MAPPING_SECTIONS = SECTION_KEYS.filter( + (key) => !(LIST_SECTIONS as readonly string[]).includes(key), +); + /** Whether an entry carries the marker at all, whatever its value: what the standalone view drops and a single document refuses. */ function carriesRemoval(entry: Readonly>): boolean { return entry[REMOVE_KEY] !== undefined; } +/** A path as its steps from the section key down, a list index a number; `z.core.toDotPath` spells it as validation spells an issue's, a key that is no plain name quoted. */ +type Steps = readonly (string | number)[]; + /** * Per list as validation spells it over the document minus its removals (`labels`, `labels.entries`, * `environments[0].variables`), the source index of each kept entry; only a list a removal shifted is recorded. */ type SourceIndices = Map; +/** What one walk of a document gathers beside its partitions. */ +interface Gathered { + /** Where each kept entry of a shifted list sat in the document as written. */ + readonly sources: SourceIndices; + + /** Each marker on an entry no list keys, by its path as validation spells one over the kept entries. */ + readonly unkeyed: string[]; + + /** Each mapping or list walked to its end with no such marker found: skipped when another path meets it again. */ + readonly clean: WeakSet; +} + /** A keyed list's removals set apart from it, its entries' nested lists likewise, in the form the layer wrote them. */ interface Partition { /** The entries minus the removals, each entry's declared nested lists partitioned in turn. */ @@ -30,16 +51,76 @@ interface Partition { readonly sites: string[]; } -/** A kept entry with each declared nested list partitioned in turn, and the sites of the removals below it. */ +/** A list's or a mapping's children, each with the step that reaches it; a list is read by index, since the walk runs before the plainness check that refuses one whose named property shadows a method. */ +function childrenOf( + container: readonly unknown[] | Readonly>, +): [step: string | number, child: unknown][] { + if (!Array.isArray(container)) { + return Object.entries(container); + } + const children: [number, unknown][] = []; + for (let index = 0; index < container.length; index += 1) { + children.push([index, container[index]]); + } + return children; +} + +/** + * Whether a value no keyed list reaches is known to hold no marker, each one below it gathered by its path: a + * `_remove` on a list's entry there removes nothing (the fold replaces such a list whole), so the entry stays. + * + * holds no marker -> remembered, so walked once however many paths a YAML alias gives it, as validate.ts walks one + * holds a marker -> walked per path, so each path keeps its own line + * closes a cycle -> the descent ends there (`above`), and no container on the way back up is remembered: what + * the cut hid is not known + */ +function gatherUnkeyed( + value: unknown, + path: Steps, + gathered: Gathered, + above: Set, +): boolean { + if (!Array.isArray(value) && !isPlainObject(value)) { + return true; + } + if (gathered.clean.has(value)) { + return true; + } + if (above.has(value)) { + return false; + } + + above.add(value); + const entries = Array.isArray(value); + let clean = true; + for (const [step, child] of childrenOf(value)) { + if (entries && isPlainObject(child) && carriesRemoval(child)) { + gathered.unkeyed.push(z.core.toDotPath([...path, step, REMOVE_KEY])); + clean = false; + } + if (!gatherUnkeyed(child, [...path, step], gathered, above)) { + clean = false; + } + } + above.delete(value); + + if (clean) { + gathered.clean.add(value); + } + return clean; +} + +/** A kept entry with each declared nested list partitioned in turn, and the sites of the removals below it; its other fields are walked for the markers no list keys. */ function partitionNestedLists( entry: Readonly>, keyed: KeyedListLayering, site: string, - viewSite: string, - sources: SourceIndices, + viewSite: Steps, + gathered: Gathered, ): { entry: Record; sites: string[] } { const out: Record = { ...entry }; const sites: string[] = []; + const partitioned = new Set(); for (const [field, nested] of Object.entries(keyed.nested ?? {})) { const form = nestedForm(entry[field]); if (form === null) { @@ -50,25 +131,32 @@ function partitionNestedLists( form.entries, nested, `${site}.${field}`, - form.knobs === null ? `${viewSite}.${field}` : `${viewSite}.${field}.entries`, - sources, + form.knobs === null ? [...viewSite, field] : [...viewSite, field, "entries"], + gathered, ); sites.push(...below.sites); out[field] = form.knobs === null ? below.kept : { ...form.knobs, entries: below.kept }; + partitioned.add(field); + } + + for (const [field, value] of Object.entries(entry)) { + if (!partitioned.has(field)) { + gatherUnkeyed(value, [...viewSite, field], gathered, new Set()); + } } return { entry: out, sites }; } /** - * `path` names the list as the sites do, by the document's own indices; `viewPath` as validation spells the kept - * entries (`labels.entries`, `environments[1].variables`), the key under which `sources` records their origins. + * `path` names the list as the sites do, by the document's own indices; `viewPath` as validation reaches the kept + * entries (`labels.entries`, `environments[1].variables`), the key under which `gathered` records their origins. */ function partitionRemovals( entries: readonly unknown[], keyed: KeyedListLayering, path: string, - viewPath: string, - sources: SourceIndices, + viewPath: Steps, + gathered: Gathered, ): Partition { const kept: unknown[] = []; const sites: string[] = []; @@ -89,15 +177,15 @@ function partitionRemovals( continue; } - const viewSite = `${viewPath}[${kept.length}]`; + const viewSite = [...viewPath, kept.length]; origins.push(index); - const below = partitionNestedLists(entry, keyed, site, viewSite, sources); + const below = partitionNestedLists(entry, keyed, site, viewSite, gathered); sites.push(...below.sites); kept.push(below.entry); } if (origins.some((source, index) => source !== index)) { - sources.set(viewPath, origins); + gathered.sources.set(z.core.toDotPath(viewPath), origins); } return { kept, sites }; } @@ -141,22 +229,26 @@ export interface SeparatedRemovals { /** Each removal's site (`labels[0]._remove`): list sections in LIST_SECTIONS order, each list as written, nested lists under their entry. */ readonly sites: readonly string[]; + /** Each `_remove` on an entry no list keys, which stays in `rest`: by its path over `rest` as validation spells one (`rulesets.entries[0].bypass_actors[0]._remove`), the list sections first; `asWritten` is its way back. */ + readonly unkeyed: readonly string[]; + /** A validation issue over `rest`, its leading path renumbered to the document as written: a removal dropped from the list shifts every entry after it, so `labels[0].color` in `rest` is the reader's `labels[1].color`. */ readonly asWritten: (issue: string) => string; } /** - * Every list section partitioned by the walk the fold uses. A single document has no lower layer to remove from, so + * Every section walked once: a list section partitioned by the walk the fold uses, the rest of the document searched + * for the markers that walk does not reach. A single document has no lower layer to remove from, so * validateSettingsDoc refuses the sites and judges `rest`; a layer of a fold reaches it as its standalone view, * `rest` minus the `_layering` directives. Either way what is said about `rest` is said of the document as written. */ export function separateRemovals(doc: unknown): SeparatedRemovals { if (!isPlainObject(doc)) { - return { rest: doc, sites: [], asWritten: (issue) => issue }; + return { rest: doc, sites: [], unkeyed: [], asWritten: (issue) => issue }; } const rest: Record = { ...doc }; const sites: string[] = []; - const sources: SourceIndices = new Map(); + const gathered: Gathered = { sources: new Map(), unkeyed: [], clean: new WeakSet() }; for (const key of LIST_SECTIONS) { const form = nestedForm(doc[key]); if (form === null) { @@ -166,11 +258,16 @@ export function separateRemovals(doc: unknown): SeparatedRemovals { form.entries, listLayering(key), key, - form.knobs === null ? key : `${key}.entries`, - sources, + form.knobs === null ? [key] : [key, "entries"], + gathered, ); sites.push(...below.sites); rest[key] = form.knobs === null ? below.kept : { ...form.knobs, entries: below.kept }; } - return { rest, sites, asWritten: renumbering(sources) }; + + for (const key of MAPPING_SECTIONS) { + gatherUnkeyed(doc[key], [key], gathered, new Set()); + } + + return { rest, sites, unkeyed: gathered.unkeyed, asWritten: renumbering(gathered.sources) }; } diff --git a/src/flows/layers.ts b/src/flows/layers.ts index 647f12b4..b7c19e3b 100644 --- a/src/flows/layers.ts +++ b/src/flows/layers.ts @@ -13,6 +13,7 @@ import { type ValidatedSettings, validateSettingsDoc } from "../engine/orchestra import { SectionSelection } from "../engine/section-selection.js"; import type { Io } from "../io.js"; import type { LayerProblem, ProblemOf, SettingsProblem } from "../problem.js"; +import { capturingIo } from "./redact.js"; import { readSettingsFile } from "./settings-read.js"; export function readLayerFiles( @@ -47,7 +48,9 @@ export interface FoldedLayers { */ function validateLayer(layer: Layer, io: Io): Result { const view = standaloneView(layer.doc); - return validateSettingsDoc(view.doc, layer.name, EVERY_SECTION, io).mapErr((problem) => + return validateSettingsDoc(view.doc, layer.name, EVERY_SECTION, io, { + asWritten: view.asWritten, + }).mapErr((problem) => problem.code === "settings-malformed-sections" ? { ...problem, issues: problem.issues.map(view.asWritten) } : problem, @@ -58,6 +61,9 @@ function validateLayer(layer: Layer, io: Io): Result { - return Result.combine(layers.map((layer) => validateLayer(layer, io))) + const held = capturingIo(io); + return Result.combine(layers.map((layer) => validateLayer(layer, held.io))) .andThen(() => mergeLayers(layers, options)) .andThen((merged) => // The fold resolved every policy already, so the run input changes nothing here; passed so the two paths read alike. - validateSettingsDoc(merged.settings, sourceLabel, EVERY_SECTION, io, { + validateSettingsDoc(merged.settings, sourceLabel, EVERY_SECTION, held.io, { undeclared: options.undeclared, }).map((settings) => ({ settings, @@ -77,5 +84,15 @@ export function foldLayers( // Validation just proved the fold a plain mapping of section keys: the directives were consumed, and any other key refused. yaml: renderCanonicalYaml(merged.settings as Record), })), - ); + ) + .map((folded) => { + for (const { level, line } of held.drain()) { + if (level === undefined) { + io.log(line); + } else { + io.annotate(level, line); + } + } + return folded; + }); } diff --git a/test/e2e/scenarios/apply-layering-outside-values-warns.yml b/test/e2e/scenarios/apply-layering-outside-values-warns.yml new file mode 100644 index 00000000..4a8b0751 --- /dev/null +++ b/test/e2e/scenarios/apply-layering-outside-values-warns.yml @@ -0,0 +1,27 @@ +# A single document's top-level `_layering` outside replace|shallow|deep is read +# by nothing, since no fold runs: the apply goes on as it always did, now under +# one warning naming the values and the fix. mode: render refuses the same file +# as a layer. Raw text, because the published schema refuses the value and the +# corpus's schema check holds every `settings` mapping to it. +name: apply-layering-outside-values-warns +settings_raw: | + _layering: banana + labels: + - name: bug + color: d73a4a +expect: + exit_code: 0 + result: applied + outcomes: + labels: applied + mutations: + - "POST /repos/{repo}/labels" + stdout_contains: + - "::warning::" + - >- + settings.yml: _layering must be one of "replace", "shallow", "deep"; got a string that is none of them. + A single document is not folded, so the directive is not read and the run goes on; + mode: render refuses this file as a layer. + Write one of the values, or remove the key; the next major version refuses it here too + - 'labels: created label "bug"' + fixpoint: converges diff --git a/test/e2e/scenarios/apply-unkeyed-remove-warns.yml b/test/e2e/scenarios/apply-unkeyed-remove-warns.yml new file mode 100644 index 00000000..5368e811 --- /dev/null +++ b/test/e2e/scenarios/apply-unkeyed-remove-warns.yml @@ -0,0 +1,32 @@ +# `_remove` on an entry of a list the registry does not key (a ruleset's +# bypass_actors) removes nothing: the entry is applied as written, the marker +# one of its fields as before, and the run now warns, naming the entry's path. +name: apply-unkeyed-remove-warns +settings: + rulesets: + - name: r + target: branch + enforcement: active + bypass_actors: + - actor_id: 1 + actor_type: Team + bypass_mode: always + _remove: true +token_permissions: + administration: write +expect: + exit_code: 0 + result: applied + outcomes: + rulesets: applied + mutations: + - "POST /repos/{repo}/rulesets" + stdout_contains: + - "::warning::" + - >- + settings.yml: rulesets[0].bypass_actors[0]._remove: + _remove belongs on an entry of a keyed list, and this list is not keyed, + so it removes nothing and passes through as a field of the entry. + Remove the key; the next major version refuses it + - 'rulesets: created ruleset "r"' + fixpoint: converges diff --git a/test/e2e/scenarios/render-unkeyed-remove-warns.yml b/test/e2e/scenarios/render-unkeyed-remove-warns.yml new file mode 100644 index 00000000..84eeffe8 --- /dev/null +++ b/test/e2e/scenarios/render-unkeyed-remove-warns.yml @@ -0,0 +1,54 @@ +# A layer's warning names the entry by its index in the file as written: the +# keyed removal before it is counted (rulesets[1], where the layer's standalone +# view holds it at 0) and still drops the lower entry. The marker rides into +# the rendered document as before, which the fold names once more by that +# document's own path. The warnings print before the removal's notice. +name: render-unkeyed-remove-warns +inputs: + mode: render +settings_layers: + - rulesets: + - name: old + target: branch + enforcement: active + - name: r + target: branch + enforcement: active +settings: + rulesets: + - name: old + _remove: true + - name: r + bypass_actors: + - actor_id: 1 + actor_type: Team + bypass_mode: always + _remove: true +expect: + exit_code: 0 + result: rendered + zero_requests: true + stdout_contains: + - >- + settings.yml: rulesets[1].bypass_actors[0]._remove: + _remove belongs on an entry of a keyed list, and this list is not keyed, + so it removes nothing and passes through as a field of the entry. + Remove the key; the next major version refuses it + - >- + ::warning::the rendered settings document: rulesets.entries[0].bypass_actors[0]._remove: + _remove belongs on an entry of a keyed list, and this list is not keyed, + so it removes nothing and passes through as a field of the entry. + Remove the key; the next major version refuses it + - "settings.yml: rulesets[0] carries _remove: true and dropped the entry a lower layer declared under its key" + rendered: + rulesets: + _undeclared: keep + entries: + - name: r + target: branch + enforcement: active + bypass_actors: + - actor_id: 1 + actor_type: Team + bypass_mode: always + _remove: true diff --git a/test/engine/orchestrate.test.ts b/test/engine/orchestrate.test.ts index c4a53729..c992b364 100644 --- a/test/engine/orchestrate.test.ts +++ b/test/engine/orchestrate.test.ts @@ -1,6 +1,8 @@ import { afterEach, describe, expect, spyOn, test } from "bun:test"; import { err, ok } from "neverthrow"; +import { unkeyedRemovalNote } from "../../src/engine/directives.js"; +import { standaloneView } from "../../src/engine/layers.js"; import { preflightProbe, type RepoRunOptions, @@ -299,6 +301,31 @@ describe("validateSettingsDoc secret references", () => { }); describe("validateSettingsDoc", () => { + const TEAM_ACTOR = { actor_id: 1, actor_type: "Team", bypass_mode: "always" }; + + /** One ruleset holding the given fields, as a document. */ + const ruleset = (fields: Record) => ({ + rulesets: [{ name: "r", target: "branch", enforcement: "active", ...fields }], + }); + + /** One ruleset whose pull_request rule holds the given parameters beside the required ones. */ + const pullRequestRuleset = (parameters: Record) => + ruleset({ + rules: [ + { + type: "pull_request", + parameters: { + dismiss_stale_reviews_on_push: false, + require_code_owner_review: false, + require_last_push_approval: false, + required_approving_review_count: 1, + required_review_thread_resolution: false, + ...parameters, + }, + }, + ], + }); + test("unknown top-level keys are one collected line naming the known sections", () => { const { io } = captureIo(); expect(validateSettingsDoc({ labls: [] }, "repos/x.yml", SectionSelection.ALL, io)).toEqual( @@ -343,6 +370,31 @@ describe("validateSettingsDoc", () => { ).toBe(true); }); + // Nothing folds a single document, so nothing read the value: `_layering: union` ran silently, while mode: render refused the same file as a layer. + test.each<[what: string, value: unknown, got: string]>([ + ["a string outside its values", "union", "a string that is none of them"], + ["null", null, "null"], + ])( + "a single document's top-level _layering that is %s draws one warning naming the values, and the document still validates", + (_what, value, got) => { + const { io, annotations } = captureIo(); + const doc = { _layering: value, labels: [{ name: "bug" }] }; + expect(validateSettingsDoc(doc, "s.yml", SectionSelection.ALL, io).isOk()).toBe(true); + expect(annotations).toEqual([ + `warning: s.yml: _layering must be one of "replace", "shallow", "deep"; got ${got}. ` + + "A single document is not folded, so the directive is not read and the run goes on; mode: render refuses this file as a layer. " + + "Write one of the values, or remove the key; the next major version refuses it here too", + ]); + }, + ); + + test("control: a top-level _layering of the set is the file's directive as a layer, so a single document carries it without a warning", () => { + const { io, annotations } = captureIo(); + const doc = { _layering: "shallow", labels: [{ name: "bug" }] }; + expect(validateSettingsDoc(doc, "s.yml", SectionSelection.ALL, io).isOk()).toBe(true); + expect(annotations).toEqual([]); + }); + // Each of these once stopped the run alone, so a file with all three took three runs to fix. test("an unknown directive, an unknown section, and a bad enum are reported in one run, in that order", () => { const { io } = captureIo(); @@ -495,6 +547,224 @@ describe("validateSettingsDoc", () => { }, ); + // The class the removal walk passed over: a list of mappings the registry does not key. The marker removes nothing + // there, and it rode to GitHub as a field named _remove with nothing said. + test("the warning for a marker no list keys says what becomes of it and the fix, and the validated document keeps the entry as written", () => { + const { io, annotations } = captureIo(); + const doc = ruleset({ bypass_actors: [{ ...TEAM_ACTOR, _remove: true }] }); + const validated = validateSettingsDoc(doc, "s.yml", SectionSelection.ALL, io); + expect(annotations).toEqual([ + "warning: s.yml: rulesets[0].bypass_actors[0]._remove: _remove belongs on an entry of a keyed list, and this list is not keyed, so it removes nothing and passes through as a field of the entry. Remove the key; the next major version refuses it", + ]); + expect(JSON.stringify(validated._unsafeUnwrap())).toContain( + '"bypass_actors":[{"actor_id":1,"actor_type":"Team","bypass_mode":"always","_remove":true}]', + ); + }); + + // The rest of the class, one list each: the same warning by its own path. + test.each<[list: string, doc: Record, site: string]>([ + [ + "a rule's required_status_checks, below a nested keyed entry", + ruleset({ + rules: [ + { + type: "required_status_checks", + parameters: { + strict_required_status_checks_policy: false, + required_status_checks: [{ context: "ci", _remove: true }], + }, + }, + ], + }), + "rulesets[0].rules[0].parameters.required_status_checks[0]._remove", + ], + [ + "a rule's workflows", + ruleset({ + rules: [ + { + type: "workflows", + parameters: { workflows: [{ path: "ci.yml", repository_id: 1, _remove: true }] }, + }, + ], + }), + "rulesets[0].rules[0].parameters.workflows[0]._remove", + ], + [ + "a rule's code_scanning_tools", + ruleset({ + rules: [ + { + type: "code_scanning", + parameters: { + code_scanning_tools: [ + { + tool: "CodeQL", + alerts_threshold: "errors", + security_alerts_threshold: "high_or_higher", + _remove: true, + }, + ], + }, + }, + ], + }), + "rulesets[0].rules[0].parameters.code_scanning_tools[0]._remove", + ], + [ + "a pull_request rule's required_reviewers", + pullRequestRuleset({ + required_reviewers: [ + { + file_patterns: ["*.ts"], + minimum_approvals: 1, + reviewer: { id: 1, type: "Team" }, + _remove: true, + }, + ], + }), + "rulesets[0].rules[0].parameters.required_reviewers[0]._remove", + ], + [ + "a pull_request rule's dismissal_restriction.allowed_actors", + pullRequestRuleset({ + dismissal_restriction: { + enabled: true, + allowed_actors: [{ id: 1, type: "Team", _remove: true }], + }, + }), + "rulesets[0].rules[0].parameters.dismissal_restriction.allowed_actors[0]._remove", + ], + [ + "a mapping section's list", + { + check_suite_preferences: { + auto_trigger_checks: [{ app_id: 1, setting: true, _remove: true }], + }, + }, + "check_suite_preferences.auto_trigger_checks[0]._remove", + ], + [ + "a list under a passthrough key", + { repository: { custom_list: [{ a: 1, _remove: true }] } }, + "repository.custom_list[0]._remove", + ], + [ + "a list of a wrapper's entry, spelled through .entries, the marker's value unjudged", + { + rulesets: { + entries: ruleset({ bypass_actors: [{ ...TEAM_ACTOR, _remove: "yes" }] }).rulesets, + }, + }, + "rulesets.entries[0].bypass_actors[0]._remove", + ], + ])( + "_remove on an entry of %s draws a warning naming its path, and the entry stays as written", + (_list, doc, site) => { + const { io, annotations } = captureIo(); + const validated = validateSettingsDoc(doc, "s.yml", SectionSelection.ALL, io); + expect(annotations).toEqual([`warning: ${unkeyedRemovalNote("s.yml", site)}`]); + expect(JSON.stringify(validated._unsafeUnwrap())).toContain('"_remove":'); + }, + ); + + // The warnings speak of a document that runs. Beside a refusal the marker's one said "passes through" of a key a + // closed shape had just refused, and promised for the next major what this run already did. + test.each<[what: string, doc: Record, issue: string | RegExp]>([ + [ + "a closed entry shape names the marker itself", + { + branches: [ + { + name: "main", + protection: { + required_status_checks: { strict: true, checks: [{ context: "ci", _remove: true }] }, + }, + }, + ], + }, + /^branches\[0\]\.protection\.required_status_checks\.checks\[0\]: .* remove "_remove"$/, + ], + [ + "a keyed removal stops it, a bad top-level _layering beside it", + { + _layering: "union", + rulesets: [ + { name: "old", _remove: true }, + ...ruleset({ bypass_actors: [{ ...TEAM_ACTOR, _remove: true }] }).rulesets, + ], + }, + singleDocumentRemovalIssue("rulesets[0]._remove"), + ], + ])( + "a refused document draws no warning for a directive nothing reads: %s", + (_what, doc, issue) => { + const { io, annotations } = captureIo(); + expect(validateSettingsDoc(doc, "s.yml", SectionSelection.ALL, io)).toEqual( + err({ + code: "settings-malformed-sections", + source: "s.yml", + issues: [typeof issue === "string" ? issue : expect.stringMatching(issue)], + }), + ); + expect(annotations).toEqual([]); + }, + ); + + // The way back reads a path's steps, so a key that only looks like one must not be read as one: unquoted, the + // `[0]` of the key below was renumbered with the rules list the removal shifted. + test("a passthrough key spelled like a list step is quoted in the path a warning names, as an issue's is, so a removal in the list it resembles never renumbers it", () => { + const { io, annotations } = captureIo(); + const view = standaloneView( + ruleset({ + rules: [{ type: "creation", _remove: true }, { type: "deletion" }], + "rules[0]": [{ _remove: true }], + }), + ); + const options = { asWritten: view.asWritten }; + expect( + validateSettingsDoc(view.doc, "repo.yml", SectionSelection.ALL, io, options).isOk(), + ).toBe(true); + expect(annotations).toEqual([ + `warning: ${unkeyedRemovalNote("repo.yml", 'rulesets[0]["rules[0]"][0]._remove')}`, + ]); + }); + + // A layer is judged as its standalone view, the keyed removals gone, so the view's index is not the file's; the + // wrapper form is the one whose path the way back reads through .entries. + test("a layer's view hands over its way back, so a warning names the wrapper's entry by its index in the layer as written", () => { + const { io, annotations } = captureIo(); + const view = standaloneView({ + rulesets: { + entries: [ + { name: "old", _remove: true }, + ...ruleset({ bypass_actors: [{ ...TEAM_ACTOR, _remove: true }] }).rulesets, + ], + }, + }); + const options = { asWritten: view.asWritten }; + expect( + validateSettingsDoc(view.doc, "repo.yml", SectionSelection.ALL, io, options).isOk(), + ).toBe(true); + expect(annotations).toEqual([ + `warning: ${unkeyedRemovalNote("repo.yml", "rulesets.entries[1].bypass_actors[0]._remove")}`, + ]); + }); + + test("control: a cyclic passthrough value still ends in the boundary's refusal, since the marker walk does not follow a cycle", () => { + const loop: Record = { list: [] }; + (loop.list as unknown[]).push(loop); + expect( + validateSettingsDoc({ repository: { loop } }, "s.yml", SectionSelection.ALL, silentIo()), + ).toEqual( + err({ + code: "settings-malformed-sections", + source: "s.yml", + issues: [expect.stringContaining("refers back to one of its own containers")], + }), + ); + }); + test("a list whose named property shadows a method is refused by the plainness check, never met by the removal walk", () => { // The walk for removals runs on the raw document, before the shapes; calling the list's own forEach would throw here. expect( diff --git a/test/engine/separate-removals.test.ts b/test/engine/separate-removals.test.ts new file mode 100644 index 00000000..390f9acc --- /dev/null +++ b/test/engine/separate-removals.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, test } from "bun:test"; +import { separateRemovals } from "../../src/engine/separate-removals.js"; + +/** A plain mapping that counts each time a walk lists its keys; a Proxy over one still reads as plain YAML data. */ +function counted(target: T, visits: { count: number }): T { + return new Proxy(target, { + ownKeys: (inner) => { + visits.count += 1; + return Reflect.ownKeys(inner); + }, + }); +} + +describe("separateRemovals: the markers no list keys", () => { + // One YAML anchor aliased under two keys, 24 levels deep, is 25 mappings and 2^25 - 1 paths: walked per path, + // a library caller's document that holds no marker at all took seconds to say so. + test("a mapping shared by many paths is walked once when it holds no marker", () => { + const visits = { count: 0 }; + let shared: object = counted({}, visits); + for (let depth = 0; depth < 24; depth += 1) { + shared = counted({ left: shared, right: shared }, visits); + } + expect(separateRemovals({ repository: { custom: shared } }).unkeyed).toEqual([]); + expect(visits.count).toBe(25); + }); + + // A walk a cycle cut short does not know what the cut hid. Remembered as marker-free all the same, the mapping + // was skipped on its second path, and a document whose cycle the shapes drop (an own `__proto__` key) lost a line. + test("a mapping whose walk a cycle cut short is not remembered as marker-free, so a second path through it still finds the marker", () => { + const first: Record = { custom_list: [{ _remove: true }] }; + const second = { back: first }; + first.next = second; + expect(separateRemovals({ repository: first, actions: second }).unkeyed).toEqual([ + "repository.custom_list[0]._remove", + "actions.back.custom_list[0]._remove", + ]); + }); + + test("control: a shared list that holds a marker is still walked per path, so every path keeps its own line", () => { + const marked = [{ _remove: true }]; + const quiet = { nested: [{ a: 1 }] }; + const doc = { + repository: { first: marked, calm: quiet, second: { again: marked, calm: quiet } }, + }; + expect(separateRemovals(doc).unkeyed).toEqual([ + "repository.first[0]._remove", + "repository.second.again[0]._remove", + ]); + }); +}); diff --git a/test/flows/layers.test.ts b/test/flows/layers.test.ts index 0ba59bf8..b8624726 100644 --- a/test/flows/layers.test.ts +++ b/test/flows/layers.test.ts @@ -2,11 +2,13 @@ import { describe, expect, spyOn, test } from "bun:test"; import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { err, ok } from "neverthrow"; +import { unkeyedRemovalNote } from "../../src/engine/directives.js"; import type { Layering } from "../../src/engine/layers.js"; import { foldLayers, readLayerFiles } from "../../src/flows/layers.js"; import * as settingsRead from "../../src/flows/settings-read.js"; import { silentIo } from "../../src/io.js"; import { describeProblem } from "../../src/problem.js"; +import { captureIo } from "../io/capture.js"; import { withTempDir } from "../temp-dir.js"; describe("readLayerFiles", () => { @@ -54,6 +56,9 @@ describe("readLayerFiles", () => { describe("foldLayers", () => { const fleet = { name: "fleet.yml", doc: { labels: [{ name: "bug", description: "fleet" }] } }; + + /** A repository section whose passthrough list carries a marker no list keys: valid, and warned of. */ + const MARKED_REPOSITORY = { custom_list: [{ _remove: true }] }; const fold = (doc: Record, layering: Layering) => foldLayers([fleet, { name: "repo.yml", doc }], "merged", { layering }, silentIo()); @@ -179,6 +184,96 @@ describe("foldLayers", () => { }, ); + // A warning is no issue, so nothing renumbered it: the layer's view index went out, one short per removal before it. + test("a layer's warning names the entry by its index in the file as written, the keyed removal before it counted and still dropping its lower entry; the warnings print in layer order, the fold's own last", () => { + const ruleset = (name: string) => ({ name, target: "branch", enforcement: "active" }); + const marked = { actor_id: 1, actor_type: "Team", bypass_mode: "always", _remove: true }; + const { io, annotations } = captureIo(); + const folded = foldLayers( + [ + { + name: "fleet.yml", + doc: { + rulesets: [ruleset("old"), ruleset("r")], + repository: { custom_list: [{ _remove: true }] }, + }, + }, + { + name: "repo.yml", + doc: { + rulesets: [ + { name: "old", _remove: true }, + { name: "r", bypass_actors: [marked] }, + ], + }, + }, + ], + "merged", + { layering: "deep" }, + io, + ); + expect(folded.map((out) => out.notices)).toEqual( + ok([{ layer: "repo.yml", path: "rulesets[0]" }]), + ); + expect(annotations).toEqual([ + `warning: ${unkeyedRemovalNote("fleet.yml", "repository.custom_list[0]._remove")}`, + `warning: ${unkeyedRemovalNote("repo.yml", "rulesets[1].bypass_actors[0]._remove")}`, + `warning: ${unkeyedRemovalNote("merged", "rulesets.entries[0].bypass_actors[0]._remove")}`, + `warning: ${unkeyedRemovalNote("merged", "repository.custom_list[0]._remove")}`, + ]); + }); + + // A layer's standalone view validates without what the fold alone judges, so its warning went out ahead of the + // fold's refusal: "passes through as a field" printed for a render that wrote nothing. + test.each<[what: string, above: { name: string; doc: Record }[], code: string]>([ + [ + "its own top-level _layering is outside the values", + [{ name: "repo.yml", doc: { _layering: "banana", repository: MARKED_REPOSITORY } }], + "layer-bad-directive", + ], + [ + "a keyed marker beside it is not true", + [ + { + name: "repo.yml", + doc: { labels: [{ name: "bug", _remove: false }], repository: MARKED_REPOSITORY }, + }, + ], + "layer-remove-not-true", + ], + [ + "a wrapper's _layering beside it is outside the values", + [ + { + name: "repo.yml", + doc: { + labels: { _layering: "union", entries: [{ name: "docs" }] }, + repository: MARKED_REPOSITORY, + }, + }, + ], + "layer-bad-directive", + ], + [ + "another layer is one the shapes refuse", + [ + { name: "team.yml", doc: { repository: MARKED_REPOSITORY } }, + { name: "repo.yml", doc: { labels: "oops" } }, + ], + "settings-malformed-sections", + ], + ])("a refused fold prints no layer's warning: %s", (_what, above, code) => { + const { io, annotations } = captureIo(); + const folded = foldLayers([fleet, ...above], "merged", { layering: "deep" }, io); + expect( + folded.match( + () => null, + (problem) => problem.code, + ), + ).toBe(code); + expect(annotations).toEqual([]); + }); + test("a malformed marker inside a closed entry schema is the fold's refusal, not the layer's own unknown-key problem", () => { // Per-layer validation runs before the fold, and an environment secret's schema is closed: were the marker left in // the standalone view, the layer would fail on `Unrecognized key: "_remove"` and never hear that it takes only true.