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-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', 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([]); + }); +});