diff --git a/.changeset/18779-per-package-dedup-positional-key.md b/.changeset/18779-per-package-dedup-positional-key.md new file mode 100644 index 00000000000..e79004b387a --- /dev/null +++ b/.changeset/18779-per-package-dedup-positional-key.md @@ -0,0 +1,58 @@ +--- +'@objectstack/cli': patch +--- + +The per-package author-time de-duplication key ignores the top-level collection index, so a package-local finding no longer survives as an echo of the union finding it duplicates + +`runPerPackageAuthoringRules` runs the author-time rule table once per +`packages[]` entry and drops anything the union run already reported. Its key +was `rule` + `where` + `path` + `message`, and `path` is **positional**: a +package body re-bases every collection from 0, while the flattened union numbers +that same entry wherever `authoringRuleUnionStack` placed it. +`objects[0].fields.industry` and `objects[1].fields.industry` are ONE finding +under two spellings, so the `Set` never matched them and the echo survived the +filter that exists to remove it. + +Measured on `origin/main` a43b9d0654 over the repo's own two-package fixture +`examples/app-multi-package`, at every door, before and after: + +| | before | after | +|---|---|---| +| `os build --json` | warnings 4, exit 0 | warnings 3, exit 0 | +| `os validate --json` | warnings 4, exit 0 | warnings 3, exit 0 | +| `os lint --json` | total 4, failing 0, exit 0 | total 3, failing 0, exit 0 | +| `os lint --json --strict` | total 4, failing 4, exit 1 | total 3, failing 3, exit 1 | + +The one warning that stops being reported is `field-no-consumers` on +`crm_account.industry` re-reported at the package-local index — the union run's +own finding, printed a second time. Its twin is still reported, which is why no +verdict moves. + +**No input's verdict changes, and that is structural rather than a property of +this fixture.** Every finding the de-duplication drops has, by construction, a +finding carrying the same key already in the reported set: the seed is the union +run's findings, which every door reports, and it grows only with per-package +findings that themselves survived. So a door's refusal cannot flip — `os build` +already exits 1 on a union error before this pass runs, and `os lint --strict` +fails on `errors + warnings`, a count that could only reach zero if the twin +went unreported too. + +Only the **top-level** index is neutralised. Nested positions (`.indexes[1]`, +`.columns[0]`) address the author's own document and read identically in both +views, so they stay in the key and keep discriminating. A finding's own `path` +is never modified — every door still prints the location it always printed. + +What this does **not** buy: the key becomes position-insensitive, not +collision-proof. Two entries that render the same `where` still share a key, +exactly as they already did whenever their indices happened to match. Measured +over every example stack in this repo that parses today (`app-multi-package`'s +built artifact, `app-crm`, `app-showcase`, `app-todo`), 45 registry rules +produced 103 findings and 103 distinct neutralised keys — zero collisions. + +Also corrected: the sentence "what survives the filter is exactly the set the +union could not see", which was false for as long as the key was positional and +had been copied from `compile.ts` into the `os validate` and `os lint` doors as +each was wired. It is now stated at the bound the pass can actually hold, in +every file that carried it. + +Clause-②: no diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 5d282be6807..0f708f8fa9c 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -452,8 +452,22 @@ export default class Compile extends Command { // DE-DUPLICATED against the union run, because the union contains // every package's items: without this, a two-package project reports // every finding twice and the author cannot tell a real per-package - // finding from an echo. What survives the filter is exactly the set - // the union could not see. + // finding from an echo. + // + // ⚠️ [#18779] This comment used to end "What survives the filter is + // exactly the set the union could not see", and that was FALSE for + // as long as the de-duplication key carried the POSITIONAL `path`: + // a package body re-bases its collections from 0, so one finding got + // two keys and its echo survived the very filter described here. The + // sentence was quoted as authority by #18677 and #18778 without the + // definition being opened, and copied into the `os validate` and + // `os lint` doors as each was wired. `findingKey` now neutralises + // the top-level collection index, so what survives is the set of + // per-package findings no union finding already carried under the + // same rule, `where`, message and non-top-level position. ⛔ Do not + // re-inflate that to "exactly the set the union could not see" — + // `utils/artifact-packages.ts` states the bound and why it is + // narrower than that sentence. // // [#16611] Each package's stack is handed the artifact's `packages[]` // as RESOLUTION CONTEXT — see `packageBodyAsStack`. The list read here diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index 5b2eb1af023..1ccf9274a96 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -684,10 +684,19 @@ export function lintConfig(config: any, opts: LintConfigOptions = {}): LintIssue // // The second half of the run above, and the half THIS door ran without. // `os build` has run it since #16611 and `os validate` since #18677; `os - // lint` ran the union fold and stopped. `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. + // lint` ran the union fold and stopped. Every finding this pass yields is + // therefore one `os build` reported and this command structurally could not. + // + // ⚠️ [#18779] This paragraph used to size that gap by quoting `compile.ts` + // step 3b-ii — "exactly the set the union could not see" — and that sentence + // was FALSE when it was copied here: the de-duplication key carried the + // POSITIONAL `path`, so a package-local finding and its flattened twin got + // two keys and the ECHO survived. Part of every survivor set was therefore + // something this door's own union run ALREADY reported. The key was + // corrected in `utils/artifact-packages.ts`; the gap this door closed is + // real and its direction is unchanged, but ⛔ do not re-derive its size from + // that sentence — it was quoted, never measured, by the two cards that + // wired the second and third doors. // // ⚠️ The reading that hid it for two cards is the one the imports above // invite: this file DOES call `artifactPackages` and `packageBodyAsStack` — @@ -727,8 +736,10 @@ export function lintConfig(config: any, opts: LintConfigOptions = {}): LintIssue // walks. ⛔ Not `stack` — that would judge un-lowered package bodies here // and lowered ones there, which is #16095 one layer in. parsed: lowered, - // De-duplicated against the run above, on the UNPREFIXED finding, so what - // reaches the list below is the set the union could not see. + // De-duplicated against the run above, on the UNPREFIXED finding — so what + // reaches the list below is what that run did not already carry under the + // same rule, `where`, message and non-top-level position (#18779; the key + // used to compare the top-level index too, and let the echo through). unionFindings, sduiManifest: opts.sduiManifest, loweredHookRefs, diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index 340cec62539..dd1947b1a97 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -391,13 +391,23 @@ export default class Validate extends Command { // // `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. + // `packageBodyAsStack`. Every finding this pass yields is therefore + // one `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. + // + // ⚠️ [#18779] This step used to size that gap by quoting `compile.ts` + // step 3b-ii — "exactly the set the union could not see" — and that + // sentence was FALSE when it was copied here: the de-duplication key + // carried the POSITIONAL `path`, so a package-local finding and its + // flattened twin got two keys and the ECHO survived. Part of every + // survivor set was therefore something THIS door's own union run + // already reported. The key was corrected in + // `utils/artifact-packages.ts`; the gap this step closed is real and + // its direction is unchanged, but ⛔ do not re-derive its size from + // that sentence — it was quoted, never measured. // // ⛔ Not a second copy of the loop — `runPerPackageAuthoringRules` is // the one the build door calls, so the de-duplication key, the diff --git a/packages/cli/src/utils/artifact-packages.ts b/packages/cli/src/utils/artifact-packages.ts index af0f3a0554c..0b93d661b8f 100644 --- a/packages/cli/src/utils/artifact-packages.ts +++ b/packages/cli/src/utils/artifact-packages.ts @@ -46,15 +46,62 @@ import { type AuthoringFinding, } from '@objectstack/lint'; +/** + * The leading `collection[N]` of a finding path — the ONE coordinate that + * differs between the two views of a single finding. `objects[3].fields.x` + * matches `objects[3]`; `manifest.namespace` matches nothing. + */ +const TOP_LEVEL_COLLECTION_INDEX = /^([A-Za-z_][A-Za-z0-9_]*)\[\d+\]/; + /** * 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. + * Moved here from `compile.ts` unchanged (#18677); its POSITIONAL half was + * corrected here (#18779). All three doors must de-duplicate identically, or + * what survives the filter means a different thing depending on which command + * the author happened to run. + * + * ## Why the top-level collection index is neutralised (#18779) + * + * `rule`, `where` and `message` say WHICH finding this is; `path` says where + * it sits. The inherited key used `path` raw — and `path` is positional, so + * one finding judged twice got two keys and the `Set` below never matched + * them. A package body re-bases every collection from 0, while the flattened + * union numbers that same entry wherever `authoringRuleUnionStack` placed it: + * `objects[0].fields.industry` (package-local) and `objects[1].fields.industry` + * (union) are ONE finding under two spellings. Measured on + * `examples/app-multi-package` before this landed — 1 survivor, 1 echo, 0 + * genuinely new, and `os build` printed "4 author-time warning(s)" for 3 + * distinct ones. The de-duplication exists precisely so that "the author + * cannot tell a real per-package finding from an echo" would stop being true, + * and the positional key is why it stayed true. + * + * ⛔ The rewrite touches the KEY only — a finding's own `path` is never + * modified, so every door still prints the positional location it always + * printed. And only the TOP-LEVEL index: nested positions (`.indexes[1]`, + * `.columns[0]`) address the author's own document and read identically in + * both views, so they stay in the key and keep discriminating. + * + * ⛔ Not `nameKeyFindingPath` (`@objectstack/lint`'s runtime-gate rewrite of + * this same coordinate), for the reason that function's own docblock records: + * it is "Applied AFTER the differential, not before it … two stored items that + * (illegitimately) share a name must not have their distinct findings merged + * or cancelled by the rewrite". A de-duplication key IS that differential, so + * name-keying is the one place it rules itself out. Two further readings from + * the same docblock: its key set is DERIVED and holds `objects`, `permissions` + * and `books` today, so it would leave every other collection's echo standing, + * and it is "Exported for the pin, not for callers" — it sits on neither of + * that package's entries. + * + * ⚠️ What this does NOT buy, written down so the next reader does not + * re-inflate it: the key becomes position-insensitive, ⛔ not collision-proof. + * Two entries that render the same `where` — an illegitimate duplicate name — + * still share a key, exactly as they already did whenever their indices + * matched too. The claim the pass below is entitled to make is stated there, + * and it is narrower than "exactly the set the union could not see". */ const findingKey = (f: { rule: string; where: string; path: string; message: string }): string => - [f.rule, f.where, f.path, f.message].join('\u0000'); + [f.rule, f.where, f.path.replace(TOP_LEVEL_COLLECTION_INDEX, '$1[]'), f.message].join('\u0000'); /** * The artifact's package entries, as `{ index, id, body }` (ADR-0130 D4). @@ -161,30 +208,55 @@ export function packageBodyAsStack( * ## 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. + * importing neither seam above. Every finding this pass yields was therefore + * one `os build` reported and `os validate` structurally could not — the + * direction is FALSE-CLEAN, and on the command an author runs BEFORE shipping. + * Same direction and 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. + * + * ⚠️ #18779 corrected the SIZE that sentence used to be given, ⛔ not its + * direction. #18677 and #18778 both sized this blind spot by quoting + * `compile.ts`' claim that the survivors are "exactly the set the union could + * not see" — but the key was positional, so part of every survivor set was + * ECHO: findings the union run ALSO reported, which means `os validate` was + * reporting them all along through its own union run. On + * `examples/app-multi-package` the whole of it was — 1 survivor, 1 echo, 0 + * genuinely new — so `os validate`'s true blind spot on that fixture was ZERO + * findings, not one. ⛔ Neither card measured that; both quoted it. The + * asymmetry was real and worth closing on every door; its magnitude was + * inherited from a sentence nobody had read the definition behind. + * + * ## What the de-duplication key can and cannot promise + * + * `findingKey` above neutralises the top-level collection index (#18779), so + * the two views of one finding now produce one key and an echo is filtered. + * What reaches the lists below is therefore the set of per-package findings + * whose `rule`, `where`, `message` and NON-top-level position no union finding + * already carried. + * + * ⚠️ That is the whole claim, and it is deliberately narrower than "exactly the + * set the union could not see" — ⛔ do not restate it as that sentence. Two + * entries rendering the same `where` still collapse (see `findingKey`), and a + * rule that reports the same `rule`/`where`/`message` for genuinely different + * items distinguished ONLY by their top-level index would collapse with them. + * + * ⛔ And do not size that residue by quoting the pin next door — that move is + * exactly what this card exists to correct. `packages/lint/src/ + * data-model-rule-where-slot.test.ts` holds something NARROWER than "every + * rule names its entity in `where`": it runs the whole registry and fails any + * rule that puts a BARE CONFIG PATH in `where`. That forbids the one spelling + * which would make the collapse systematic; it does ⛔ not promise that two + * entries always render different `where` strings. So the residue is MEASURED + * instead — over every example stack in this repo that parses today + * (`app-multi-package`'s built artifact, `app-crm`, `app-showcase`, + * `app-todo`), 45 registry rules produced 103 findings and 103 distinct + * neutralised keys: ZERO groups held two different raw paths. ⛔ Re-measure + * rather than re-quote that number — a corpus reading is a count plus the tree + * it was taken against, and this one was taken on a43b9d0654. */ export function runPerPackageAuthoringRules(run: { /** Which door is asking — the same string its union run passed. */ diff --git a/packages/cli/test/build-text-face-advisory-count.test.ts b/packages/cli/test/build-text-face-advisory-count.test.ts index 04074622ff6..e6d1765c04b 100644 --- a/packages/cli/test/build-text-face-advisory-count.test.ts +++ b/packages/cli/test/build-text-face-advisory-count.test.ts @@ -44,6 +44,30 @@ * per-package prefix is not something the command emits unconditionally, and * the equality holds there on a shape that never had the defect. * + * ⚠️ [#18779] That control was itself being lit by an ECHO, and it went red + * when the echo stopped surviving. `CONFIG_MULTI` gave `bc_account.industry` no + * consumer anywhere, so the union run raised that finding too and the + * per-package "survivor" was the same finding re-reported at the package-local + * index. The de-duplication key used to compare the POSITIONAL `path`, which is + * why a duplicate reached the list at all; once the key stopped comparing the + * top-level collection index the survivor count went to 0 and + * `toBeGreaterThan(0)` failed — correctly. + * + * ⇒ `orders` now owns the VIEW that displays `bc_account.industry`, the same + * falsifier shape `lint-per-package-authoring-parity.test.ts` and + * `validate-per-package-authoring-parity.test.ts` carry. Folded into one union + * the field HAS a consumer and nothing is raised; judged per package, `core` + * declares a field nothing in `core` reads. So the control is now lit by a + * survivor the union genuinely could not see, ⛔ not by a duplicate of + * something it already printed. ⛔ Do not remove the view to "simplify" the + * fixture, and ⛔ do not relax `toBeGreaterThan(0)` — that assertion is what + * stops every equality below it from going vacuous. + * + * ⚠️ The `warnings: 4` in the measurement block above is a reading of the + * DEFECT on `examples/app-multi-package` at 17.4.0 and is kept as the record of + * it. That same fixture reports 3 since #18779 removed the echo; the equality + * these pins assert is unaffected, because it was never a number. + * * ## Tier * * SPAWNS the CLI ⇒ INTEGRATION tier by `packages/cli/vitest-tiers.ts`' @@ -161,13 +185,24 @@ const ordersObjects = [{ account: { name: 'account', type: 'lookup', label: 'Account', reference: 'bc_account' }, }, }]; +const ordersViews = [ + { + name: 'bc_account_list', label: 'Account List', object: 'bc_account', + list: { label: 'Account List', columns: ['name', 'industry'] }, + }, + { + name: 'bc_order_list', label: 'Order List', object: 'bc_order', + list: { label: 'Order List', columns: ['name', 'account'] }, + }, +]; export default { manifest: coreManifest, objects: [...ordersObjects, ...coreObjects], apps: [...coreApps], + views: [...ordersViews], packages: [ - { manifest: { ...ordersManifest, objects: ordersObjects } }, + { manifest: { ...ordersManifest, objects: ordersObjects, views: ordersViews } }, { manifest: { ...coreManifest, objects: coreObjects, apps: coreApps } }, ], }; @@ -235,8 +270,24 @@ describe("#18780 — `os build`'s text face prints every advisory its summary li expect(multiText.code, `${multiText.stdout}\n${multiText.stderr}`).toBe(0); expect(multiText.stdout).toContain('Running author-time rules per package (2)'); expect(multiJson.code, `${multiJson.stdout}\n${multiJson.stderr}`).toBe(0); - const perPackage = perPackageWarnings(payloadOf(multiJson, 'os build --json').warnings as unknown[]); + const warnings = payloadOf(multiJson, 'os build --json').warnings as unknown[]; + const perPackage = perPackageWarnings(warnings); expect(perPackage.length).toBeGreaterThan(0); + + // ⭐ [#18779] And the survivor's PEDIGREE, because the count alone is what + // this control had before: it was lit by an ECHO of a union finding, and a + // duplicate keeps the count above zero while proving nothing about the pass. + // `industry` must be named ONLY behind the per-package prefix — a union + // finding naming it too means the fixture has drifted back to the shape + // whose survivor the de-duplication now correctly removes. + const named = (w: unknown): string => + typeof (w as { where?: unknown })?.where === 'string' ? (w as { where: string }).where : ''; + const industry = warnings.filter((w) => /industry/.test(named(w))); + expect(industry.length, 'the fixture no longer raises the per-package finding').toBe(1); + expect( + PER_PACKAGE_WHERE.test(named(industry[0])), + 'the `industry` finding is being raised by the UNION run — this control would be lit by an echo', + ).toBe(true); }); it('the summary line counts exactly what the list above it renders', () => { diff --git a/packages/cli/test/lint-per-package-authoring-parity.test.ts b/packages/cli/test/lint-per-package-authoring-parity.test.ts index 40c9cfdb937..e874f594f69 100644 --- a/packages/cli/test/lint-per-package-authoring-parity.test.ts +++ b/packages/cli/test/lint-per-package-authoring-parity.test.ts @@ -7,9 +7,16 @@ * * `os build` has run the rule table a second time, once per * `artifactPackages(…)` entry, since #16611; `os validate` joined it in #18677. - * `os lint` ran the union fold and stopped, so the survivors of that pass — - * "exactly the set the union could not see", in `compile.ts`' own words — were - * findings `os build` reported and `os lint` structurally could not. + * `os lint` ran the union fold and stopped, so every survivor of that pass was + * a finding `os build` reported and `os lint` structurally could not. + * + * ⚠️ [#18779] This header used to size that set by quoting `compile.ts` — + * "exactly the set the union could not see" — and the sentence was false when + * it was quoted: the de-duplication key carried the POSITIONAL `path`, so + * echoes of union findings survived it and part of every survivor set was + * already being reported by this door's own union run. The key was corrected + * there. The PARITY this file pins is unaffected in either direction, because + * both doors always ran the one pass with the one key. * * ## Why this file exists next to the in-process seam pin * diff --git a/packages/cli/test/lint-per-package-authoring-seam.test.ts b/packages/cli/test/lint-per-package-authoring-seam.test.ts index 684b9ad1b3a..a22658631bd 100644 --- a/packages/cli/test/lint-per-package-authoring-seam.test.ts +++ b/packages/cli/test/lint-per-package-authoring-seam.test.ts @@ -6,11 +6,17 @@ * * `os build` has run the table a second time, once per `artifactPackages(…)` * entry with `packageBodyAsStack(…)` as resolution context, since #16611; - * `os validate` joined it in #18677. `os lint` did not. `compile.ts` step 3b-ii - * 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 lint` + * `os validate` joined it in #18677. `os lint` did not, so every survivor of + * that de-duplication was a finding `os build` reported and `os lint` * structurally could not — FALSE-CLEAN, on the fastest of the three doors. * + * ⚠️ [#18779] This header used to size that set by quoting `compile.ts` step + * 3b-ii — "exactly the set the union could not see" — and the sentence was + * false when it was quoted: the de-duplication key carried the POSITIONAL + * `path`, so echoes of union findings survived it. The key was corrected + * there. The fixture below is unaffected and is the reason why: it raises a + * finding the union genuinely cannot produce, ⛔ not an echo. + * * ## ⭐ Why symbol presence scored this door as covered * * `lint.ts` DOES import `artifactPackages` and `packageBodyAsStack`. Opening @@ -58,10 +64,16 @@ const PER_PACKAGE = /^package '[^']+' — /; * The `core` package owns `pp_account`; the `orders` package owns the view that * displays `pp_account.industry`. Judged as one flattened union the field has a * consumer and `field-no-consumers` says nothing; judged per package, `core` - * declares a field nothing IN CORE reads. That is not an echo of a union - * finding — it is a member of "the set the union could not see", which is the - * only shape that can falsify this card, and the reason this fixture is not the + * declares a field nothing IN CORE reads. That is ⛔ not an echo of a union + * finding — the union produced no such finding at all — which is the only + * shape that can falsify this card, and the reason this fixture is not the * sibling file's. + * + * [#18779] It is also the positive control for the corrected de-duplication + * key: a key that stopped discriminating would swallow this finding too, and + * this file goes red. The echo the corrected key DOES remove is pinned in + * `per-package-dedup-positional-echo.test.ts`, on a fixture built the other + * way round. */ function twoPackageArtifact(): Record { const core = defineStack({ diff --git a/packages/cli/test/per-package-dedup-positional-echo.test.ts b/packages/cli/test/per-package-dedup-positional-echo.test.ts new file mode 100644 index 00000000000..9451fdf4e1d --- /dev/null +++ b/packages/cli/test/per-package-dedup-positional-echo.test.ts @@ -0,0 +1,303 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #18779 — the per-package de-duplication key was POSITIONAL, so an ECHO of a + * union finding survived the filter that exists to remove it. + * + * `runPerPackageAuthoringRules` judges each `packages[]` entry as its own stack + * and drops anything the union run already reported. A package body re-bases + * every collection from 0, while the flattened union numbers that same entry + * wherever `authoringRuleUnionStack` put it — so one finding arrived under two + * paths, `findingKey` produced two keys, and the `Set` never matched them. + * + * ## What that cost, measured on `origin/main` 18cc3b1dfc + * + * On `examples/app-multi-package`, the repo's own two-package fixture: + * + * os build --json warnings 4 <- 3 union + 1 per-package survivor + * the survivor field-no-consumers on crm_account.industry + * union path objects[1].fields.industry + * survivor path objects[0].fields.industry + * => 1 of 1 an echo, 0 genuinely new + * + * So the pass's entire output on that fixture was a duplicate of a warning the + * same command had already printed, and `compile.ts`' claim that the survivors + * are "exactly the set the union could not see" was false in the one place + * anyone could check it. ⛔ Two cards (#18677, #18778) and two review seats + * quoted that sentence as authority; none opened `findingKey`. + * + * ## What this file pins + * + * The echo case ⇒ de-duplicated, and THREE controls that must still survive, + * because a key that stopped discriminating would satisfy the echo case + * trivially. The controls are what make a green here mean something: + * + * - a union twin differing only in its NESTED INDEX — the coordinate the + * rewrite deliberately leaves alone; + * - a union twin differing in `where` — a different entity; + * - a union twin differing in `rule`. + * + * ⚠️ The first control is built on `unique/unscoped-declared-index`, ⛔ not on + * the `field-no-consumers` finding the echo cases use, and the fixture carries + * a bare `unique: true` index for no other reason. Measured, ⛔ not reasoned: + * an ablation that widens the rewrite from the top-level index to EVERY index + * left all six cases green while that control was written against a + * `field-no-consumers` twin, because such a path ends in a field NAME and the + * twin therefore differed in a name rather than in a position. A control that + * cannot fail is decoration; re-run that ablation if this file is edited. + * + * `REAL_UNION` closes it end to end on a fixture shaped like the example: the + * union run really does raise the twin at a different index, so the case is + * not an artefact of hand-built `unionFindings`. + * + * ## Tier + * + * Spawns nothing and boots no kernel ⇒ UNIT tier by + * `packages/cli/vitest-tiers.ts`' predicate, asserted in BOTH directions by the + * last case rather than assumed from the filename. + */ + +import { describe, it, expect } from 'vitest'; +import { dirname, relative, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { normalizeStackInput, ObjectStackDefinitionSchema } from '@objectstack/spec'; +import { runAuthoringRules, type AuthoringFinding } from '@objectstack/lint'; +import { authoringRuleUnionStack } from '../src/utils/stack-collections.js'; +import { runPerPackageAuthoringRules } from '../src/utils/artifact-packages.js'; +import { firedSignals, integrationTestFiles, isIntegration, tierOfFile } from '../vitest-tiers.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const PKG = resolve(HERE, '..'); +const THIS_FILE = relative(PKG, fileURLToPath(import.meta.url)); + +const CORE_MANIFEST = { + id: 'com.example.echo.core', + name: 'echo core', + namespace: 'pp', + version: '1.0.0', + type: 'app', + engines: { protocol: '^17' }, +}; + +const CORE_OBJECTS = [ + { + 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 CORE_APPS = [ + { + 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 ORDERS_MANIFEST = { + id: 'com.example.echo.orders', + name: 'echo orders', + namespace: 'pp', + version: '1.0.0', + type: 'module', + engines: { protocol: '^17' }, + dependencies: { 'com.example.echo.core': '^1.0.0' }, +}; + +const ORDERS_OBJECTS = [ + { + 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' }, + }, + // ⛔ Not decoration. A bare `unique: true` trips + // `unique/unscoped-declared-index`, whose path carries a NESTED index + // (`objects[0].indexes[0]`) — and a finding with a nested index is the ONLY + // thing the NESTED control below can discriminate on. Every other rule this + // fixture raises produces `objects[N].fields.`, where the sole index + // is the top-level one, so a "nested position" control written against one + // of those cannot fail and is not a control. Measured: without this, an + // ablation that rewrites EVERY index instead of the top-level one keeps all + // six cases green. + indexes: [{ name: 'pp_order_name_uq', fields: ['name'], unique: true }], + }, +]; + +/** + * The example's shape: `orders` is listed FIRST, so `pp_account` lands at + * `objects[1]` in the flattened union and at `objects[0]` in `core`'s own body. + * That index disagreement IS the defect — a fixture with one package, or with + * the App package first, cannot exhibit it. + */ +function artifact(): Record { + return { + manifest: CORE_MANIFEST, + objects: [...ORDERS_OBJECTS, ...CORE_OBJECTS], + apps: [...CORE_APPS], + packages: [ + { manifest: { ...ORDERS_MANIFEST, objects: ORDERS_OBJECTS } }, + { manifest: { ...CORE_MANIFEST, objects: CORE_OBJECTS, apps: CORE_APPS } }, + ], + } as Record; +} + +function parsedArtifact(): Record { + const normalized = normalizeStackInput(artifact(), {}); + const result = ObjectStackDefinitionSchema.safeParse(normalized); + if (!result.success) { + throw new Error(`fixture is not schema-valid: ${JSON.stringify(result.error.issues.slice(0, 3))}`); + } + return result.data as unknown as Record; +} + +/** The pass's own output with NO union run to de-duplicate against. */ +function rawSurvivors(parsed: Record): AuthoringFinding[] { + return runPerPackageAuthoringRules({ command: 'build', parsed, unionFindings: [] }).findings; +} + +/** `where` as the de-duplication sees it — the pass prefixes it on the way out. */ +const unprefixed = (f: AuthoringFinding): AuthoringFinding => ({ + ...f, + where: f.where.replace(/^package '[^']*' — /, ''), +}); + +const survivorsAgainst = ( + parsed: Record, + unionFindings: readonly AuthoringFinding[], +): AuthoringFinding[] => runPerPackageAuthoringRules({ command: 'build', parsed, unionFindings }).findings; + +describe('#18779 — the per-package de-duplication key is position-insensitive', () => { + it('the fixture reaches the pass and raises the finding the rest of this file is about', () => { + // Asserted first: every case below is vacuous if the pass produces nothing, + // which is how this defect survived two cards' worth of parity pins. + const raw = rawSurvivors(parsedArtifact()); + expect(raw.length).toBeGreaterThan(0); + expect(raw.some((f) => f.rule === 'field-no-consumers' && /industry/.test(f.path))).toBe(true); + // The carrier the NESTED control needs — asserted here so its absence reads + // as "the fixture stopped raising it" rather than as a silently weakened + // control further down. + expect(raw.some((f) => /^objects\[\d+\]\.indexes\[\d+\]$/.test(f.path))).toBe(true); + expect(raw.every((f) => /^package '/.test(f.where))).toBe(true); + }); + + it('ECHO: a union twin at a different TOP-LEVEL index de-duplicates the survivor', () => { + const parsed = parsedArtifact(); + const raw = rawSurvivors(parsed); + const target = raw.find((f) => f.rule === 'field-no-consumers' && /industry/.test(f.path)); + expect(target, 'fixture no longer raises the finding this case is built on').toBeTruthy(); + + const local = unprefixed(target!); + // The SAME finding as the flattened union numbers it: `pp_account` sits at + // `objects[1]` there and at `objects[0]` in `core`'s body. Only that one + // coordinate differs — which is exactly what an echo is. + expect(local.path).toMatch(/^objects\[0\]\./); + const twin: AuthoringFinding = { ...local, path: local.path.replace(/^objects\[0\]/, 'objects[1]') }; + expect(twin.path).not.toBe(local.path); + + const survived = survivorsAgainst(parsed, [twin]); + expect( + survived.filter((f) => f.rule === target!.rule && f.where === target!.where), + 'an echo of a union finding must not survive the de-duplication that exists to remove it', + ).toEqual([]); + }); + + it('CONTROL: a union twin differing only in its NESTED index still lets the survivor through', () => { + // The rewrite is the top-level index ONLY. Nested positions address the + // author's own document and read the same in both views, so widening the + // rewrite to every index would swallow genuinely different findings — this + // case is what goes red if someone does. + // + // ⚠️ It has to be built on a finding whose path ACTUALLY carries a nested + // index. `unique/unscoped-declared-index` is that finding here + // (`objects[0].indexes[0]`); `field-no-consumers` is not — its path ends in + // a field NAME, so a twin built from it differs in a name rather than in a + // position and stays distinct under any index rewrite at all. Measured: the + // over-wide ablation keeps a `field-no-consumers` twin green and turns this + // one red, which is the whole difference between a control and a decoration. + const parsed = parsedArtifact(); + const target = rawSurvivors(parsed).find((f) => f.rule === 'unique/unscoped-declared-index'); + expect(target, 'fixture no longer raises a finding with a NESTED index in its path').toBeTruthy(); + const local = unprefixed(target!); + expect(local.path).toMatch(/^objects\[\d+\]\.indexes\[\d+\]$/); + + // Same top-level index, DIFFERENT nested one — the coordinate the rewrite + // must leave alone. + const twin: AuthoringFinding = { ...local, path: local.path.replace(/\.indexes\[0\]$/, '.indexes[1]') }; + expect(twin.path).not.toBe(local.path); + expect(twin.path.replace(/^objects\[\d+\]/, '')).not.toBe(local.path.replace(/^objects\[\d+\]/, '')); + + expect(survivorsAgainst(parsed, [twin]).map((f) => f.where)).toContain(target!.where); + }); + + it('CONTROL: a union twin differing in `where` or `rule` still lets the survivor through', () => { + const parsed = parsedArtifact(); + const target = rawSurvivors(parsed).find((f) => f.rule === 'field-no-consumers')!; + const local = unprefixed(target); + const otherWhere: AuthoringFinding = { + ...local, + where: 'object "pp_order" · field "industry"', + path: local.path.replace(/^objects\[0\]/, 'objects[1]'), + }; + const otherRule: AuthoringFinding = { + ...local, + rule: 'some-other-rule', + path: local.path.replace(/^objects\[0\]/, 'objects[1]'), + }; + expect(survivorsAgainst(parsed, [otherWhere]).map((f) => f.where)).toContain(target.where); + expect(survivorsAgainst(parsed, [otherRule]).map((f) => f.where)).toContain(target.where); + }); + + it('REAL_UNION: the union run really does raise the twin, at a different index, and it is filtered', () => { + // End to end, with no hand-built finding anywhere: the union run over the + // folded stack produces the twin itself. Before #18779 this fixture + // reported the same warning twice. + const parsed = parsedArtifact(); + const normalized = normalizeStackInput(artifact(), {}); + const unionFindings = runAuthoringRules('build', { + normalized: authoringRuleUnionStack(normalized as Record), + parsed: authoringRuleUnionStack(parsed), + }); + + const raw = rawSurvivors(parsed); + const target = unprefixed(raw.find((f) => f.rule === 'field-no-consumers' && /industry/.test(f.path))!); + const twin = unionFindings.find( + (u) => u.rule === target.rule && u.where === target.where && u.message === target.message, + ); + // Non-vacuity: the union MUST carry the twin, or "it was filtered" says + // nothing. And it must carry it at a DIFFERENT path, or the old key would + // have matched it too and this fixture would not exhibit the defect. + expect(twin, 'the union run no longer raises the twin — this fixture no longer exhibits the defect').toBeTruthy(); + expect(twin!.path).not.toBe(target.path); + + const survived = survivorsAgainst(parsed, unionFindings); + expect(survived.filter((f) => f.rule === target.rule && f.where.endsWith(target.where))).toEqual([]); + }); + + it('is a UNIT-tier file, asserted in BOTH directions from the predicate itself', () => { + // A pin in the wrong tier runs where the merge queue does not look, and a + // pin that MOVES its file between tiers takes its neighbours with it. Both + // directions, read from the predicate rather than from the filename: no + // integration signal fires, and the derived integration population does + // not contain this file. + const signals = tierOfFile(PKG, THIS_FILE); + expect(isIntegration(signals), `fired: ${firedSignals(signals)}`).toBe(false); + expect(integrationTestFiles(PKG)).not.toContain(THIS_FILE); + }); +}); diff --git a/packages/cli/test/validate-per-package-authoring-parity.test.ts b/packages/cli/test/validate-per-package-authoring-parity.test.ts index e4a24cdf2c2..893dc2856e6 100644 --- a/packages/cli/test/validate-per-package-authoring-parity.test.ts +++ b/packages/cli/test/validate-per-package-authoring-parity.test.ts @@ -5,11 +5,17 @@ * 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. + * entry; `os validate` ran the union fold and stopped, so every survivor of + * that pass was a finding `os build` reported and `os validate` structurally + * could not. FALSE-CLEAN, on the fast pre-flight an author runs before + * shipping. + * + * ⚠️ [#18779] This header used to size that set by quoting `compile.ts` — + * "exactly the set the union could not see" — and the sentence was false when + * it was quoted: the de-duplication key carried the POSITIONAL `path`, so + * echoes survived it. The key was corrected there; the PARITY pinned here is + * unaffected in either direction, because both doors run the one pass with the + * one key. * * ## Why a NEW fixture and not an assertion on the existing parity file * @@ -27,19 +33,31 @@ * 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 + * ## This file's fixture used to be carried by an ECHO — [#18779] it is not now + * + * ⚠️ The version of `CONFIG_MULTI` this file shipped with had NO consumer for + * `pp_account.industry` anywhere, so the union run raised that finding too and + * the per-package survivor was the same finding re-reported at the + * package-local index. Once the de-duplication key stopped comparing the + * top-level index (#18779) that survivor was correctly filtered, the fixture's + * per-package set went EMPTY, and the non-vacuity case below went red — which + * is how the echo was discovered to be the only thing holding this pin up. * - * ⚠️ 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. + * ⇒ the fixture now carries the FALSIFIER shape instead, the same one + * `lint-per-package-authoring-parity.test.ts` uses: `core` owns `pp_account` + * and `orders` owns the view that displays `pp_account.industry`. Folded into + * one union the field HAS a consumer and nothing is raised; judged per package, + * `core` declares a field nothing in `core` reads. So the union run is clean + * and the per-package run is not — a survivor the union genuinely could not + * see, ⛔ not a duplicate of something it already reported. Measured on the new + * fixture: `os build --json` warnings 2, of which exactly 1 carries the + * per-package `where` prefix and no union finding names `industry` at all. + * + * ⭐ The pins themselves are unchanged and still measure PARITY rather than the + * survivor's pedigree — but a parity pin whose per-package set is empty + * measures nothing, so the pedigree is what decides whether the pin is alive. + * 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 * @@ -102,6 +120,14 @@ const perPackageWarnings = (warnings: unknown[]): string[] => * 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. + * + * ⚠️ [#18779] `orders` also owns the VIEW that displays `pp_account.industry`, + * and that is load-bearing rather than decoration: it is what makes the union + * run clean while the per-package run is not, so the survivor these pins + * compare is one the union could not see. Without it the only survivor was an + * echo of a union finding, and it stopped surviving the moment the + * de-duplication key was corrected — leaving the pins below comparing two empty + * sets. ⛔ Do not remove the view to "simplify" the fixture. */ const CONFIG_MULTI = ` const coreManifest = { @@ -135,6 +161,16 @@ const ordersObjects = [{ account: { name: 'account', type: 'lookup', label: 'Account', reference: 'pp_account' }, }, }]; +const ordersViews = [ + { + name: 'pp_account_list', label: 'Account List', object: 'pp_account', + list: { label: 'Account List', columns: ['name', 'industry'] }, + }, + { + name: 'pp_order_list', label: 'Order List', object: 'pp_order', + list: { label: 'Order List', columns: ['name', 'account'] }, + }, +]; export default { // The ARTIFACT's own identity: \`preserve\` is additive, so the singular @@ -144,12 +180,13 @@ export default { manifest: coreManifest, objects: [...ordersObjects, ...coreObjects], apps: [...coreApps], + views: [...ordersViews], // …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: { ...ordersManifest, objects: ordersObjects, views: ordersViews } }, { manifest: { ...coreManifest, objects: coreObjects, apps: coreApps } }, ], }; @@ -206,7 +243,7 @@ describe('#18677 — `os validate` and `os build` report the same per-package ad 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 () => { + it('the multi-package fixture reaches the pass at all — and its survivor is NOT an echo', 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. @@ -214,6 +251,21 @@ describe('#18677 — `os validate` and `os build` report the same per-package ad 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); + + // ⭐ [#18779] And the second half, which is what this file was missing: a + // per-package survivor that merely ECHOES a union finding keeps the count + // above zero while measuring nothing. `industry` must be raised ONLY behind + // the per-package prefix — if a union finding names it too, the fixture has + // drifted back to the shape whose survivor the de-duplication now (rightly) + // removes, and every parity case below is comparing two empty sets. + const named = (w: unknown): string => + typeof (w as { where?: unknown })?.where === 'string' ? (w as { where: string }).where : ''; + const industryWarnings = warnings.filter((w) => /industry/.test(named(w))); + expect(industryWarnings.length, 'the fixture no longer raises the per-package finding').toBe(1); + expect( + PER_PACKAGE_WHERE.test(named(industryWarnings[0])), + 'the `industry` finding is being raised by the UNION run — the survivor would be an echo', + ).toBe(true); }, 180_000); it('`os validate` reports every per-package advisory `os build` does', async () => { diff --git a/packages/cli/test/validate-per-package-authoring-seam.test.ts b/packages/cli/test/validate-per-package-authoring-seam.test.ts index cb45450cf93..d2df9753eff 100644 --- a/packages/cli/test/validate-per-package-authoring-seam.test.ts +++ b/packages/cli/test/validate-per-package-authoring-seam.test.ts @@ -3,12 +3,16 @@ /** * #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. + * context; `os validate` ran the union fold and stopped, so every survivor of + * that de-duplication was a finding `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. + * + * ⚠️ [#18779] This header used to size that set by quoting `compile.ts` — + * "exactly the set the union could not see" — and the sentence was false when + * it was quoted: the key carried the POSITIONAL `path`, so echoes survived it. + * The key was corrected there; the seam this file pins is unaffected. * * ## What this file pins, and what its sibling pins * @@ -186,8 +190,13 @@ describe('#18677 — the per-package author-time pass is ONE seam both doors rea 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. + // OWN output as the union run must empty it — the property that makes the + // survivor list mean anything at all. ⚠️ This case is NOT sensitive to + // #18779's defect and never was: both runs judge the same stack, so the + // two paths are identical and the positional key matched them anyway. The + // echo it missed needs the two views to DISAGREE about the index, which + // only a real package-body-vs-union comparison produces — pinned in + // `per-package-dedup-positional-echo.test.ts`. const parsed = twoPackageArtifact(); const first = runPerPackageAuthoringRules({ command: 'validate', parsed, unionFindings: [] }); const raw = [...first.errors, ...first.advisories];