fix(engine): warn of a _layering or _remove that nothing reads

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
 

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

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];
+}

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<string, unknown>)._layering,
+    unkeyed: removals.unkeyed,
+    asWritten: options.asWritten,
+  }).forEach((note) => {
+    io.annotate("warning", note);
+  });
   return shapes.map((parsed) => mintValidatedSettings(parsed, policy ?? options.undeclared));
 }
 

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<Record<string, unknown>>): 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<string, readonly number[]>;
 
+/** 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<object>;
+}
+
 /** 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<Record<string, unknown>>,
+): [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<object>,
+): 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<Record<string, unknown>>,
   keyed: KeyedListLayering,
   site: string,
-  viewSite: string,
-  sources: SourceIndices,
+  viewSite: Steps,
+  gathered: Gathered,
 ): { entry: Record<string, unknown>; sites: string[] } {
   const out: Record<string, unknown> = { ...entry };
   const sites: string[] = [];
+  const partitioned = new Set<string>();
   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<string, unknown> = { ...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) };
 }