fix(cli): os validate runs the per-package author-time rule pass os build already ran

.changeset/18677-validate-per-package-authoring-pass.md

@@ -0,0 +1,22 @@
+---
+'@objectstack/cli': minor
+---
+
+`os validate` runs the per-package author-time rule pass `os build` already ran — the false-clean residue #17069 left one layer down.
+
+`os build` runs the artifact's authoring rules **twice**: once over the union-folded stack, then a second `runAuthoringRules('build', …)` pass over each `artifactPackages(…)` entry with `packageBodyAsStack(…)` as resolution context, de-duplicated against the union run. `os validate` ran the union pass and stopped — it imported neither seam. By `compile.ts`' own description the survivors of that second pass are "exactly the set the union could not see", so that whole set was findings `os build` reported and `os validate` **structurally could not**. The direction is false-clean, and on the worse door: the fast pre-flight is what an author runs *before* shipping, so its clean bill of health is the strongest false assurance the three commands can give.
+
+Measured on `origin/main` 09e16a574 over `examples/app-multi-package`, both commands exiting 0:
+
+```
+os build    --json  warnings: 4      <- 3 union + 1 per-package survivor
+os validate --json  warnings: 3      <- the survivor is the defect
+```
+
+After: both report 4, the same set, in the same order.
+
+**The loop is now one seam, not two copies.** `runPerPackageAuthoringRules` lives beside `artifactPackages` / `packageBodyAsStack` in `utils/artifact-packages.ts`, whose header already forbids a second copy of that shape by name. What would have drifted between two hand-written loops is not the package reading but the **verdict** — the de-duplication key, the severity split, the `where` prefix. `os build`'s observable output is unchanged (text face byte-identical modulo timings; `--json` payload identical).
+
+**Severity mapping is `os build`'s, unchanged.** A per-package `error` refuses (exit 1); an advisory joins `warnings`. So `os validate` is narrowed only to the bar the command that *ships* already holds: every input it can now refuse is one `os build` already refuses, which means **nothing that builds today stops validating**. No newly-refused input could be exhibited on any fixture — across the repo's own two-package example and three constructed variants the observable change is advisory-only, because `packageBodyAsStack` hands each package the artifact's whole `packages[]` as resolution context and the reference-integrity suite resolves object names through it. Graded `minor` rather than `patch` for the new observable step line, the new advisories and the newly reachable non-zero exit; ⛔ **not** declared breaking, because the narrowing could not be exhibited and is bounded by an existing gate.
+
+Unchanged and out of scope: the ADR-0130 D4 union fold (#17069, fixed — `authoringRuleUnionStack` is in both commands), `--json` rendering (#11727), and disagreements *within* the per-package pass's verdicts (#18204). `os lint` still runs the union pass alone; its `artifactPackages` / `packageBodyAsStack` imports serve its own intra-package duplicate-name advisory, not the shared table.

packages/cli/src/commands/compile.ts

@@ -16,7 +16,7 @@ import {
 import { loadConfig, namedExportRejectionHints } from '../utils/config.js';
 import { lowerCallables } from '../utils/lower-callables.js';
 import { authoringRuleUnionStack } from '../utils/stack-collections.js';
-import { artifactPackages, packageBodyAsStack } from '../utils/artifact-packages.js';
+import { artifactPackages, runPerPackageAuthoringRules } from '../utils/artifact-packages.js';
 import { buildAccessMatrix, diffAccessMatrix } from '@objectstack/lint';
 import { runAuthoringRules, splitBySeverity, authoringRulesFor } from '@objectstack/lint';
 import { resolveSduiManifest } from '../utils/sdui-manifest.js';
@@ -59,10 +59,6 @@ import {
 } from '../utils/permission-set-name-collisions.js';
 import type { PermissionSetNameCollisionDiagnostic } from '@objectstack/plugin-security';
 
-/** Identity of one finding, for the per-package de-duplication below. */
-const findingKey = (f: { rule: string; where: string; path: string; message: string }): string =>
-  `${f.rule}\u0000${f.where}\u0000${f.path}\u0000${f.message}`;
-
 export default class Compile extends Command {
   static override description = 'Compile ObjectStack configuration to JSON artifact';
 
@@ -425,32 +421,35 @@ export default class Compile extends Command {
       //     is the one `artifactPackages` above walked, off the same parsed
       //     stack, so the context a package resolves against is exactly the set
       //     of packages this artifact will register (ADR-0130 D4/D5).
-      const artifactPackageEntries = (result.data as Record<string, unknown>).packages;
+      //
+      //     ⛔ [#18677] The LOOP itself is not written here either — it is
+      //     `runPerPackageAuthoringRules`, beside the two seams it reads, for
+      //     the reason that module's header already gives about them: the
+      //     `os validate` door owes the identical pass, and the thing that
+      //     would have drifted between two hand-written copies is the VERDICT
+      //     (the de-duplication key, the severity split, the `where` prefix),
+      //     not the package reading. Every observable of this step — the step
+      //     line, the advisory order, the error sentence, the `--json` envelope
+      //     — is unchanged; only the loop moved.
+      //
+      //     The count is read for the step LINE before the pass runs, so the
+      //     line still precedes the work it announces on every path — including
+      //     a rule that throws inside it.
       const packageEntries = artifactPackages(result.data as Record<string, unknown>);
       if (packageEntries.length > 0) {
         if (!flags.json) {
           printStep(`Running author-time rules per package (${packageEntries.length})...`);
         }
-        const alreadyReported = new Set(findings.map(findingKey));
-        const perPackageErrors: Array<{ package: string } & typeof ruleErrors[number]> = [];
-        for (const pkg of packageEntries) {
-          const asStack = packageBodyAsStack(pkg.body, artifactPackageEntries);
-          const pkgFindings = runAuthoringRules('build', {
-            normalized: asStack,
-            parsed: asStack,
-            sduiManifest: resolveSduiManifest(),
-            loweredHookRefs: lowering.loweredHookRefs,
-          }).filter((f) => !alreadyReported.has(findingKey(f)));
-          for (const f of pkgFindings) alreadyReported.add(findingKey(f));
-          const split = splitBySeverity(pkgFindings);
-          ruleAdvisories = [
-            ...ruleAdvisories,
-            ...split.advisories.map((a) => ({ ...a, where: `package '${pkg.id}' — ${a.where}` })),
-          ];
-          perPackageErrors.push(
-            ...split.errors.map((e) => ({ ...e, package: pkg.id, where: `package '${pkg.id}' — ${e.where}` })),
-          );
-        }
+        const perPackage = runPerPackageAuthoringRules({
+          command: 'build',
+          parsed: result.data as Record<string, unknown>,
+          unionFindings: findings,
+          sduiManifest: resolveSduiManifest(),
+          loweredHookRefs: lowering.loweredHookRefs,
+        });
+        const perPackageErrors: Array<{ package: string } & typeof ruleErrors[number]> =
+          perPackage.errors;
+        ruleAdvisories = [...ruleAdvisories, ...perPackage.advisories];
         if (perPackageErrors.length > 0) {
           if (flags.json) {
             await emitJson(

packages/cli/src/commands/validate.ts

@@ -15,6 +15,9 @@ import {
 import { loadConfig, namedExportRejectionHints } from '../utils/config.js';
 import { lowerCallables } from '../utils/lower-callables.js';
 import { authoringRuleUnionStack } from '../utils/stack-collections.js';
+// [#18677] The per-package half of the author-time rule run, shared with
+// `os compile` — ⛔ the loop is not re-written here; see that module's header.
+import { artifactPackages, runPerPackageAuthoringRules } from '../utils/artifact-packages.js';
 import { runAuthoringRules, splitBySeverity, authoringRulesFor } from '@objectstack/lint';
 import { resolveSduiManifest } from '../utils/sdui-manifest.js';
 import { preflightRequiredCapabilities, renderCapabilityMessage } from '../utils/capability-preflight.js';
@@ -382,6 +385,70 @@ export default class Validate extends Command {
         this.exit(1);
       }
 
+      // 3a-ii. [ADR-0130 D4, #18677] The SAME rule table, once per PACKAGE —
+      //     the second half of the run above, and the half this door ran
+      //     without.
+      //
+      //     `os build` has run it since #16611; `os validate` ran the union
+      //     fold and stopped, importing neither `artifactPackages` nor
+      //     `packageBodyAsStack`. `compile.ts` step 3b-ii says what survives
+      //     the de-duplication is "exactly the set the union could not see" ⇒
+      //     that whole set was findings `os build` reported and this command
+      //     structurally could not. Same FALSE-CLEAN direction #17069 fixed one
+      //     layer up, and the worse door for it: the fast inner-loop check is
+      //     what an author runs BEFORE shipping, so its clean bill of health is
+      //     the strongest false assurance the three commands can give.
+      //
+      //     ⛔ Not a second copy of the loop — `runPerPackageAuthoringRules` is
+      //     the one the build door calls, so the de-duplication key, the
+      //     severity split and the `where` prefix cannot drift between the two
+      //     doors. That drift is the defect this step closes, one layer down.
+      //
+      //     The SEVERITY MAPPING is `os build`'s, unchanged and deliberately:
+      //     an `error` refuses (exit 1), an advisory joins `ruleAdvisories` and
+      //     rides `warningsSoFar()`. The card asked for the asymmetry, ⛔ not
+      //     for a severity judgement, and a per-package `error` is one
+      //     `os build` ALREADY refuses — so this narrows `os validate` to the
+      //     bar the command that ships already holds, never past it.
+      //
+      //     Skipped entirely for a stack with no `packages[]`: one package by
+      //     definition, already judged whole by the union run above.
+      const packageEntries = artifactPackages(result.data as Record<string, unknown>);
+      if (packageEntries.length > 0) {
+        if (!flags.json) {
+          printStep(`Running author-time rules per package (${packageEntries.length})...`);
+        }
+        const perPackage = runPerPackageAuthoringRules({
+          command: 'validate',
+          parsed: result.data as Record<string, unknown>,
+          unionFindings: findings,
+          sduiManifest: resolveSduiManifest(),
+          // [#16546] The same ref set the union run above was handed, so a
+          // per-package hook write-set finding reports at the same `path` the
+          // other two doors report it at.
+          loweredHookRefs: lowering.loweredHookRefs,
+        });
+        ruleAdvisories = [...ruleAdvisories, ...perPackage.advisories];
+        if (perPackage.errors.length > 0) {
+          if (flags.json) {
+            await emitJson({
+              valid: false,
+              errors: perPackage.errors,
+              warnings: warningsSoFar(),
+              conversions: conversionNotices,
+              duration: timer.elapsed(),
+            });
+            this.exit(1);
+          }
+          console.log('');
+          printError(
+            `Author-time rules failed inside the artifact's packages (${perPackage.errors.length} issue${perPackage.errors.length > 1 ? 's' : ''})`,
+          );
+          printAuthoringRuleErrors(perPackage.errors, { remedy: JSON_FULL_LIST_REMEDY });
+          this.exit(1);
+        }
+      }
+
       // 3b. [#3366] Installable-provider preflight — the shift-left of the
       //     `serve`-time capability check. `os validate` previously only checked
       //     the `requires` tokens against the vocabulary (ADR-0066), never

packages/cli/src/utils/artifact-packages.ts

@@ -24,8 +24,30 @@
  * slightly differently is how one entry comes to judge a different set of
  * packages than the other while both look right — so the functions moved here
  * unchanged and both entries call these.
+ *
+ * Since #18677 the module also carries the PASS those two seams exist to feed —
+ * {@link runPerPackageAuthoringRules} — for the same reason one layer out: the
+ * `os build` door ran it and the `os validate` door did not, and a second copy
+ * of the loop is how that asymmetry would come back.
  */
 
+import {
+  runAuthoringRules,
+  splitBySeverity,
+  type AuthoringCommand,
+  type AuthoringFinding,
+} from '@objectstack/lint';
+
+/**
+ * Identity of one finding, for the per-package de-duplication below.
+ *
+ * Moved here from `compile.ts` unchanged (#18677): the two doors must
+ * de-duplicate identically, or "the set the union could not see" means two
+ * different things depending on which command the author happened to run.
+ */
+const findingKey = (f: { rule: string; where: string; path: string; message: string }): string =>
+  [f.rule, f.where, f.path, f.message].join('\u0000');
+
 /**
  * The artifact's package entries, as `{ index, id, body }` (ADR-0130 D4).
  *
@@ -107,3 +129,90 @@ export function packageBodyAsStack(
 ): Record<string, unknown> {
   return { ...body, manifest: body, packages: artifactPackageEntries };
 }
+
+/**
+ * The author-time rule table, run ONCE PER PACKAGE and de-duplicated against a
+ * union run — the pass `os build` has run since #16611 and `os validate` did
+ * not (#18677).
+ *
+ * ## Why it lives here and not in one of the two commands
+ *
+ * It is the THIRD entry to owe the shape the module header describes, and the
+ * header's fence binds it: the only ways to reach `compile.ts`' loop from
+ * `validate.ts` are to import one oclif command from another — pulling the
+ * lowerer and the docs sweep into every `os validate` invocation — or to write
+ * a second copy. ⛔ The second copy is what must not happen, and here it would
+ * not be the `{index,id,body}` reading that drifted but the VERDICT: two loops
+ * choosing their own de-duplication key, their own severity split or their own
+ * `where` prefix is how one door comes to report a different set from the other
+ * while both look right. That is the defect #18677 is, one layer down.
+ *
+ * ## What the asymmetry was, measured
+ *
+ * `os build` ran this pass; `os validate` ran the union fold and stopped,
+ * importing neither seam above. `compile.ts`' own comment says what survives
+ * the de-duplication is "exactly the set the union could not see" ⇒ that whole
+ * set was findings `os build` reported and `os validate` structurally could
+ * not. The direction is FALSE-CLEAN, and on the command an author runs BEFORE
+ * shipping — the same direction and the same door #17069 fixed one layer up,
+ * which is why `authoringRuleUnionStack` being in both commands did not settle
+ * it. `packages/cli/test/build-json-advisory-parity.e2e.test.ts` already
+ * asserted "nothing rides in build's `warnings` that validate does not also
+ * report"; it stayed green because its fixture declares no `packages[]` at all,
+ * so the pass it would have caught never ran there.
+ *
+ * ## The de-duplication key is the caller's, and it is not perfect
+ *
+ * `findingKey` below is `compile.ts`' key, moved unchanged: `rule`, `where`,
+ * `path`, `message`. ⚠️ `path` is POSITIONAL, and a collection index in one
+ * package's own body is not the index the flattened top level gives the same
+ * item — so a finding on any package whose local index differs from its
+ * flattened one survives the filter as an ECHO of a union finding rather than
+ * as something the union could not see. Measured on `examples/app-multi-package`
+ * (2 packages, `crm_account.industry`): 1 survivor, 0 of them new. ⛔ Not fixed
+ * here — changing the key changes what `os build` reports, which is a separate
+ * decision from making the two doors agree, and agreeing IMPERFECTLY at one
+ * seam is strictly better than disagreeing at two. When it is fixed it is
+ * fixed once, for both commands, which is the property this module buys.
+ */
+export function runPerPackageAuthoringRules(run: {
+  /** Which door is asking — the same string its union run passed. */
+  command: AuthoringCommand;
+  /** The PARSED stack, as `artifactPackages` reads it. */
+  parsed: Record<string, unknown>;
+  /** The union run's findings, whose keys this pass de-duplicates against. */
+  unionFindings: readonly AuthoringFinding[];
+  sduiManifest?: unknown;
+  loweredHookRefs?: ReadonlySet<string>;
+}): {
+  /** How many package entries were walked — 0 means the pass did not run. */
+  packageCount: number;
+  errors: Array<{ package: string } & AuthoringFinding>;
+  advisories: AuthoringFinding[];
+} {
+  const artifactPackageEntries = run.parsed.packages;
+  const packageEntries = artifactPackages(run.parsed);
+  const errors: Array<{ package: string } & AuthoringFinding> = [];
+  const advisories: AuthoringFinding[] = [];
+  if (packageEntries.length === 0) return { packageCount: 0, errors, advisories };
+
+  const alreadyReported = new Set(run.unionFindings.map(findingKey));
+  for (const pkg of packageEntries) {
+    const asStack = packageBodyAsStack(pkg.body, artifactPackageEntries);
+    const pkgFindings = runAuthoringRules(run.command, {
+      normalized: asStack,
+      parsed: asStack,
+      sduiManifest: run.sduiManifest,
+      loweredHookRefs: run.loweredHookRefs,
+    }).filter((f) => !alreadyReported.has(findingKey(f)));
+    for (const f of pkgFindings) alreadyReported.add(findingKey(f));
+    const split = splitBySeverity(pkgFindings);
+    advisories.push(
+      ...split.advisories.map((a) => ({ ...a, where: `package '${pkg.id}' — ${a.where}` })),
+    );
+    errors.push(
+      ...split.errors.map((e) => ({ ...e, package: pkg.id, where: `package '${pkg.id}' — ${e.where}` })),
+    );
+  }
+  return { packageCount: packageEntries.length, errors, advisories };
+}