From 3ac25d55e548a8e11dcd79773373a841c35b55b4 Mon Sep 17 00:00:00 2001 From: Cody Swann Date: Fri, 21 Aug 2026 11:11:33 -0400 Subject: [PATCH 1/2] fix(templates): fail when a stack is handed a tool it is not given MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A key added to a parent stack's `package.lisa.json` force section is written verbatim into every child stack's package.json. Inheritance is keyed on the type hierarchy and never on the receiving stack's toolchain, so a Jest stack and a vitest stack inherit identically — and an unreferenced script costs nothing and proves nothing until something invokes it, which can be weeks later, in a consumer, from a pre-push hook. The invisibility is the defect; the one bad pin is only its first symptom. The check resolves each stack the way `apply` does — through `PackageLisaStrategy.planPackageJson`, not a second copy of the merge rules — and asks whether every tool a resolved script names comes from a package that stack actually receives. Two properties keep it from becoming a roster: - the stacks come from `PROJECT_TYPE_HIERARCHY`, so adding one is covered with no edit here; - which tools are *governed* is derived from the templates too. A tool is governed when some stack pins a package providing it. Tools no stack pins (`node`, `tsc` before this commit, `bash`, `docker`) are the host's to supply and are ignored — but the moment one stack pins a tool, every stack whose scripts name it must pin it too. That asymmetry is exactly what makes an inherited pin dangerous, so the exemption can never hide the hazard. Binary names come from each installed package's own `bin` field, so `tsc` maps to typescript and `ast-grep` to @ast-grep/cli with no alias table, and `@types/node` — which declares no binary — is not mistaken for the provider of `node`. That reading is required rather than preferred: with no installed packages to read, the map would have to guess from names, which cannot see that `@playwright/test` provides `playwright`. The check refuses instead of degrading, because an empty result from a check that could not do its job is the failure mode this whole class is made of. Restoring the original defect reproduces it by name: with expo's own `test:cov:unit` pin deleted, the check reports expo: script "test:cov:unit" runs `vitest` (inherited from typescript) but expo is given none of: vitest The check found six live instances of the same class and they are fixed here. The typescript base forces lint, format, typecheck, prepare and six vitest scripts while pinning none of the tools they name, so a project detected as plain `typescript` received commands it could not run; two stacks pinned the same toolchain for themselves, which is what made those tools governed while five stacks went without. The base now supplies them as `defaults`, where a host's own pin wins, per the force/defaults/merge semantics. vitest is pinned there too and removed on the Jest stack, following the entry that stack already carries for the vitest mutation runner — a Jest project must not be handed a runner it cannot use. nestjs pinned tsx for its migration scripts nowhere; it does now. The pair-specific coverage-unit runner guard still stands: it asserts something stronger about one pair of keys that no dependency-derived check can see. Work-Item: CodySwannGT/lisa#2848 Co-Authored-By: Claude --- expo/package-lisa/package.lisa.json | 4 +- nestjs/package-lisa/package.lisa.json | 3 +- src/core/upstream-evidence-manifest.ts | 8 +- tests/helpers/template-toolchain.ts | 400 ++++++++++++++++++ .../config/template-script-toolchain.test.ts | 212 ++++++++++ typescript/package-lisa/package.lisa.json | 9 +- 6 files changed, 630 insertions(+), 6 deletions(-) create mode 100644 tests/helpers/template-toolchain.ts create mode 100644 tests/unit/config/template-script-toolchain.test.ts diff --git a/expo/package-lisa/package.lisa.json b/expo/package-lisa/package.lisa.json index 45c00128b0..22b2a0ed64 100644 --- a/expo/package-lisa/package.lisa.json +++ b/expo/package-lisa/package.lisa.json @@ -191,7 +191,9 @@ "brace-expansion" ], "devDependencies": [ - "@stryker-mutator/vitest-runner" + "@stryker-mutator/vitest-runner", + "@vitest/coverage-v8", + "vitest" ] } } diff --git a/nestjs/package-lisa/package.lisa.json b/nestjs/package-lisa/package.lisa.json index 8534c917a0..d5902c9626 100644 --- a/nestjs/package-lisa/package.lisa.json +++ b/nestjs/package-lisa/package.lisa.json @@ -4,7 +4,8 @@ "@codyswann/lisa": "^2.106.0", "@graphql-codegen/cli": "^6.1.0", "@graphql-codegen/typescript": "^4.1.6", - "@graphql-codegen/typescript-operations": "^4.4.2" + "@graphql-codegen/typescript-operations": "^4.4.2", + "tsx": "^4.20.6" }, "scripts": { "migration:generate": "tsx ./node_modules/typeorm/cli.js migration:generate -d typeorm.config.ts src/database/migrations/$npm_config_name", diff --git a/src/core/upstream-evidence-manifest.ts b/src/core/upstream-evidence-manifest.ts index bb4237f62d..92227bc4d5 100644 --- a/src/core/upstream-evidence-manifest.ts +++ b/src/core/upstream-evidence-manifest.ts @@ -289,7 +289,7 @@ export const UPSTREAM_EVIDENCE_MANIFEST: Readonly> = "expo/merge/.oxlintrc.json": "95b3069256c0040be0ef1a5adae46d14687ad56fb18f473a653ba2de45d106bb", "expo/package-lisa/package.lisa.json": - "4b299aec0e1c3bdb0ea5b85f1ccc415747ee3a168ed962aea654634ad5efeb19", + "fe9b22bc59d9e95680f37f102dc353c4badb8266c31a765deab84e9b48ec437d", "harper-fabric/copy-contents/.prettierignore": "478c782f4c5611187e21584dfd5522e37fc636c5eb03394fea3db45321c6712c", "harper-fabric/copy-contents/gitignore": @@ -417,7 +417,7 @@ export const UPSTREAM_EVIDENCE_MANIFEST: Readonly> = "nestjs/merge/.oxlintrc.json": "1de29d135744df0258e8659ee0b684acf84e687bbefade51db0576813e6ff097", "nestjs/package-lisa/package.lisa.json": - "2649752534751c072ccae09000d36c575d713aeb41af6bc57626ab906045c89a", + "0988afbca21b2bbdfa2bd5ef9623e6b560cfdf0a5c18bf75385d4d21d7725441", "npm-package/create-only/.github/workflows/publish-to-npm.yml": "1d051007a328ba4f6c67a5e3123593921b823e0866c05343c4588a6156e9f593", "npm-package/package-lisa/package.lisa.json": @@ -2405,7 +2405,7 @@ export const UPSTREAM_EVIDENCE_MANIFEST: Readonly> = "typescript/merge/.oxlintrc.json": "9504c20db80470c242c4ffe8cccad6951ed8141dfb5bf6503053e0b2712ab276", "typescript/package-lisa/package.lisa.json": - "0da7069e5b7eb4e16e67201d047be1dd85b82c0e2bcf7cf0fa0ab0caff8a8b1a", + "705debefdf6f88877e796436619509104c1368e376852ddb1eb451b0e63e5248", "ui/README.md": "deeb35e767ea5dd2883268835ea3ad21cbad9fa63ec8d8ff5e200f0e2a7d2751", "ui/index.html": @@ -8756,6 +8756,7 @@ export const UPSTREAM_SURFACE_MANIFEST: Readonly> = "tests/helpers/safety-net-guard-fixtures.ts": true, "tests/helpers/safety-net-guard-harness.ts": true, "tests/helpers/safety-net-subst-fixtures.ts": true, + "tests/helpers/template-toolchain.ts": true, "tests/helpers/test-utils.ts": true, "tests/helpers/verification-gate-fixtures.ts": true, "tests/helpers/verification-gate-harness.ts": true, @@ -9014,6 +9015,7 @@ export const UPSTREAM_SURFACE_MANIFEST: Readonly> = "tests/unit/config/rails-template.test.ts": true, "tests/unit/config/release-push-retry.test.ts": true, "tests/unit/config/security-pin-floors.test.ts": true, + "tests/unit/config/template-script-toolchain.test.ts": true, "tests/unit/config/tsconfig-no-unused-flags.test.ts": true, "tests/unit/config/tsconfig-template-references.test.ts": true, "tests/unit/config/vitest-base.test.ts": true, diff --git a/tests/helpers/template-toolchain.ts b/tests/helpers/template-toolchain.ts new file mode 100644 index 0000000000..bed9dac030 --- /dev/null +++ b/tests/helpers/template-toolchain.ts @@ -0,0 +1,400 @@ +/** + * Answers one question of Lisa's shipped `package.lisa.json` templates: does + * every tool a script names come from a package the receiving stack is + * actually given? + * + * The templates deep-merge parent into child, so a key added to + * `typescript/package-lisa/package.lisa.json` is written verbatim into the + * `package.json` of every stack that names `typescript` as its parent — + * including stacks whose toolchain cannot run it. Nothing in the edit, the + * diff, or the apply summary says which stacks received it, and an unreferenced + * script costs nothing until something invokes it, which can be months later + * and in a consumer rather than here. That is how a Jest stack came to carry a + * vitest coverage command (#2848) and a vitest mutation runner (#1413). + * + * Two properties keep this from being a roster that has to be edited: + * + * - The stacks come from {@link PROJECT_TYPE_HIERARCHY} and the resolution + * comes from {@link PackageLisaStrategy.planPackageJson} — the same code + * `apply` runs — so adding a stack or a template layer is covered with no + * edit here. + * - Which tools are *governed* is derived from the templates too: a tool is + * governed when some stack pins a package that provides it. Tools no stack + * pins (`node`, `tsc`, `bash`, `docker`) are the host's to supply and are + * ignored. The moment one stack pins a tool, every stack whose scripts name + * it must pin it as well — which is precisely the asymmetry that makes an + * inherited pin dangerous. + * + * @module tests/helpers/template-toolchain + */ + +import * as fs from "node:fs"; +import * as path from "node:path"; + +import type { ProjectType } from "../../src/core/config.js"; +import { PROJECT_TYPE_HIERARCHY } from "../../src/core/config.js"; +import { PackageLisaStrategy } from "../../src/strategies/package-lisa.js"; + +/** A stack's shipped scripts and the packages it receives. */ +interface ResolvedStack { + /** Scripts the stack's `package.json` ends up with. */ + readonly scripts: Readonly>; + /** Every dependency and devDependency the stack ends up with. */ + readonly packages: ReadonlySet; +} + +/** One script that names a tool its stack is not given. */ +export interface ToolchainViolation { + /** Project type whose resolved `package.json` carries the script. */ + readonly stack: string; + /** The `scripts` key. */ + readonly scriptKey: string; + /** The script body, verbatim. */ + readonly command: string; + /** The executable the command invokes. */ + readonly tool: string; + /** Packages that provide {@link tool}, none of which the stack receives. */ + readonly providers: readonly string[]; + /** Layer the script's final value came from, or the stack itself. */ + readonly origin: string; +} + +/** + * Wrappers that fetch their argument on demand, so the tool they name needs no + * pin. `npx vitest` is a download, not a dependency. + */ +const ON_DEMAND_RUNNERS: ReadonlySet = new Set([ + "npx", + "bunx", + "pnpx", + "dlx", +]); + +/** + * Package managers whose next word is a sibling script, not a tool. That + * sibling is resolved and checked in its own right. + */ +const SCRIPT_DELEGATES: ReadonlySet = new Set([ + "bun", + "npm", + "pnpm", + "yarn", +]); + +/** Shell metacharacters that end one command and begin another. */ +const SEGMENT_BREAKS: ReadonlySet = new Set(["&", "|", ";"]); + +/** Placeholder host manifest — never Lisa's own, which resolves differently. */ +const PROBE_MANIFEST: Readonly> = { + name: "lisa-template-toolchain-probe", + version: "0.0.0", +}; + +/** Running state of the quote-aware segment splitter. */ +interface SplitState { + readonly quote: string | null; + readonly current: string; + readonly segments: readonly string[]; +} + +/** + * Split a script body into the individual commands a shell would run. + * @remarks + * Quote-aware, because `node -e "a; b"` is one command and not two. Any run of + * `&`, `|` or `;` outside quotes separates; empty pieces are dropped, so `&&` + * and `||` need no special case. + * @param command - A `scripts` value + * @returns The commands, in order + */ +function splitSegments(command: string): readonly string[] { + const final = [...command].reduce( + (state, char) => { + if (state.quote !== null) { + return { + ...state, + quote: char === state.quote ? null : state.quote, + current: state.current + char, + }; + } + if (char === '"' || char === "'") { + return { ...state, quote: char, current: state.current + char }; + } + if (SEGMENT_BREAKS.has(char)) { + return { + quote: null, + current: "", + segments: [...state.segments, state.current], + }; + } + return { ...state, current: state.current + char }; + }, + { quote: null, current: "", segments: [] } + ); + + return [...final.segments, final.current] + .map(segment => segment.trim()) + .filter(segment => segment.length > 0); +} + +/** + * The executable one command invokes, if it invokes a nameable one. + * @remarks + * Leading `VAR=value` assignments are stripped. On-demand runners and + * package-manager script delegation yield nothing: neither names a tool that + * has to be pinned. + * @param segment - A single command + * @returns The executable name, or null + */ +function executableOf(segment: string): string | null { + const words = segment.split(/\s+/u).filter(word => word.length > 0); + const head = words.find(word => !/^[A-Za-z_]\w*=/u.test(word)); + if (head === undefined) return null; + if (ON_DEMAND_RUNNERS.has(head) || SCRIPT_DELEGATES.has(head)) return null; + return head; +} + +/** + * Every executable a script body invokes. + * @param command - A `scripts` value + * @returns Executable names, deduplicated, in first-seen order + */ +export function commandTools(command: string): readonly string[] { + const named = splitSegments(command) + .map(executableOf) + .filter((tool): tool is string => tool !== null); + return [...new Set(named)]; +} + +/** + * The binaries an installed package declares. + * @remarks + * Read from the package's own `bin` field where the package is installed, so + * `tsc` maps to `typescript` and `ast-grep` to `@ast-grep/cli` without a + * hand-written alias table — and so `@types/node`, which declares no binary, + * is not mistaken for the provider of `node`. Where the package is not + * installed the name is the only evidence available, and `@scope/name` + * conventionally provides `name`. + * @param pkg - Package name + * @param nodeModulesDir - Directory installed packages live in + * @returns Binary names the package provides + */ +function binariesOf(pkg: string, nodeModulesDir: string): readonly string[] { + // Unconditional, and not merely a fallback: a type package ships no binary, + // and letting `@types/node` name itself the provider of `node` would turn + // every `node scripts/…` script in the tree into a violation the moment a + // lockfile stopped hoisting it. + if (pkg.startsWith("@types/")) return []; + const manifestPath = path.join(nodeModulesDir, pkg, "package.json"); + if (fs.existsSync(manifestPath)) { + const manifest = JSON.parse(fs.readFileSync(manifestPath, "utf8")) as { + readonly bin?: string | Record; + }; + if (manifest.bin === undefined) return []; + return typeof manifest.bin === "string" ? [pkg] : Object.keys(manifest.bin); + } + const tail = pkg.startsWith("@") ? (pkg.split("/")[1] ?? pkg) : pkg; + return [...new Set([pkg, tail])]; +} + +/** + * Index every governed binary to the packages that provide it. + * @param packages - Every package any stack receives + * @param nodeModulesDir - Directory installed packages live in + * @returns Binary name to providing packages + */ +function indexBinaries( + packages: ReadonlySet, + nodeModulesDir: string +): ReadonlyMap { + const pairs = [...packages].flatMap(pkg => + binariesOf(pkg, nodeModulesDir).map(bin => ({ bin, pkg })) + ); + return new Map( + [...new Set(pairs.map(pair => pair.bin))].map(bin => [ + bin, + pairs.filter(pair => pair.bin === bin).map(pair => pair.pkg), + ]) + ); +} + +/** + * Resolve one stack's `package.json` exactly as `apply` would for a greenfield + * project of that type. + * @param lisaDir - Directory the template layers live in + * @param type - Project type + * @returns Its scripts and packages + */ +async function resolveStack( + lisaDir: string, + type: string +): Promise { + const planned = await new PackageLisaStrategy().planPackageJson( + { ...PROBE_MANIFEST }, + [type as ProjectType], + lisaDir + ); + const section = (name: string): Record => + (planned[name] ?? {}) as Record; + return { + scripts: section("scripts"), + packages: new Set([ + ...Object.keys(section("dependencies")), + ...Object.keys(section("devDependencies")), + ]), + }; +} + +/** + * A stack's ancestors, nearest parent first. + * @param type - Project type + * @param hierarchy - Parent map + * @returns Ancestor types + */ +function ancestorsOf( + type: string, + hierarchy: Readonly> +): readonly string[] { + const parent = hierarchy[type]; + return parent === undefined + ? [] + : [parent, ...ancestorsOf(parent, hierarchy)]; +} + +/** + * Which layer a stack's script value came from. + * @param scriptKey - The `scripts` key + * @param command - Its resolved value + * @param stack - The receiving stack + * @param ancestors - The stack's ancestors, nearest first + * @param resolved - Every stack's resolution + * @returns The nearest ancestor carrying the identical value, else the stack + */ +function originOf( + scriptKey: string, + command: string, + stack: string, + ancestors: readonly string[], + resolved: ReadonlyMap +): string { + const inherited = ancestors.find( + ancestor => resolved.get(ancestor)?.scripts[scriptKey] === command + ); + return inherited ?? stack; +} + +/** Optional inputs, all defaulted from `lisaDir`. */ +export interface ToolchainCheckOptions { + /** Project types to resolve; defaults to every type in the hierarchy. */ + readonly types?: readonly string[]; + /** + * Where installed packages live; defaults to `/node_modules`, which + * must exist. Supply a path explicitly only for a fixture tree, whose + * synthetic packages are named after the binaries they provide. + */ + readonly nodeModulesDir?: string; +} + +/** + * Every shipped script that names a governed tool its stack is not given. + * @param lisaDir - Directory the template layers live in + * @param options - Overrides, for exercising the check against fixtures + * @returns Violations, stack by stack + */ +export async function findToolchainViolations( + lisaDir: string, + options: ToolchainCheckOptions = {} +): Promise { + const types = options.types ?? Object.keys(PROJECT_TYPE_HIERARCHY); + const nodeModulesDir = + options.nodeModulesDir ?? path.join(lisaDir, "node_modules"); + if (options.nodeModulesDir === undefined && !fs.existsSync(nodeModulesDir)) { + // Refuse rather than degrade. Without installed packages the binary map + // falls back to guessing from names, which cannot see that + // `@playwright/test` provides `playwright` — so a stack that is fine reads + // as a violation, and a stack that is not can read as fine. An empty + // result from a check that could not do its job is the failure mode this + // whole class of defect is made of. + throw new Error( + `${nodeModulesDir} is missing; install dependencies before checking template toolchains` + ); + } + + const resolved = new Map( + await Promise.all( + types.map( + async type => + [ + type, + await resolveStack(lisaDir, type), + ] as const satisfies readonly [string, ResolvedStack] + ) + ) + ); + + const governed = new Set( + [...resolved.values()].flatMap(stack => [...stack.packages]) + ); + const binaries = indexBinaries(governed, nodeModulesDir); + + return types.flatMap(stack => violationsForStack(stack, resolved, binaries)); +} + +/** + * Every violation one stack's resolved scripts carry. + * @param stack - Project type + * @param resolved - Every stack's resolution + * @param binaries - Governed binary to providing packages + * @returns Violations for this stack + */ +function violationsForStack( + stack: string, + resolved: ReadonlyMap, + binaries: ReadonlyMap +): readonly ToolchainViolation[] { + const resolution = resolved.get(stack); + if (resolution === undefined) return []; + const ancestors = ancestorsOf(stack, PROJECT_TYPE_HIERARCHY); + return Object.entries(resolution.scripts).flatMap(([scriptKey, command]) => + commandTools(command) + .filter(tool => isUnavailable(tool, resolution.packages, binaries)) + .map(tool => ({ + stack, + scriptKey, + command, + tool, + providers: binaries.get(tool) ?? [], + origin: originOf(scriptKey, command, stack, ancestors, resolved), + })) + ); +} + +/** + * Whether a tool is governed by some stack's pin yet absent from this one's. + * @param tool - Executable name + * @param packages - Packages the stack receives + * @param binaries - Governed binary to providing packages + * @returns True when the stack cannot run the tool + */ +function isUnavailable( + tool: string, + packages: ReadonlySet, + binaries: ReadonlyMap +): boolean { + const providers = binaries.get(tool) ?? []; + return ( + providers.length > 0 && !providers.some(provider => packages.has(provider)) + ); +} + +/** + * One violation as a single operator-readable line. + * @param violation - The violation + * @returns A line naming the stack, the script key, and the missing tool + */ +export function formatViolation(violation: ToolchainViolation): string { + const source = + violation.origin === violation.stack + ? "pinned by the stack itself" + : `inherited from ${violation.origin}`; + return `${violation.stack}: script "${violation.scriptKey}" runs \`${violation.tool}\` (${source}) but ${violation.stack} is given none of: ${violation.providers.join(", ")}`; +} diff --git a/tests/unit/config/template-script-toolchain.test.ts b/tests/unit/config/template-script-toolchain.test.ts new file mode 100644 index 0000000000..7f09e5aa9f --- /dev/null +++ b/tests/unit/config/template-script-toolchain.test.ts @@ -0,0 +1,212 @@ +/** + * A script pinned in a parent stack's `package.lisa.json` reaches every child + * stack, and can name a tool that stack cannot run — invisibly (#2848). + * + * The invisibility is the defect, not any one bad pin. `package.lisa.json` + * layers deep-merge parent into child, so a key added to the `typescript` + * template is written verbatim into six other stacks' `package.json`. Nothing + * says which six, and an unreferenced script proves nothing until something + * invokes it — which is how a Jest stack came to carry a vitest coverage + * command that only became a failure when a hook started resolving that key, + * four weeks later, in consumers. + * + * This is the class check. The pair-specific guard in + * `coverage-unit-script-runner-parity` still stands: it asserts something + * stronger about one pair of keys (same runner as each other) that no + * dependency-derived check can see. + * @module tests/unit/config/template-script-toolchain + */ + +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { + commandTools, + findToolchainViolations, + formatViolation, +} from "../../helpers/template-toolchain.js"; + +const REPO_ROOT = path.resolve(__dirname, "..", "..", ".."); + +/** + * The fixture tree is written under real type names because the resolver + * expands the shipped hierarchy — `typescript` is the parent layer and `expo` + * the inheriting child. Their contents are entirely synthetic. + */ +const FIXTURE_PARENT = "typescript"; +const FIXTURE_CHILD = "expo"; + +/** Synthetic tools and pins, chosen so nothing here matches a real package. */ +const TOOL_A = "runner-a"; +const TOOL_B = "runner-b"; +const RANGE = "^1.0.0"; +const SCRIPT_KEY = "test:cov:unit"; +const COMMAND_A = `${TOOL_A} run --coverage`; +const COMMAND_B = `${TOOL_B} run --coverage`; + +/** + * Write a synthetic template layer. + * @param root - Fixture lisaDir + * @param layer - Layer directory name + * @param template - Template body + */ +function writeLayer( + root: string, + layer: string, + template: Record +): void { + const dir = path.join(root, layer, "package-lisa"); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, "package.lisa.json"), + JSON.stringify(template) + ); +} + +describe("shipped package.lisa.json templates", () => { + it("never hand a stack a script whose tool it is not given", async () => { + const violations = await findToolchainViolations(REPO_ROOT); + expect(violations.map(formatViolation)).toStrictEqual([]); + }); + + it("refuse to return a verdict with no installed packages to read a bin from", async () => { + // Binary names come from each installed package's own `bin` field. Guessed + // from names instead, the map cannot see that `@playwright/test` provides + // `playwright`, and the check would report a stack that is fine as broken + // — or, worse, quietly return nothing at all. + await expect( + findToolchainViolations(path.join(REPO_ROOT, "no-such-lisa-dir")) + ).rejects.toThrow(/install dependencies/u); + }); +}); + +describe("commandTools", () => { + it("reads the executable through leading environment assignments", () => { + expect(commandTools("NODE_ENV=test jest --coverage")).toStrictEqual([ + "jest", + ]); + }); + + it("reads every command in a chain", () => { + expect(commandTools("oxlint && eslint . --quiet")).toStrictEqual([ + "oxlint", + "eslint", + ]); + }); + + it("does not split on a separator inside quotes", () => { + expect( + commandTools(`node -e "if (a) process.exit(0); process.exit(1);"`) + ).toStrictEqual(["node"]); + }); + + it("ignores a tool an on-demand runner fetches", () => { + expect(commandTools("npx some-cli@latest .")).toStrictEqual([]); + }); + + it("ignores a sibling script a package manager delegates to", () => { + expect( + commandTools("bun run build:dist && node scripts/x.mjs") + ).toStrictEqual(["node"]); + }); +}); + +describe("the check itself", () => { + // eslint-disable-next-line functional/no-let -- Fixture root is created once + let root = ""; + + beforeAll(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), "lisa-toolchain-")); + }); + + afterAll(() => { + fs.rmSync(root, { recursive: true, force: true }); + }); + + /** + * Run the check over the fixture tree. + * @returns Formatted violations + */ + const run = async (): Promise => + ( + await findToolchainViolations(root, { + types: [FIXTURE_PARENT, FIXTURE_CHILD], + nodeModulesDir: path.join(root, "node_modules"), + }) + ).map(formatViolation); + + it("fails when an inherited pin names a tool the child is not given, even though nothing invokes it", async () => { + writeLayer(root, FIXTURE_PARENT, { + force: { + scripts: { [SCRIPT_KEY]: COMMAND_A }, + devDependencies: { [TOOL_A]: RANGE }, + }, + }); + writeLayer(root, FIXTURE_CHILD, { + force: { devDependencies: { [TOOL_B]: RANGE } }, + remove: { devDependencies: [TOOL_A] }, + }); + + const violations = await run(); + + expect(violations).toHaveLength(1); + expect(violations[0]).toContain(FIXTURE_CHILD); + expect(violations[0]).toContain(SCRIPT_KEY); + expect(violations[0]).toContain(TOOL_A); + expect(violations[0]).toContain(`inherited from ${FIXTURE_PARENT}`); + }); + + it("exonerates a child that overrides the inherited script", async () => { + writeLayer(root, FIXTURE_PARENT, { + force: { + scripts: { [SCRIPT_KEY]: COMMAND_A }, + devDependencies: { [TOOL_A]: RANGE }, + }, + }); + writeLayer(root, FIXTURE_CHILD, { + force: { + scripts: { [SCRIPT_KEY]: COMMAND_B }, + devDependencies: { [TOOL_B]: RANGE }, + }, + remove: { devDependencies: [TOOL_A] }, + }); + + expect(await run()).toStrictEqual([]); + }); + + it("covers a newly added parent pin with no roster edited", async () => { + writeLayer(root, FIXTURE_PARENT, { + force: { + scripts: { + [SCRIPT_KEY]: COMMAND_B, + "audit:licences": `${TOOL_A} scan`, + }, + devDependencies: { [TOOL_A]: RANGE, [TOOL_B]: RANGE }, + }, + }); + writeLayer(root, FIXTURE_CHILD, { + force: { + scripts: { [SCRIPT_KEY]: COMMAND_B }, + devDependencies: { [TOOL_B]: RANGE }, + }, + remove: { devDependencies: [TOOL_A] }, + }); + + const violations = await run(); + + expect(violations).toHaveLength(1); + expect(violations[0]).toContain("audit:licences"); + }); + + it("ignores a tool no stack pins, which the host supplies", async () => { + writeLayer(root, FIXTURE_PARENT, { + force: { scripts: { typecheck: "tsc --noEmit" } }, + }); + writeLayer(root, FIXTURE_CHILD, { force: {} }); + + expect(await run()).toStrictEqual([]); + }); +}); diff --git a/typescript/package-lisa/package.lisa.json b/typescript/package-lisa/package.lisa.json index c194ce2535..cf48dc2bd9 100644 --- a/typescript/package-lisa/package.lisa.json +++ b/typescript/package-lisa/package.lisa.json @@ -64,7 +64,14 @@ }, "defaults": { "devDependencies": { - "@codyswann/lisa": "^2.106.0" + "@ast-grep/cli": "^0.40.4", + "@codyswann/lisa": "^2.106.0", + "@vitest/coverage-v8": "^4.1.0", + "eslint": "^9.39.0", + "husky": "^8.0.0", + "prettier": "^3.3.3", + "typescript": "^6.0.3", + "vitest": "^4.1.0" }, "scripts": { "build": "tsc", From d0c3e715dd2df421528878bb391c537076b5239e Mon Sep 17 00:00:00 2001 From: Cody Swann Date: Sat, 22 Aug 2026 01:38:22 -0400 Subject: [PATCH 2/2] fix(tests): read the two tool sources this check was guessing at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from review, and both are the failure this check exists to catch turned on the check itself: it reported success while proving nothing. 1. `bun run ` is not always script delegation. Bun's documented resolution order is scripts, then source files, then `node_modules/.bin`, then $PATH — so `bun run vitest` in a stack with no `vitest` script runs the vitest BINARY. Every `bun` command was treated as delegation, so `commandTools` returned no tool at all and a stack missing that dependency passed. Scoped to `bun` deliberately. `npm run `, `pnpm run` and `yarn run` fail when no such script exists rather than falling through to a binary, so for those the next word really is always a sibling script and reading it as a tool would invent violations. Sibling script names now reach `commandTools` from the call site, which already had them, so the distinction is made against the stack's real scripts rather than a guess. 2. `binariesOf` read `bin` only. npm also exposes every file in `directories.bin` as an executable, so a package declaring only that field appeared to provide nothing — its binary was governed by no package, and a child stack that dropped the dependency passed. Three tests, each confirmed to FAIL against the pre-fix helper and pass with it, with the whole file green either way otherwise (15 passed / 3 failed pre-fix, 15 passed post-fix): reads a binary `bun run` falls through to when no such script exists expected [] to strictly equal [ 'vitest' ] looks past flags between the verb and the binary expected [] to strictly equal [ 'vitest' ] sees the binaries a package exposes only through directories.bin expected [] to have a length of 1 but got +0 The third asserts a violation IS found, because the pre-fix failure direction is a false PASS, not a false alarm — a test asserting the clean case would have passed against the defect and pinned nothing. The `directories.bin` fixture uses package and binary names distinct from the other cases on purpose: the fixture root is shared across that block, and an earlier draft installed a manifest under a reused name and broke a neighbouring test that had been passing. 🤖 Generated with Claude Code Co-Authored-By: Claude Work-Item: CodySwannGT/lisa#2848 --- tests/helpers/template-toolchain.ts | 90 +++++++++++++++-- .../config/template-script-toolchain.test.ts | 96 ++++++++++++++++++- 2 files changed, 178 insertions(+), 8 deletions(-) diff --git a/tests/helpers/template-toolchain.ts b/tests/helpers/template-toolchain.ts index bed9dac030..e14e2478b0 100644 --- a/tests/helpers/template-toolchain.ts +++ b/tests/helpers/template-toolchain.ts @@ -143,24 +143,71 @@ function splitSegments(command: string): readonly string[] { * package-manager script delegation yield nothing: neither names a tool that * has to be pinned. * @param segment - A single command + * @param siblingScripts - Script names defined alongside this one * @returns The executable name, or null */ -function executableOf(segment: string): string | null { +function executableOf( + segment: string, + siblingScripts: ReadonlySet +): string | null { const words = segment.split(/\s+/u).filter(word => word.length > 0); - const head = words.find(word => !/^[A-Za-z_]\w*=/u.test(word)); + const meaningful = words.filter(word => !/^[A-Za-z_]\w*=/u.test(word)); + const head = meaningful[0]; if (head === undefined) return null; - if (ON_DEMAND_RUNNERS.has(head) || SCRIPT_DELEGATES.has(head)) return null; + if (ON_DEMAND_RUNNERS.has(head)) return null; + if (SCRIPT_DELEGATES.has(head)) { + return delegatedTool(head, meaningful.slice(1), siblingScripts); + } return head; } +/** + * The tool a package-manager delegation actually runs, if it is not a sibling + * script. + * @remarks + * `bun run ` is not always script delegation. Bun's documented resolution + * order falls through to `node_modules/.bin` when no script of that name + * exists, so `bun run vitest` in a stack with no `vitest` script runs the + * vitest BINARY — and treating every `bun` command as delegation let a missing + * `vitest` dependency pass this check entirely. That is the shape this whole + * suite exists to catch: a check that reports success while proving nothing. + * + * Deliberately `bun` only. `npm run `, `pnpm run ` and + * `yarn run ` fail when no such script exists rather than falling back to + * a binary, so for those the next word really is always a sibling script and + * treating it as a tool would invent violations. + * @param delegate - The package manager that heads the segment + * @param rest - The remaining words of the segment + * @param siblingScripts - Script names defined alongside this one + * @returns The binary name, or null when the word is a sibling script + */ +function delegatedTool( + delegate: string, + rest: readonly string[], + siblingScripts: ReadonlySet +): string | null { + if (delegate !== "bun") return null; + const [verb, ...after] = rest; + if (verb !== "run") return null; + // `bun run --silent vitest` — flags sit between the verb and the name. + const name = after.find(word => !word.startsWith("-")); + if (name === undefined) return null; + return siblingScripts.has(name) ? null : name; +} + /** * Every executable a script body invokes. * @param command - A `scripts` value + * @param siblingScripts - Script names defined alongside this one, so a + * package-manager delegation can be told from a binary of the same shape * @returns Executable names, deduplicated, in first-seen order */ -export function commandTools(command: string): readonly string[] { +export function commandTools( + command: string, + siblingScripts: ReadonlySet = new Set() +): readonly string[] { const named = splitSegments(command) - .map(executableOf) + .map(segment => executableOf(segment, siblingScripts)) .filter((tool): tool is string => tool !== null); return [...new Set(named)]; } @@ -188,14 +235,43 @@ function binariesOf(pkg: string, nodeModulesDir: string): readonly string[] { if (fs.existsSync(manifestPath)) { const manifest = JSON.parse(fs.readFileSync(manifestPath, "utf8")) as { readonly bin?: string | Record; + readonly directories?: { readonly bin?: string }; }; - if (manifest.bin === undefined) return []; + if (manifest.bin === undefined) { + // npm exposes every file in `directories.bin` as an executable, so a + // manifest that declares only that field still provides binaries. + // Reading `bin` alone reported none, which let a child stack omit the + // package and pass — the same false pass this check exists to prevent. + return directoryBinaries(path.join(nodeModulesDir, pkg), manifest); + } return typeof manifest.bin === "string" ? [pkg] : Object.keys(manifest.bin); } const tail = pkg.startsWith("@") ? (pkg.split("/")[1] ?? pkg) : pkg; return [...new Set([pkg, tail])]; } +/** + * Binaries an installed package exposes through `directories.bin`. + * @param packageDir - Where the package is installed + * @param manifest - Its parsed manifest + * @param manifest.directories - The manifest's `directories` field + * @param manifest.directories.bin - Directory whose files npm exposes as binaries + * @returns Every file name in the declared directory, or none + */ +function directoryBinaries( + packageDir: string, + manifest: { readonly directories?: { readonly bin?: string } } +): readonly string[] { + const declared = manifest.directories?.bin; + if (declared === undefined) return []; + const binDir = path.join(packageDir, declared); + if (!fs.existsSync(binDir)) return []; + return fs + .readdirSync(binDir, { withFileTypes: true }) + .filter(entry => !entry.isDirectory()) + .map(entry => entry.name); +} + /** * Index every governed binary to the packages that provide it. * @param packages - Every package any stack receives @@ -355,7 +431,7 @@ function violationsForStack( if (resolution === undefined) return []; const ancestors = ancestorsOf(stack, PROJECT_TYPE_HIERARCHY); return Object.entries(resolution.scripts).flatMap(([scriptKey, command]) => - commandTools(command) + commandTools(command, new Set(Object.keys(resolution.scripts))) .filter(tool => isUnavailable(tool, resolution.packages, binaries)) .map(tool => ({ stack, diff --git a/tests/unit/config/template-script-toolchain.test.ts b/tests/unit/config/template-script-toolchain.test.ts index 7f09e5aa9f..ce8341fd86 100644 --- a/tests/unit/config/template-script-toolchain.test.ts +++ b/tests/unit/config/template-script-toolchain.test.ts @@ -47,6 +47,15 @@ const SCRIPT_KEY = "test:cov:unit"; const COMMAND_A = `${TOOL_A} run --coverage`; const COMMAND_B = `${TOOL_B} run --coverage`; +/** + * A package whose binary is declared only through `directories.bin`, and the + * binary it exposes. Deliberately distinct from {@link TOOL_A} and + * {@link TOOL_B}: the fixture root is shared across this block, so reusing one + * of those would leak an installed manifest into the neighbouring cases. + */ +const DIRBIN_PACKAGE = "runner-dirbin"; +const DIRBIN_BINARY = "dirbin-cli"; + /** * Write a synthetic template layer. * @param root - Fixture lisaDir @@ -66,6 +75,36 @@ function writeLayer( ); } +/** + * Install a synthetic package into the fixture's `node_modules`. + * @param root - Fixture lisaDir + * @param pkg - Package name + * @param manifest - Its `package.json` body, beyond name and version + * @param binFiles - Files to create under `directories.bin`, if declared + */ +function installPackage( + root: string, + pkg: string, + manifest: Record, + binFiles: readonly string[] = [] +): void { + const dir = path.join(root, "node_modules", pkg); + const declared = (manifest as { directories?: { bin?: string } }).directories + ?.bin; + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, "package.json"), + JSON.stringify({ name: pkg, version: "1.0.0", ...manifest }) + ); + if (declared !== undefined) { + const binDir = path.join(dir, declared); + fs.mkdirSync(binDir, { recursive: true }); + for (const file of binFiles) { + fs.writeFileSync(path.join(binDir, file), "#!/usr/bin/env node\n"); + } + } +} + describe("shipped package.lisa.json templates", () => { it("never hand a stack a script whose tool it is not given", async () => { const violations = await findToolchainViolations(REPO_ROOT); @@ -109,9 +148,36 @@ describe("commandTools", () => { it("ignores a sibling script a package manager delegates to", () => { expect( - commandTools("bun run build:dist && node scripts/x.mjs") + commandTools( + "bun run build:dist && node scripts/x.mjs", + new Set(["build:dist"]) + ) ).toStrictEqual(["node"]); }); + + it("reads a binary `bun run` falls through to when no such script exists", () => { + // Bun's documented resolution order is scripts, then source files, then + // `node_modules/.bin`, then $PATH. So `bun run vitest` in a stack with no + // `vitest` script runs the vitest BINARY, and treating every `bun` command + // as script delegation let a stack missing that dependency pass this check + // reporting nothing at all. + expect( + commandTools("bun run vitest", new Set(["build:dist"])) + ).toStrictEqual(["vitest"]); + }); + + it("still ignores the delegated word for a manager with no binary fallback", () => { + // `npm run ` fails when no such script exists rather than falling + // through to a binary, so the next word really is always a sibling script. + // Reading it as a tool would invent a violation. + expect(commandTools("npm run vitest", new Set())).toStrictEqual([]); + }); + + it("looks past flags between the verb and the binary", () => { + expect(commandTools("bun run --silent vitest", new Set())).toStrictEqual([ + "vitest", + ]); + }); }); describe("the check itself", () => { @@ -159,6 +225,34 @@ describe("the check itself", () => { expect(violations[0]).toContain(`inherited from ${FIXTURE_PARENT}`); }); + it("sees the binaries a package exposes only through directories.bin", async () => { + // npm exposes every file in `directories.bin` as an executable. Reading + // only `bin`, this package appeared to provide NOTHING — so the binary its + // script invokes was governed by no package at all, and a child that drops + // the dependency passed a check whose entire job is to catch that. The + // failure direction matters: the check did not report a false violation, + // it reported success while proving nothing. + installPackage(root, DIRBIN_PACKAGE, { directories: { bin: "cli" } }, [ + DIRBIN_BINARY, + ]); + writeLayer(root, FIXTURE_PARENT, { + force: { + scripts: { [SCRIPT_KEY]: `${DIRBIN_BINARY} --check` }, + devDependencies: { [DIRBIN_PACKAGE]: RANGE }, + }, + }); + writeLayer(root, FIXTURE_CHILD, { + remove: { devDependencies: [DIRBIN_PACKAGE] }, + }); + + const violations = await run(); + + expect(violations).toHaveLength(1); + expect(violations[0]).toContain(FIXTURE_CHILD); + expect(violations[0]).toContain(DIRBIN_BINARY); + expect(violations[0]).toContain(DIRBIN_PACKAGE); + }); + it("exonerates a child that overrides the inherited script", async () => { writeLayer(root, FIXTURE_PARENT, { force: {