From ceb9774be6bc3c7f2ed83d4158ce18e3e646bde0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 19:30:59 +0000 Subject: [PATCH 1/2] fix(cli): `os validate` runs the per-package author-time rule pass `os build` already ran MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 — importing 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. False-clean, and on the worse door: the fast pre-flight is what an author runs before shipping. 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. After: both 4, same set, 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, an advisory joins `warnings`. So `os validate` is narrowed only to the bar the command that ships already holds, and nothing that builds today stops validating. Two pins, one per tier. `test/validate-per-package-authoring-seam.test.ts` (unit) ratchets that both doors call the shared pass, that neither names `packageBodyAsStack` itself, and that the pass cannot answer differently per door — with a non-vacuity case, because every single-package fixture in this suite is blind to the defect. That blindness is measured, not asserted: `test/build-json-advisory-parity.e2e.test.ts` already claims "nothing rides in build's `warnings` that validate does not also report" and stayed green through this defect, its fixtures declaring no `packages[]` at all. `test/validate-per-package-authoring-parity.test.ts` (integration) is the behavioural half over a two-package project, with a single-package control. Claude-Session: https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3 Co-authored-by: Claude --- ...677-validate-per-package-authoring-pass.md | 22 ++ packages/cli/src/commands/compile.ts | 51 ++-- packages/cli/src/commands/validate.ts | 67 +++++ packages/cli/src/utils/artifact-packages.ts | 109 ++++++++ ...idate-per-package-authoring-parity.test.ts | 260 ++++++++++++++++++ ...alidate-per-package-authoring-seam.test.ts | 223 +++++++++++++++ 6 files changed, 706 insertions(+), 26 deletions(-) create mode 100644 .changeset/18677-validate-per-package-authoring-pass.md create mode 100644 packages/cli/test/validate-per-package-authoring-parity.test.ts create mode 100644 packages/cli/test/validate-per-package-authoring-seam.test.ts diff --git a/.changeset/18677-validate-per-package-authoring-pass.md b/.changeset/18677-validate-per-package-authoring-pass.md new file mode 100644 index 00000000000..53b968962fe --- /dev/null +++ b/.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. diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 32b23f5fc57..eb05c5ea3d1 100644 --- a/packages/cli/src/commands/compile.ts +++ b/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).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); 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, + 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( diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index bafa54b07f4..340cec62539 100644 --- a/packages/cli/src/commands/validate.ts +++ b/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); + 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, + 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 diff --git a/packages/cli/src/utils/artifact-packages.ts b/packages/cli/src/utils/artifact-packages.ts index febc6ad4ede..c7740598214 100644 --- a/packages/cli/src/utils/artifact-packages.ts +++ b/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 { 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; + /** The union run's findings, whose keys this pass de-duplicates against. */ + unionFindings: readonly AuthoringFinding[]; + sduiManifest?: unknown; + loweredHookRefs?: ReadonlySet; +}): { + /** 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 }; +} diff --git a/packages/cli/test/validate-per-package-authoring-parity.test.ts b/packages/cli/test/validate-per-package-authoring-parity.test.ts new file mode 100644 index 00000000000..e4a24cdf2c2 --- /dev/null +++ b/packages/cli/test/validate-per-package-authoring-parity.test.ts @@ -0,0 +1,260 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #18677, the BEHAVIOURAL half — `os validate` and `os build` report the same + * author-time advisory set for a MULTI-PACKAGE project. + * + * `os build` ran the rule table a second time, once per `artifactPackages(…)` + * entry; `os validate` ran the union fold and stopped. By `compile.ts`' own + * description the survivors of that pass are "exactly the set the union could + * not see" ⇒ that whole set was findings `os build` reported and `os validate` + * structurally could not. FALSE-CLEAN, on the fast pre-flight an author runs + * before shipping. + * + * ## Why a NEW fixture and not an assertion on the existing parity file + * + * `test/build-json-advisory-parity.e2e.test.ts` already asserts, in so many + * words, that "nothing rides in build's `warnings` that validate does not also + * report" — and it stayed green through this entire defect. Its fixtures are + * SINGLE-package: they declare no `packages[]`, `artifactPackages` returns `[]`, + * the per-package pass is skipped on both doors, and the two agree for the wrong + * reason. The claim was right and the fixture was blind to the one shape that + * can falsify it. ⇒ what this file adds is the shape, not a new claim. + * + * Measured on `origin/main` 09e16a574 over `examples/app-multi-package` (the + * repo's own two-package fixture), 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 + * + * ## The survivor on that fixture is an ECHO, and this file says so + * + * ⚠️ The de-duplication key includes the POSITIONAL `path`, and a collection + * index inside one package's own body is not the index the flattened top level + * gives the same item — so on `examples/app-multi-package` what survives is the + * union's own `crm_account.industry` finding re-reported at the package-local + * index, not something the union could not see. That is a separate defect in + * the key (filed, not fixed here — fixing it changes what `os build` reports, + * which is a different decision from making the two doors agree). It does not + * weaken these pins: whatever the pass produces, the assertion is that BOTH + * doors produce it, so the pins measure parity rather than the survivor's + * pedigree. The `where` prefix (`package '' — …`) is what identifies a + * per-package finding here, and it is the pass's own, not this file's guess. + * + * ## Tier + * + * SPAWNS the CLI ⇒ INTEGRATION tier by `packages/cli/vitest-tiers.ts`' predicate + * (`childProcess` + `helperCliOrTsx`). The filename deliberately carries no + * `.e2e` segment, so by the orthogonal NIGHTLY cut it is a QUEUE-tier file — the + * combination that tiers module names as deliberate. Its unit-tier sibling is + * `validate-per-package-authoring-seam.test.ts`. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFile } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { CLI, TSX, childEnv } from './helpers/serve-process.js'; + +interface Run { + code: number; + stdout: string; + stderr: string; +} + +function runCli(args: string[], cwd: string): Promise { + return new Promise((resolvePromise) => { + execFile( + TSX, + [CLI, ...args], + { cwd, maxBuffer: 16 * 1024 * 1024, env: childEnv({ NO_COLOR: '1' }) }, + (err, stdout, stderr) => { + resolvePromise({ + code: err ? (typeof (err as { code?: unknown }).code === 'number' ? (err as unknown as { code: number }).code : 1) : 0, + stdout: String(stdout), + stderr: String(stderr), + }); + }, + ); + }); +} + +function payloadOf(run: Run, label: string): Record { + try { + return JSON.parse(run.stdout) as Record; + } catch { + throw new Error(`${label}: stdout was not one JSON document (exit ${run.code})\n${run.stdout}\n${run.stderr}`); + } +} + +/** The `where` prefix `runPerPackageAuthoringRules` puts on every finding it raises. */ +const PER_PACKAGE_WHERE = /^package '[^']+' — /; + +const perPackageWarnings = (warnings: unknown[]): string[] => + warnings + .filter((w): w is { where: string } => typeof (w as { where?: unknown })?.where === 'string') + .map((w) => w.where) + .filter((where) => PER_PACKAGE_WHERE.test(where)); + +/** + * The two package bodies, mirroring `examples/app-multi-package`: an App package + * owning `pp_account` and publishing the navigation container, plus a module + * owning `pp_order`, whose `account` lookup points at the sibling's object — + * legal under ADR-0130 §1.5, and the point of the shape. + */ +const CONFIG_MULTI = ` +const coreManifest = { + id: 'com.example.ppparity.core', name: 'ppparity core', namespace: 'pp', + version: '1.0.0', type: 'app', engines: { protocol: '^17' }, +}; +const coreObjects = [{ + name: 'pp_account', label: 'Account', pluralLabel: 'Accounts', sharingModel: 'private', + fields: { + name: { name: 'name', type: 'text', label: 'Account Name', required: true }, + industry: { name: 'industry', type: 'text', label: 'Industry' }, + }, +}]; +const coreApps = [{ + name: 'pp_crm', label: 'PP CRM', + navigation: [{ + id: 'sales_group', type: 'group', label: 'Sales', + children: [{ id: 'nav_accounts', type: 'object', objectName: 'pp_account', label: 'Accounts' }], + }], +}]; + +const ordersManifest = { + id: 'com.example.ppparity.orders', name: 'ppparity orders', namespace: 'pp', + version: '1.0.0', type: 'module', engines: { protocol: '^17' }, + dependencies: { 'com.example.ppparity.core': '^1.0.0' }, +}; +const ordersObjects = [{ + name: 'pp_order', label: 'Order', pluralLabel: 'Orders', sharingModel: 'private', + fields: { + name: { name: 'name', type: 'text', label: 'Order Number', required: true }, + account: { name: 'account', type: 'lookup', label: 'Account', reference: 'pp_account' }, + }, +}]; + +export default { + // The ARTIFACT's own identity: \`preserve\` is additive, so the singular + // manifest is still picked by the default 'last' rule (ADR-0019 D1) and + // carries manifest fields ONLY — \`ManifestSchema\` is strict, and a + // collection key here is refused by name. + manifest: coreManifest, + objects: [...ordersObjects, ...coreObjects], + apps: [...coreApps], + // …and the per-package view the runtime registers from (ADR-0130 D4/D5). Each + // entry's \`manifest\` is that package ASSEMBLED — its manifest fields with the + // collections it owns written over them — which is the superset + // \`packageBodyAsStack\` reads back as one package's stack. + packages: [ + { manifest: { ...ordersManifest, objects: ordersObjects } }, + { manifest: { ...coreManifest, objects: coreObjects, apps: coreApps } }, + ], +}; +`; + +/** + * The CONTROL: one package, no `packages[]`. The per-package pass is skipped on + * both doors, so parity here holds for a reason that has nothing to do with the + * fix — which is precisely how the defect survived the existing parity file. + * Without this case, "the two payloads agree" cannot be read as a measurement of + * anything. + */ +const CONFIG_SINGLE = ` +export default { + manifest: { + id: 'com.example.ppsingle', name: 'ppsingle', namespace: 'ps', + version: '1.0.0', type: 'app', engines: { protocol: '^17' }, + }, + objects: [{ + name: 'ps_thing', label: 'Thing', pluralLabel: 'Things', sharingModel: 'private', + fields: { + name: { name: 'name', type: 'text', label: 'Name', required: true }, + unused: { name: 'unused', type: 'text', label: 'Unused' }, + }, + }], + apps: [{ + name: 'ps_app', label: 'PS App', + navigation: [{ id: 'nav_things', type: 'object', objectName: 'ps_thing', label: 'Things' }], + }], +}; +`; + +const dirs = { multi: '', single: '' }; + +function plant(config: string): string { + const dir = mkdtempSync(join(tmpdir(), 'os-ppparity-')); + mkdirSync(join(dir, 'src'), { recursive: true }); + writeFileSync(join(dir, 'objectstack.config.ts'), config, 'utf8'); + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ name: 'ppparity-fixture', private: true, type: 'module' }, null, 2), + 'utf8', + ); + return dir; +} + +describe('#18677 — `os validate` and `os build` report the same per-package advisory set', () => { + beforeAll(() => { + dirs.multi = plant(CONFIG_MULTI); + dirs.single = plant(CONFIG_SINGLE); + }); + + afterAll(() => { + for (const dir of Object.values(dirs)) if (dir) rmSync(dir, { recursive: true, force: true }); + }); + + it('the multi-package fixture reaches the pass at all — `os build` raises a per-package finding', async () => { + // Asserted BEFORE any claim about parity: a fixture that never reaches the + // per-package pass makes every comparison below vacuous, which is the exact + // way this defect stayed invisible. + const build = await runCli(['build', '--json'], dirs.multi); + expect(build.code, `os build --json failed:\n${build.stdout}${build.stderr}`).toBe(0); + const warnings = payloadOf(build, 'os build --json').warnings as unknown[]; + expect(perPackageWarnings(warnings).length).toBeGreaterThan(0); + }, 180_000); + + it('`os validate` reports every per-package advisory `os build` does', async () => { + // The pin. On `origin/main` 09e16a574 this read `build: 1, validate: 0`. + const build = await runCli(['build', '--json'], dirs.multi); + const validate = await runCli(['validate', '--json'], dirs.multi); + expect(build.code, `os build --json failed:\n${build.stdout}${build.stderr}`).toBe(0); + expect(validate.code, `os validate --json failed:\n${validate.stdout}${validate.stderr}`).toBe(0); + + const bw = payloadOf(build, 'os build --json').warnings as unknown[]; + const vw = payloadOf(validate, 'os validate --json').warnings as unknown[]; + + const inValidate = new Set(perPackageWarnings(vw)); + const missingFromValidate = perPackageWarnings(bw).filter((w) => !inValidate.has(w)); + expect( + missingFromValidate, + 'these per-package findings ride `os build` and `os validate` cannot see them — the #18677 false-clean set', + ).toEqual([]); + }, 180_000); + + it('…and nothing per-package rides `os validate` that `os build` does not report either', async () => { + // The reverse end. Parity is an equality, and a validate that over-reports + // is its own defect — an author fixing a finding the command that SHIPS + // never raises. + const build = await runCli(['build', '--json'], dirs.multi); + const validate = await runCli(['validate', '--json'], dirs.multi); + const bw = payloadOf(build, 'os build --json').warnings as unknown[]; + const vw = payloadOf(validate, 'os validate --json').warnings as unknown[]; + + const inBuild = new Set(perPackageWarnings(bw)); + expect(perPackageWarnings(vw).filter((w) => !inBuild.has(w))).toEqual([]); + }, 180_000); + + it('CONTROL — a single-package project raises NO per-package finding on either door', async () => { + // "Present" must be distinguishable from "always present": the prefix this + // file matches on is not something either command emits unconditionally. + const build = await runCli(['build', '--json'], dirs.single); + const validate = await runCli(['validate', '--json'], dirs.single); + expect(build.code, `os build --json failed:\n${build.stdout}${build.stderr}`).toBe(0); + expect(validate.code, `os validate --json failed:\n${validate.stdout}${validate.stderr}`).toBe(0); + expect(perPackageWarnings(payloadOf(build, 'os build --json').warnings as unknown[])).toEqual([]); + expect(perPackageWarnings(payloadOf(validate, 'os validate --json').warnings as unknown[])).toEqual([]); + }, 180_000); +}); diff --git a/packages/cli/test/validate-per-package-authoring-seam.test.ts b/packages/cli/test/validate-per-package-authoring-seam.test.ts new file mode 100644 index 00000000000..cb45450cf93 --- /dev/null +++ b/packages/cli/test/validate-per-package-authoring-seam.test.ts @@ -0,0 +1,223 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #18677 — `os build` ran the author-time rule table a SECOND time, once per + * `artifactPackages(…)` entry with `packageBodyAsStack(…)` as resolution + * context; `os validate` ran the union fold and stopped. `compile.ts`' own + * comment says what survives that 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. FALSE-CLEAN, on the command an author + * runs BEFORE shipping — the same direction #17069 fixed one layer up, which is + * why `authoringRuleUnionStack` landing in both commands did not settle it. + * + * ## What this file pins, and what its sibling pins + * + * This one is the SEAM: the pass exists once, both doors reach it, and it + * cannot answer differently depending on which door asked. It spawns nothing + * and boots no kernel, so it is UNIT tier (`packages/cli/vitest-tiers.ts`) and + * gates the merge queue. `validate-per-package-authoring-parity.test.ts` is the + * behavioural half — it runs the two real commands over a two-package project + * and compares their payloads — and lands in the INTEGRATION tier because it + * spawns the CLI. + * + * ## Why a source ratchet and not only a behavioural assertion + * + * Because the defect was structural, not numeric: the per-package pass was + * absent from one command's source, and on every single-package fixture in this + * suite that absence is INVISIBLE — `artifactPackages` returns `[]`, the pass is + * skipped, and both doors agree for the wrong reason. That is exactly why + * `test/build-json-advisory-parity.e2e.test.ts`' standing assertion ("nothing + * rides in build's `warnings` that validate does not also report") stayed green + * through this defect: its fixture declares no `packages[]` at all. A ratchet on + * the source is the reading that does not depend on a fixture happening to carry + * the shape — the same instrument `packages/lint/src/authoring-rule-wiring.test.ts` + * uses on these same two files, and for the same reason. + * + * ⛔ The ratchet is NOT "the file mentions the helper". It asserts the two + * spellings that can drift: the helper is CALLED, and the loop's own seam + * (`packageBodyAsStack`) appears in NEITHER command — because a command that + * names it is a command that has started writing a second copy of the loop, + * which is the failure `utils/artifact-packages.ts`' header forbids by name. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { defineStack, composeStacks } from '@objectstack/spec'; +import { artifactPackages, runPerPackageAuthoringRules } from '../src/utils/artifact-packages.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const COMMANDS = resolve(HERE, '../src/commands'); +const sourceOf = (file: string) => readFileSync(join(COMMANDS, file), 'utf8'); + +/** The two doors that hold an ARTIFACT to the author-time bar. */ +const DOORS = ['compile.ts', 'validate.ts'] as const; + +/** + * A two-package artifact, mirroring `examples/app-multi-package`: an App package + * owning `crm_account` plus a module owning `crm_order`, whose `account` lookup + * points at the sibling's object. `manifest: 'preserve'` is the one composition + * that produces `packages[]` (ADR-0130 D4) — without it there is no per-package + * pass to run and this file would pass by vacuity, which the last case measures. + */ +function twoPackageArtifact(): Record { + const core = defineStack({ + manifest: { + id: 'com.example.seam.core', + name: 'Seam Core', + namespace: 'crm', + version: '1.0.0', + type: 'app', + engines: { protocol: '^17' }, + }, + objects: [ + { + name: 'crm_account', + label: 'Account', + pluralLabel: 'Accounts', + sharingModel: 'private', + fields: { + name: { name: 'name', type: 'text', label: 'Account Name', required: true }, + industry: { name: 'industry', type: 'text', label: 'Industry' }, + }, + }, + ], + // The App package publishes the navigation container, exactly as + // `examples/app-multi-package` does. ⛔ Not decoration: `field-no-consumers` + // returns early on a stack whose only collection is `objects` (its + // `hasConsumerRoot` guard), so without a consumer root on at least one + // package body the per-package pass produces nothing and the equality + // below would hold between two empty lists. The non-vacuity case is what + // keeps that honest. + apps: [ + { + name: 'seam_crm', + label: 'Seam CRM', + navigation: [ + { + id: 'sales_group', + type: 'group', + label: 'Sales', + children: [ + { id: 'nav_accounts', type: 'object', objectName: 'crm_account', label: 'Accounts' }, + ], + }, + ], + }, + ], + }); + const orders = defineStack({ + manifest: { + id: 'com.example.seam.orders', + name: 'Seam Orders', + namespace: 'crm', + version: '1.0.0', + type: 'module', + engines: { protocol: '^17' }, + dependencies: { 'com.example.seam.core': '^1.0.0' }, + }, + objects: [ + { + name: 'crm_order', + label: 'Order', + pluralLabel: 'Orders', + sharingModel: 'private', + fields: { + name: { name: 'name', type: 'text', label: 'Order Number', required: true }, + account: { name: 'account', type: 'lookup', label: 'Account', reference: 'crm_account' }, + }, + }, + ], + }); + return composeStacks([orders, core], { manifest: 'preserve' }) as unknown as Record; +} + +describe('#18677 — the per-package author-time pass is ONE seam both doors reach', () => { + it('both `os build` and `os validate` CALL the shared pass', () => { + for (const door of DOORS) { + expect( + sourceOf(door), + `${door} must run the per-package author-time pass — its absence is the #18677 false-clean gap`, + ).toMatch(/\brunPerPackageAuthoringRules\s*\(/); + } + }); + + it('⛔ neither door names `packageBodyAsStack` — that spelling IS a second copy of the loop', () => { + // `utils/artifact-packages.ts`' header forbids the second copy by name: what + // drifts between two hand-written loops is the VERDICT (the de-duplication + // key, the severity split, the `where` prefix), not the package reading. + for (const door of DOORS) { + expect( + sourceOf(door).replace(/`packageBodyAsStack`/g, ''), + `${door} reaches into the loop's own seam — call the shared pass instead`, + ).not.toMatch(/\bpackageBodyAsStack\s*\(/); + } + }); + + it('the pass answers IDENTICALLY for the two doors — same survivors, same order', () => { + // The invariant the asymmetry violated. Both modes run all 45 registered + // rules (`commands: ALL` for every gating entry), so a door-dependent answer + // here would mean the two commands hold one artifact to two bars. + const parsed = twoPackageArtifact(); + expect(artifactPackages(parsed).length, 'fixture must carry `packages[]`').toBe(2); + + const forDoor = (command: 'build' | 'validate') => + runPerPackageAuthoringRules({ command, parsed, unionFindings: [] }); + + const build = forDoor('build'); + const validate = forDoor('validate'); + expect(validate.packageCount).toBe(build.packageCount); + expect(validate.errors).toEqual(build.errors); + expect(validate.advisories).toEqual(build.advisories); + }); + + it('is NON-VACUOUS — the pass really produced findings on this fixture', () => { + // Without this, the equality above is satisfied by two empty lists and the + // whole file passes while measuring nothing. Deliberately asserted on the + // COUNT and on the `where` prefix the pass owns, not on a rule id: which + // rule fires is `lint/authoring-rules.ts`' business and may change. + const parsed = twoPackageArtifact(); + const run = runPerPackageAuthoringRules({ command: 'validate', parsed, unionFindings: [] }); + const all = [...run.errors, ...run.advisories]; + expect(all.length).toBeGreaterThan(0); + expect(all.every((f) => /^package '/.test(f.where))).toBe(true); + }); + + it('de-duplicates against the union run it is handed', () => { + // The filter `compile.ts` described and this pass now owns. Handing it its + // OWN output as the union run must empty it — the property that makes + // "exactly the set the union could not see" mean anything at all. + const parsed = twoPackageArtifact(); + const first = runPerPackageAuthoringRules({ command: 'validate', parsed, unionFindings: [] }); + const raw = [...first.errors, ...first.advisories]; + expect(raw.length).toBeGreaterThan(0); + // The pass prefixes `where`; de-duplication happens on the UNPREFIXED + // finding, so the union set is rebuilt by stripping the prefix back off. + const asUnion = raw.map((f) => ({ ...f, where: f.where.replace(/^package '[^']*' — /, '') })); + const second = runPerPackageAuthoringRules({ command: 'validate', parsed, unionFindings: asUnion }); + expect([...second.errors, ...second.advisories]).toEqual([]); + }); + + it('a stack with no `packages[]` skips the pass entirely — `packageCount` 0', () => { + // One package by definition: the union run above already judged it whole, + // and this is why every single-package fixture in this suite was blind to + // the defect. + const single = defineStack({ + manifest: { id: 'com.example.seam.single', name: 'Single', version: '1.0.0', type: 'app' }, + objects: [ + { + name: 'solo_thing', + label: 'Thing', + pluralLabel: 'Things', + sharingModel: 'private', + fields: { name: { name: 'name', type: 'text', label: 'Name', required: true } }, + }, + ], + }) as unknown as Record; + const run = runPerPackageAuthoringRules({ command: 'validate', parsed: single, unionFindings: [] }); + expect(run.packageCount).toBe(0); + expect(run.errors).toEqual([]); + expect(run.advisories).toEqual([]); + }); +}); From e62c44e48bbde25eb01109e3f1383c44df880796 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 20:18:53 +0000 Subject: [PATCH 2/2] fix(cli): close the #18491 NOT_A_GATE parity-gap entry the per-package pass was recorded in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test/validate-build-gate-parity.test.ts` holds a CLOSED roster: every bare identifier either command calls must land in exactly one of its three ledgers. Wiring `runPerPackageAuthoringRules` into both doors left it unclassified, so the roster reddened — correctly, and on the one shard that runs `packages/cli` (`Test Core (4/6)`, step "Run this shard's tests"). ⛔ Not a refusal: nothing in the suite was newly rejected by `os validate`, so the `Clause-②: no` declaration is untouched by this red. The two failures were "every call site in compile.ts and validate.ts is classified" (1 unclassified name) and "no ledger entry is stale" (3 names no command uses any more). What the ledgers now say: * `runPerPackageAuthoringRules` joins SHARED_NON_REGISTRY_GATES. It cannot become a registry rule: a registry rule is handed ONE stack, and this pass is the thing that DECIDES which stack — the artifact sliced per package (ADR-0130 D4/D5). Its row buys a real assertion for free, because `it.each(SHARED_NON_REGISTRY_GATES)` asserts both commands run it. * The #18491 entry "Input to the compile-only per-package rule walk — a real parity gap, reported not closed" is DELETED, not reworded. That entry was right and is now spent; a ledger row that outlives its finding is how a closed gap reads as an open one. * `artifactPackages` survives on its own reason — both commands read it to COUNT the packages for the step line. `packageBodyAsStack` left the command files with the loop, as did `findingKey` and the `new Set(…)` de-duplication. packages/cli unit tier: 213 files / 3041 tests, all passing. Claude-Session: https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3 Co-authored-by: Claude --- .../test/validate-build-gate-parity.test.ts | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/packages/cli/test/validate-build-gate-parity.test.ts b/packages/cli/test/validate-build-gate-parity.test.ts index 1f66896eb62..840145e55f5 100644 --- a/packages/cli/test/validate-build-gate-parity.test.ts +++ b/packages/cli/test/validate-build-gate-parity.test.ts @@ -99,6 +99,21 @@ const SHARED_NON_REGISTRY_GATES: readonly string[] = [ // widening the pattern to cover `find*` would have moved the blind spot // rather than closed it. 'checkProtocolVersionGap', + // [#18677] The author-time rule table, run once per `packages[]` entry with + // `packageBodyAsStack` as resolution context and de-duplicated against the + // union run. Not a registry rule and it cannot become one: a registry rule is + // handed ONE stack, and this pass is the thing that DECIDES which stack — the + // artifact sliced per package (ADR-0130 D4/D5), which is the answer the + // runtime will live with. + // + // ⭐ This row is the #18491 NOT_A_GATE entry CLOSED. That entry read "a real + // parity gap, reported not closed", and it was right: `os build` ran this + // pass and `os validate` did not, so `os build` judged something + // `os validate` structurally could not, in the false-clean direction. The + // entry is deleted rather than reworded — the gap it recorded is gone, and a + // ledger row that outlives its finding is how a closed gap reads as an open + // one. + 'runPerPackageAuthoringRules', ]; /** @@ -152,15 +167,16 @@ const NOT_A_GATE: Readonly> = { 'loadConfig', 'ObjectStackDefinitionSchema', ], - // ⚠️ [#18491] These two are compile-only, and so is the SECOND - // `runAuthoringRules` run they feed. That run is not covered by the roster - // above and is not covered by the union fold either: `compile.ts`'s own - // comment says what survives its de-duplication is "exactly the set the union - // could not see". So `os build` judges something `os validate` does not, in - // the false-clean direction. Recorded here rather than silently wired up — - // wiring a real gate into the other door is a decision, not a test fix. - 'Input to the compile-only per-package rule walk — a real parity gap, reported not closed (see the note above this entry)': - ['artifactPackages', 'packageBodyAsStack'], + // [#18677] Was "Input to the compile-only per-package rule walk — a real + // parity gap, reported not closed", carrying `artifactPackages` and + // `packageBodyAsStack`. That gap is CLOSED: the walk is + // `runPerPackageAuthoringRules` in SHARED_NON_REGISTRY_GATES above, run by + // both doors. `packageBodyAsStack` left this file with the loop — it is read + // inside the shared pass now, by neither command directly — and + // `artifactPackages` stays, on its own reason, because it is no longer input + // to a gap: both commands read it to COUNT the packages for the step line. + 'Reads the artifact\'s `packages[]` for a count both commands print; the pass that judges them is a gate above': + ['artifactPackages'], 'Presentation — renders, formats or serialises a verdict something else reached; judges nothing': [ 'printHeader', @@ -190,12 +206,10 @@ const NOT_A_GATE: Readonly> = { 'isExitSignal', 'isReportedError', 'cleanupOldRuntimeBundles', - 'findingKey', 'warningsSoFar', ], 'Not ours — a Node builtin, a global, an oclif base or a third-party namespace': [ 'dirname', - 'Set', 'String', 'path', 'fs',