From 60ce42b73631e03a474aad4663c2415ac9e23ac8 Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 5 Aug 2026 09:37:26 -0400 Subject: [PATCH] fix(catalog): settle the two guard collisions before Phase E inherits them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were found by implementation sessions, both were worked around rather than resolved, and both would have recurred in Phase G. Maintainer decisions 2026-08-05. D9 - reading an installed dependency's version. FR-029/T063 require the picomatch version be read at runtime and never transcribed; ADR-0013/FR-002 forbid a dynamic loader and the guard bans import.meta.resolve anywhere in the adapter. Read naively those collide. They do not: the distinction the guard protects is dynamic MODULE LOADING, not filesystem access. Walking up to node_modules/picomatch/package.json and reading its version field invokes no resolver, imports no module, and cannot load code, while still taking the value from the installed artifact rather than a literal in our source. Recorded as package-boundary.md §6 with the distinction tabulated, because to the session that implemented it this looked like a route around the guard rather than a path through it, and the next reader deserves the reasoning rather than an inference from what happened to be committed. The ADR-0016 obligation stays attached: a read that silently returns undefined and one that returns 4.0.5 are indistinguishable from a green suite alone. D10 - source-scan false positives. The scans match `from '...'` and `import '...'` textually rather than resolving them, so a file that STATES a rule is indistinguishable from one that BREAKS it. This fired on the string 'bulk-import' - ADR-0015's own plugin name - and on prose ending in "from " before a quote. Two sessions renamed around it, which leaves the trap armed for the next writer. EXCLUDED_FROM_SCAN now carries the consumer's two boundary guards, each of which must name the very thing it forbids: the schema file it pins by hash, and the adapter package it proves is never imported. The patterns are deliberately NOT loosened - a scan that misses a real edge is worse than one that occasionally over-matches, and the exclusion list is the intended relief valve. The pinned-set assertion caught this change and failed until updated deliberately, which is the guard working exactly as designed; it now reads five files and records why the two were added. Gates: bun test 1504 pass / 0 fail, typecheck clean, check:deps ok, check:freeze-hashes ok over both frozen trees, adr lint 20 records 0/0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../test/no-dynamic-loader.test.ts | 13 ++++++- .../catalog-backstage/test/source-scan.ts | 24 ++++++++++++ .../contracts/package-boundary.md | 39 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/packages/adapters/catalog-backstage/test/no-dynamic-loader.test.ts b/packages/adapters/catalog-backstage/test/no-dynamic-loader.test.ts index bba23a05..1a478997 100644 --- a/packages/adapters/catalog-backstage/test/no-dynamic-loader.test.ts +++ b/packages/adapters/catalog-backstage/test/no-dynamic-loader.test.ts @@ -107,13 +107,22 @@ describe('FR-002 — no dynamic loader anywhere in the adapter source', () => { expect(files.length).toBeGreaterThan(0); }); - test('the excluded-from-scan set is exactly the three self-referential guard files', () => { + test('the excluded-from-scan set is exactly the five self-referential guard files', () => { // These files contain the rule literals themselves. The exclusion is pinned - // so it cannot grow into a way of hiding a violation. + // so it cannot grow into a way of hiding a violation: adding an entry fails + // this test until someone updates it deliberately, which is the point. + // + // The two consumer entries were added on 2026-08-05. Each must name the very + // thing it forbids — the schema file it pins by hash, and the adapter package + // it proves is never imported — so both were unscannable without an entry + // here. The alternative two sessions reached for first was renaming around + // the scanner, which leaves the trap armed for the next writer. expect([...EXCLUDED_FROM_SCAN]).toEqual([ 'packages/adapters/catalog-backstage/test/envelope-shape-locality.test.ts', 'packages/adapters/catalog-backstage/test/no-dynamic-loader.test.ts', 'packages/adapters/catalog-backstage/test/source-scan.ts', + 'packages/catalog-envelope/test/no-core-schema-change.test.ts', + 'packages/catalog-envelope/test/no-adapter-import.test.ts', ]); }); diff --git a/packages/adapters/catalog-backstage/test/source-scan.ts b/packages/adapters/catalog-backstage/test/source-scan.ts index 9e242c70..42277443 100644 --- a/packages/adapters/catalog-backstage/test/source-scan.ts +++ b/packages/adapters/catalog-backstage/test/source-scan.ts @@ -37,11 +37,35 @@ export const CONSUMER_PACKAGE_NAME = '@adrkit/catalog-envelope'; * * This is asserted to be exactly this set. An exclusion list that can grow * without anyone noticing is the same defect the scans are guarding against. + * + * **A guard that must name what it forbids belongs here.** The scans are + * deliberately literal — `importSpecifiers` matches `from '…'` and + * `import '…'` textually rather than resolving them — so a file that *states* + * a rule is indistinguishable to it from a file that *breaks* one. Feature 010 + * hit this three times: the consumer's two boundary guards (which must name + * `schema/adr.schema.json` to hash it, and `@adrkit/catalog-backstage` to + * forbid importing it), and, more surprisingly, ordinary prose and string data + * — the literal `'bulk-import'`, which is ADR-0015's own plugin name, scanned + * as a side-effecting `import '…'`. + * + * Adding an entry is the correct fix and is preferred over renaming around + * the scanner, which is what two sessions did before this list was extended. + * Renaming leaves the trap armed for the next writer; listing the file is + * visible, reviewable, and asserted. Do **not** loosen the patterns to make a + * false positive go away — a scan that misses a real edge is worse than one + * that occasionally over-matches, and this list is the intended relief valve. + * + * @see specs/010-catalog-backstage/contracts/package-boundary.md §4 */ export const EXCLUDED_FROM_SCAN: readonly string[] = [ 'packages/adapters/catalog-backstage/test/envelope-shape-locality.test.ts', 'packages/adapters/catalog-backstage/test/no-dynamic-loader.test.ts', 'packages/adapters/catalog-backstage/test/source-scan.ts', + // Consumer-side boundary guards. Each must name the very thing it forbids: + // the schema file it pins by hash, and the adapter package it proves is + // never imported. See package-boundary.md §4. + 'packages/catalog-envelope/test/no-core-schema-change.test.ts', + 'packages/catalog-envelope/test/no-adapter-import.test.ts', ]; export interface ScannedFile { diff --git a/specs/010-catalog-backstage/contracts/package-boundary.md b/specs/010-catalog-backstage/contracts/package-boundary.md index 365ef767..1c2bd56a 100644 --- a/specs/010-catalog-backstage/contracts/package-boundary.md +++ b/specs/010-catalog-backstage/contracts/package-boundary.md @@ -231,3 +231,42 @@ verbatim by `scripts/check-deps.test.ts`. A line number is a reference that nothing checks. A reason string is a reference the test suite checks on every run, so a citation that goes stale fails the build rather than quietly misleading a reader. + +--- + +## §6. Reading an installed dependency's version — permitted, and why it is not loader behaviour + +**Decided by the maintainer, 2026-08-05.** + +FR-029 / T063 require the `picomatch` version be **read at runtime from the resolved +dependency, never transcribed**, so that the frozen glob engine recorded in a snapshot is +the engine that actually ran. ADR-0013 and FR-002 separately forbid any dynamic runtime +adapter/plugin loader, and the guard enforcing that +(`packages/adapters/catalog-backstage/test/no-dynamic-loader.test.ts`) bans +`import.meta.resolve` anywhere in the adapter. + +Read naively, those two requirements collide. + +**They do not.** The permitted implementation is a **filesystem read of the installed +dependency's manifest** — walking up from the module to `node_modules/picomatch/package.json` +and reading its `version` field. This satisfies FR-029 because the value comes from the +installed artifact rather than from a literal in our source, and it does not engage the +loader-guard's concern because it invokes no resolver, imports no module, and cannot load +code. + +The distinction the guard protects is **dynamic module loading**, not **filesystem access**: + +| | Loads code | Resolver invoked | Permitted | +|---|---|---|---| +| `import.meta.resolve(...)`, dynamic `import()`, `require.resolve` | yes | yes | **no** — ADR-0013, FR-002 | +| Reading `node_modules//package.json` as a file | no | no | **yes** — this section | + +Recorded here rather than left in a source comment because it looked, to the session that +implemented it, like a route *around* the guard rather than a path *through* it — and a +future reader deciding the same question deserves the reasoning, not an inference from +what happened to be committed. + +**The observation still applies.** A test asserting the read value matches the lockfile is +required and must be observed failing (ADR-0016), because a read that silently returns +`undefined` and a read that returns the right version are indistinguishable from a green +suite alone.