From 6fac85b480828c0668652ba07c6a4f79fdf0a91e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 12:04:27 +0000 Subject: [PATCH 1/2] chore(checks): add local quality checks and CI ratchets Adds scripts/checks: complexity, duplication and convention ratchets that the pre-commit hook runs on staged content and that CI runs against the base of the pull request, so a change can only lower the debt it touches. Extends lint-staged and the lint script to the scripts directory. The eslint config gains the segmentation feature boundary ahead of the module landing, a node-globals block for .mjs tooling, and warn-level limits that mirror the ratchet thresholds so editors surface them. Each pure layer is guarded by its one list of pure files: a pure file may import from its own feature only what is on that list, spelled from the alias, so the processing engine's sibling imports move to the alias. --- .github/workflows/checks.yml | 11 ++ CONTRIBUTING.md | 12 ++ eslint.config.js | 180 ++++++++++++++++++----- package-lock.json | 139 +++++++++++++++++ package.json | 7 +- scripts/checks/.gitignore | 1 + scripts/checks/check.mjs | 23 +++ scripts/checks/complexity.mjs | 147 ++++++++++++++++++ scripts/checks/config.mjs | 8 + scripts/checks/conventions.mjs | 43 ++++++ scripts/checks/duplication.mjs | 116 +++++++++++++++ scripts/checks/git.mjs | 61 ++++++++ src/processing/engine/formModel.ts | 2 +- src/processing/engine/jobDisplay.ts | 7 +- src/processing/engine/mintAnnotations.ts | 16 +- src/processing/engine/mintInput.ts | 4 +- src/processing/engine/mintLabelmap.ts | 12 +- src/processing/engine/sourceRefs.ts | 8 +- src/processing/engine/transport.ts | 4 +- 19 files changed, 745 insertions(+), 56 deletions(-) create mode 100644 scripts/checks/.gitignore create mode 100644 scripts/checks/check.mjs create mode 100755 scripts/checks/complexity.mjs create mode 100644 scripts/checks/config.mjs create mode 100644 scripts/checks/conventions.mjs create mode 100755 scripts/checks/duplication.mjs create mode 100644 scripts/checks/git.mjs diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 26b207ff6..07799f786 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -9,6 +9,8 @@ jobs: name: Code check and tests steps: - uses: actions/checkout@v4 + with: + fetch-depth: 0 - name: Setup node uses: actions/setup-node@v4 with: @@ -19,6 +21,15 @@ jobs: run: npx prettier --list-different "src/**/*.[jt]s" "tests/**/*.[jt]s" "src/**/*.vue" - name: Lint run: npm run lint + - name: Quality ratchets + # A file may not get worse than it is on the pull request's target branch. + if: github.event_name == 'pull_request' + env: + BASE_REF: ${{ github.base_ref }} + run: | + git fetch --no-tags origin \ + "+refs/heads/$BASE_REF:refs/remotes/origin/$BASE_REF" + node scripts/checks/check.mjs --base "origin/$BASE_REF" - name: Tests run: npm test diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 514674cf6..5e5a0c889 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,6 +75,18 @@ npm run lint --- +## Quality checks + +The pre-commit hook and CI run `node scripts/checks/check.mjs`. It compares the files you changed with the base (`HEAD` locally, the target branch in CI), so existing code is left alone and only a change that makes a file worse fails. + +- Complexity: in `src`, a function stays within cyclomatic complexity 10, nesting depth 3 and 4 parameters, and a file within 600 lines. A file already over a limit may not go further over, and a new file must be within all of them. +- Duplication: a change to the tests may not add a block of 60 or more tokens that repeats another test. +- Conventions: no new binary files outside the directories that already hold them, such as `tests/baseline`. + +The limits are in `scripts/checks/config.mjs`. Stage your changes before running the script by hand, since it reads the index. `CHECKS_SKIP=1 git commit` skips it for one commit. + +--- + ## Testing ```bash diff --git a/eslint.config.js b/eslint.config.js index 36dda8865..3476de387 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -1,3 +1,4 @@ +import path from 'node:path'; import js from '@eslint/js'; import eslintPluginVue from 'eslint-plugin-vue'; import tseslint from 'typescript-eslint'; @@ -21,15 +22,58 @@ import globals from 'globals'; // the full pattern set its files need, and no block silently erases another // feature's boundary. // --------------------------------------------------------------------------- -// `pure.upperModules` is a hand-maintained list of the feature's non-pure -// modules: a new one has to be added here or the pure layer may import it. +// `pure` lists the feature's pure files, and that one list is also everything +// a pure file may import from its own feature: a module that is not on it is +// denied without having to be named. A directory glob admits the directory. +const pureModules = (feature) => + feature.pure.map((file) => + file + .slice(`src/${feature.dir}/`.length) + .replace(/\/\*\*\/.*$/, '/**') + .replace(/\.\w+$/, '') + ); + +// Patterns follow gitignore, which cannot re-admit a path under a denied +// directory. So each directory on the way to a pure module denies its children +// and re-admits the ones that are pure or lead to one. +const ownFeatureExceptPure = (feature) => { + const root = `@/src/${feature.dir}`; + const admitted = new Set( + pureModules(feature).flatMap((module) => { + const parts = module.replace(/\/\*\*$/, '').split('/'); + return parts.map((_, depth) => parts.slice(0, depth + 1).join('/')); + }) + ); + const wholeDirs = pureModules(feature) + .filter((module) => module.endsWith('/**')) + .map((module) => module.slice(0, -3)); + const parentOf = (entry) => path.posix.dirname(entry).replace(/^\.$/, ''); + const denyingDirs = new Set( + [...admitted].map(parentOf).filter((dir) => !wholeDirs.includes(dir)) + ); + return [...denyingDirs] + .sort() + .flatMap((dir) => [ + `${root}/${dir ? `${dir}/` : ''}*`, + ...[...admitted] + .filter((entry) => parentOf(entry) === dir) + .map((entry) => `!${root}/${entry}`), + ]); +}; + +const upperLayerMessage = (feature) => + `The ${feature.dir} pure layer must not import stores, components, or upper feature modules. Dependencies point downward only.`; + const featureBoundaries = (features) => { const publicSurface = ({ dir }) => ({ group: [`@/src/${dir}/*`, `@/src/${dir}/*/**`, `!@/src/${dir}/index`], message: `Import the ${dir} feature only from its public surface \`@/src/${dir}\` (src/${dir}/index.ts). A deep import bypasses the feature boundary.`, }); + const publicFeatures = features.filter( + (feature) => feature.publicSurface !== false + ); const otherSurfaces = (feature) => - features.filter((other) => other !== feature).map(publicSurface); + publicFeatures.filter((other) => other !== feature).map(publicSurface); return [ { @@ -38,7 +82,7 @@ const featureBoundaries = (features) => { rules: { 'no-restricted-imports': [ 'error', - { patterns: features.map(publicSurface) }, + { patterns: publicFeatures.map(publicSurface) }, ], }, }, @@ -52,28 +96,37 @@ const featureBoundaries = (features) => { }, })), ...features.map((feature) => ({ - files: feature.pure.files, + files: feature.pure, ignores: ['**/__tests__/**'], rules: { 'no-restricted-imports': [ 'error', { - paths: ['pinia', 'vue'].map((name) => ({ - name, - message: `The ${feature.dir} pure layer must stay framework-free: no ${name}.`, - })), + paths: [ + ...['pinia', 'vue'].map((name) => ({ + name, + message: `The ${feature.dir} pure layer must stay framework-free: no ${name}.`, + })), + // The bare directory is the index, an upper module. + { + name: `@/src/${feature.dir}`, + message: upperLayerMessage(feature), + }, + ], patterns: [ { group: [ - ...feature.pure.upperModules.flatMap((mod) => [ - `@/src/${feature.dir}/${mod}`, - `./${mod}`, - `../${mod}`, - ]), + ...ownFeatureExceptPure(feature), '@/src/store/**', '@/src/components/**', + '@/src/composables/**', ], - message: `The ${feature.dir} pure layer must not import stores, components, or upper feature modules — dependencies point downward only.`, + message: upperLayerMessage(feature), + }, + { + // The list above can only judge the alias spelling. + regex: '^\\.\\.?(/|$)', + message: `The ${feature.dir} pure layer spells its imports from \`@/src/\`, so the pure-layer boundary can see them.`, }, ...otherSurfaces(feature), ], @@ -168,6 +221,17 @@ export default tseslint.config( '@typescript-eslint/no-unsafe-function-type': 'off', }, }, + // Node tooling (scripts/checks/, fixture servers). The block above matches + // only .js/.ts/.vue, so without this eslint reports `process`, `console` and + // `URL` as undefined in every .mjs file. + { + files: ['**/*.mjs'], + languageOptions: { + ecmaVersion: 'latest', + sourceType: 'module', + globals: globals.node, + }, + }, { files: ['**/tests/pageobjects/**/*.ts'], rules: { @@ -246,33 +310,77 @@ export default tseslint.config( }, }, ...featureBoundaries([ + { + dir: 'segmentation', + pure: [ + 'src/segmentation/geometry.ts', + 'src/segmentation/color.ts', + 'src/segmentation/model.ts', + 'src/segmentation/segment.ts', + 'src/segmentation/masks/storage.ts', + 'src/segmentation/masks/overlap.ts', + 'src/segmentation/masks/labelValue.ts', + 'src/segmentation/editing/algorithms/fillHoles.ts', + 'src/segmentation/editing/algorithms/fillHoles.worker.ts', + 'src/segmentation/editing/algorithms/gaussianSmooth.worker.ts', + ], + // Consumers import explicit modules; the pure-layer rule still applies. + publicSurface: false, + }, { dir: 'processing', - pure: { - files: [ - 'src/processing/engine/**/*.{js,ts}', - 'src/processing/types.ts', - 'src/processing/config.ts', - ], - upperModules: [ - 'store', - 'applyResults', - 'jobResultReview', - 'index', - 'components/**', - ], - }, + pure: [ + 'src/processing/engine/**/*.{js,ts}', + 'src/processing/types.ts', + 'src/processing/config.ts', + ], }, { dir: 'referenceLines', - pure: { - files: [ - 'src/referenceLines/geometry.ts', - 'src/referenceLines/crossings.ts', - ], - upperModules: ['store', 'index', 'useReferenceLines', 'components/**'], - }, + pure: [ + 'src/referenceLines/geometry.ts', + 'src/referenceLines/crossings.ts', + ], }, ]), + // Tests are excluded so this block never matches a file the `vi.mock` block + // above matches: flat config replaces a rule's options wholesale, so overlap + // would erase that rule rather than add to it. + { + files: ['src/**/*.{ts,vue}'], + ignores: ['src/**/__tests__/**', 'src/**/*.{spec,test}.{js,ts}'], + rules: { + 'no-restricted-syntax': [ + 'warn', + { + selector: + "CallExpression[callee.name='computed'][typeArguments.params.length>0]", + message: + 'Let computed() infer its type. An explicit generic goes stale and hides the inference errors it was meant to document.', + }, + ], + }, + }, + // Mirrors the limits scripts/checks/complexity.mjs enforces per commit. Warn + // level because the ratchet only fails a file whose debt grows, so existing + // files stay over these numbers without failing `npm run lint`. + { + files: ['src/**/*.{js,ts,vue}'], + ignores: [ + 'src/**/__tests__/**', + 'src/**/*.{spec,test}.{js,ts}', + 'src/**/*.d.ts', + '**/emscripten-build/**', + ], + rules: { + complexity: ['warn', 10], + 'max-depth': ['warn', 3], + 'max-params': ['warn', 4], + 'max-lines': [ + 'warn', + { max: 600, skipBlankLines: true, skipComments: true }, + ], + }, + }, eslintConfigPrettier ); diff --git a/package-lock.json b/package-lock.json index 9b8ba4996..c50b7d12a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -56,6 +56,7 @@ "globals": "^16.2.0", "happy-dom": "^20.8.9", "itk-wasm": "^1.0.0-b.199", + "jscpd": "5.1.2", "jszip": "3.10.1", "lint-staged": "16.2.7", "mitt": "^3.0.1", @@ -13943,6 +13944,144 @@ "js-yaml": "bin/js-yaml.js" } }, + "node_modules/jscpd": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd/-/jscpd-5.1.2.tgz", + "integrity": "sha512-7innyzaMstgJcluIwLxUESsP7znUFtRxiENhcUzv34aDrjzH3mJOF7m/5ecNyQGST8BBlmz8alpTuId0c6OdkQ==", + "dev": true, + "license": "MIT", + "bin": { + "jscpd": "run-jscpd.js" + }, + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://opencollective.com/jscpd" + }, + "optionalDependencies": { + "jscpd-darwin-arm64": "5.1.2", + "jscpd-darwin-x64": "5.1.2", + "jscpd-linux-arm64-gnu": "5.1.2", + "jscpd-linux-arm64-musl": "5.1.2", + "jscpd-linux-x64-gnu": "5.1.2", + "jscpd-linux-x64-musl": "5.1.2", + "jscpd-windows-arm64-msvc": "5.1.2", + "jscpd-windows-x64-msvc": "5.1.2" + } + }, + "node_modules/jscpd-darwin-arm64": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-darwin-arm64/-/jscpd-darwin-arm64-5.1.2.tgz", + "integrity": "sha512-6fYVCBHgE154jVVYcRA+9K17q5AXe0/V9wfPf3hBrDQMxqOiI+99ii2CfMT6OkcEtJULqpi+uJENCsjtYn+nmQ==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/jscpd-darwin-x64": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-darwin-x64/-/jscpd-darwin-x64-5.1.2.tgz", + "integrity": "sha512-udNKAqu7ty6NcHvSwaZ7qCktyN+JwET0Z81e6/s1WmRtHkdd6Hnv0Gix04gRN3aVkfqZzI6YKle0YnYPJe4hOA==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/jscpd-linux-arm64-gnu": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-linux-arm64-gnu/-/jscpd-linux-arm64-gnu-5.1.2.tgz", + "integrity": "sha512-FYX3IZPXwEooiDn7fQzHEytRbbsSBNqqHUEzxu32EKBXNyQ1Fx1/8hms26X1RFUJ80zpcKkRQCz1J3hPYsYhFA==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/jscpd-linux-arm64-musl": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-linux-arm64-musl/-/jscpd-linux-arm64-musl-5.1.2.tgz", + "integrity": "sha512-SuoKN9xv4pIopLS2yPx3E+ysnmZPNAKdxxAYTwvd6ixrEcO36xIKUma+DGKUw1PLXq1HO6A9M3FlqmnrTCnzYA==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/jscpd-linux-x64-gnu": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-linux-x64-gnu/-/jscpd-linux-x64-gnu-5.1.2.tgz", + "integrity": "sha512-1B+rqrw7Jt7/5Sye7ssedRnN8I1qatjUq9tFRcUQvlteR1OgW4lZuGV+DTYOgNaPNzeJN21aLzBGBa9OsxRWMA==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/jscpd-linux-x64-musl": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-linux-x64-musl/-/jscpd-linux-x64-musl-5.1.2.tgz", + "integrity": "sha512-46Astr0LnVh/prpqq5/Vehtx18MCvcs0KL8vi49kAIksOe6QdlQ/hdBKH8V61pAAmAgCr64hU9p0dv6DDKHJdg==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/jscpd-windows-arm64-msvc": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-windows-arm64-msvc/-/jscpd-windows-arm64-msvc-5.1.2.tgz", + "integrity": "sha512-19gpnY+Mid71EGGytBcFwsif/Y1+Vfu2Qp0IJ+L3WFD32Zui9O4vvASwvxocgA7z77kiZNh9zF5sZY9+oQB62A==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "win32" + ] + }, + "node_modules/jscpd-windows-x64-msvc": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/jscpd-windows-x64-msvc/-/jscpd-windows-x64-msvc-5.1.2.tgz", + "integrity": "sha512-RZlAnXP7Jmx39LBC66IIpFIGl/Bg4FcSkz4Jr6ThrDm5D1NvM201MDG70Z1i+N3wRrfI8UcDkWp5rsHzGHViSA==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "win32" + ] + }, "node_modules/jsesc": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/jsesc/-/jsesc-3.1.0.tgz", diff --git a/package.json b/package.json index 9fd4d0a52..71cfe5912 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "test:e2e:chrome": "cross-env VITE_SHOW_SAMPLE_DATA=true npm run build && wdio run ./wdio.chrome.conf.ts", "test:e2e:chrome:skip-build": "wdio run ./wdio.chrome.conf.ts", "test:e2e:dev": "cross-env VITE_SHOW_SAMPLE_DATA=true concurrently -P \"npm run dev\" \"wdio run ./wdio.dev.conf.ts --watch {@}\"", - "lint": "vue-tsc --noEmit && eslint \"src/**/*.{js,ts,vue}\" \"tests/**/*.{js,ts}\"", + "lint": "vue-tsc --noEmit && eslint \"src/**/*.{js,ts,vue}\" \"tests/**/*.{js,ts}\" \"scripts/**/*.mjs\"", "build:all": "npm run build:dicom && npm run build:resample && npm run build", "build:dicom": "itk-wasm -s src/io/itk-dicom/ build ", "build:dicom:debug": "itk-wasm -s src/io/itk-dicom/ build -- -DCMAKE_BUILD_TYPE=Debug", @@ -82,6 +82,7 @@ "globals": "^16.2.0", "happy-dom": "^20.8.9", "itk-wasm": "^1.0.0-b.199", + "jscpd": "5.1.2", "jszip": "3.10.1", "lint-staged": "16.2.7", "mitt": "^3.0.1", @@ -115,11 +116,11 @@ "zod": "^4.1.13" }, "gitHooks": { - "pre-commit": "lint-staged", + "pre-commit": "lint-staged && node scripts/checks/check.mjs", "commit-msg": "commitlint --edit $1" }, "lint-staged": { - "*.{js,jsx,ts,tsx,vue}": [ + "*.{js,jsx,ts,tsx,vue,mjs}": [ "eslint", "prettier --write" ] diff --git a/scripts/checks/.gitignore b/scripts/checks/.gitignore new file mode 100644 index 000000000..0a2101fab --- /dev/null +++ b/scripts/checks/.gitignore @@ -0,0 +1 @@ +/cache/ diff --git a/scripts/checks/check.mjs b/scripts/checks/check.mjs new file mode 100644 index 000000000..e4dd858b3 --- /dev/null +++ b/scripts/checks/check.mjs @@ -0,0 +1,23 @@ +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; + +if (process.env.CHECKS_SKIP === '1') { + console.log('Additional checks skipped (CHECKS_SKIP=1).'); +} else { + for (const script of [ + 'complexity.mjs', + 'duplication.mjs', + 'conventions.mjs', + ]) { + const result = spawnSync( + process.execPath, + [ + fileURLToPath(new URL(script, import.meta.url)), + ...process.argv.slice(2), + ], + { stdio: 'inherit' } + ); + if (result.error) throw result.error; + if (result.status !== 0) process.exitCode = 1; + } +} diff --git a/scripts/checks/complexity.mjs b/scripts/checks/complexity.mjs new file mode 100755 index 000000000..977054074 --- /dev/null +++ b/scripts/checks/complexity.mjs @@ -0,0 +1,147 @@ +#!/usr/bin/env node +// Complexity ratchet: a changed file may not carry more debt than it did at +// the base ref, and a new file must carry none. Debt is the sum of +// (measured - limit) over every violation, so growing an already oversized +// file fails even though its violation count stays at one. +// +// complexity.mjs --base main compares the index with merge-base(main, HEAD). +import { mkdtempSync, rmSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { git, materialize, stagedContext } from './git.mjs'; +import { limits as LIMITS } from './config.mjs'; +const SOURCE = /^src\/.*\.(ts|js|vue)$/; +const EXCLUDED = /(__tests__|\.(spec|test)\.|\.d\.ts$|emscripten-build)/; + +const { base, baseRef, changed: entries, worktree } = stagedContext(); +const sources = entries.filter( + ({ file }) => SOURCE.test(file) && !EXCLUDED.test(file) +); +const changed = sources.map(({ file }) => file); +if (changed.length === 0) process.exit(0); + +const req = createRequire(path.join(worktree, 'package.json')); +const { ESLint } = req('eslint'); +const vueParser = req('vue-eslint-parser'); +const { parser: tsParser } = req('typescript-eslint'); + +const rules = Object.fromEntries( + Object.entries(LIMITS).map(([rule, limit]) => [ + rule, + [ + 'error', + rule === 'max-lines' + ? { max: limit, skipBlankLines: true, skipComments: true } + : limit, + ], + ]) +); + +const lint = async (cwd) => { + const eslint = new ESLint({ + cwd, + overrideConfigFile: true, + errorOnUnmatchedPattern: false, + allowInlineConfig: false, + overrideConfig: [ + { + files: ['**/*.{ts,js,vue}'], + languageOptions: { + parser: vueParser, + parserOptions: { + parser: tsParser, + ecmaVersion: 'latest', + sourceType: 'module', + extraFileExtensions: ['.vue'], + }, + }, + rules, + }, + ], + }); + const results = await eslint.lintFiles(['**/*.{ts,js,vue}']); + return results.map((r) => ({ + file: path.relative(cwd, r.filePath), + messages: r.messages, + })); +}; + +// Core rule messages carry the measured value as "(N)" or "of N." and the +// limit as "Maximum allowed is N". +const debtOf = (message) => { + const measured = message.match(/\((\d+)\)|of (\d+)\./); + const limit = message.match(/Maximum allowed is (\d+)/); + if (!measured || !limit) + throw new Error(`Unrecognized rule diagnostic: ${message}`); + return Number(measured[1] ?? measured[2]) - Number(limit[1]); +}; + +const debtByFile = (results) => + Object.fromEntries( + results.map(({ file, messages }) => [ + file, + messages.reduce((acc, m) => { + if (!m.ruleId) throw new Error(`${file}:${m.line}: ${m.message}`); + const rule = m.ruleId; + return { ...acc, [rule]: (acc[rule] ?? 0) + debtOf(m.message) }; + }, {}), + ]) + ); + +const tmp = mkdtempSync(path.join(tmpdir(), 'ratchet-')); +try { + const candRoot = path.join(tmp, 'cand'); + const baseRoot = path.join(tmp, 'base'); + materialize( + candRoot, + sources.map(({ file }) => [file, git('show', `:${file}`)]) + ); + materialize( + baseRoot, + sources.map(({ file, before }) => [ + file, + before === undefined ? undefined : git('show', `${base}:${before}`), + ]) + ); + + const [cand, before] = await Promise.all([lint(candRoot), lint(baseRoot)]); + const candDebt = debtByFile(cand); + const baseDebt = debtByFile(before); + + const regressions = changed.flatMap((file) => + Object.entries(candDebt[file] ?? {}) + .filter(([rule, debt]) => debt > (baseDebt[file]?.[rule] ?? 0)) + .map(([rule, debt]) => ({ + file, + rule, + debt, + was: baseDebt[file]?.[rule] ?? 0, + })) + ); + + if (regressions.length > 0) { + console.error( + '\nComplexity ratchet: these files got worse than %s.\n', + baseRef + ); + regressions.forEach(({ file, rule, was, debt }) => { + console.error(` ${file} ${rule} debt ${was} -> ${debt}`); + const offending = cand.find((r) => r.file === file)?.messages ?? []; + offending + .filter((m) => (m.ruleId ?? 'parse-error') === rule) + .forEach((m) => console.error(` ${file}:${m.line} ${m.message}`)); + }); + console.error( + `\nLimits: ${Object.entries(LIMITS) + .map(([r, l]) => `${r} ${l}`) + .join( + ', ' + )}. Reduce the debt to at most the base value, or split the file.` + + `\nBypass additional checks for one commit with CHECKS_SKIP=1.\n` + ); + process.exitCode = 1; + } +} finally { + rmSync(tmp, { recursive: true, force: true }); +} diff --git a/scripts/checks/config.mjs b/scripts/checks/config.mjs new file mode 100644 index 000000000..d8346404c --- /dev/null +++ b/scripts/checks/config.mjs @@ -0,0 +1,8 @@ +export const limits = { + complexity: 10, + 'max-depth': 3, + 'max-params': 4, + 'max-lines': 600, +}; + +export const minCloneTokens = 60; diff --git a/scripts/checks/conventions.mjs b/scripts/checks/conventions.mjs new file mode 100644 index 000000000..8a85f726c --- /dev/null +++ b/scripts/checks/conventions.mjs @@ -0,0 +1,43 @@ +#!/usr/bin/env node +// A check that needs no judgement: no committed binaries outside the +// directories that hold them. +import { git, stagedContext } from './git.mjs'; + +// The directories that already track binaries: visual baselines, documentation +// and application images, the favicons, and the itk wasm build output. Editing +// one of those is routine; a binary anywhere else is what this check is for. +const BINARY_DIRS = [ + 'tests/baseline/', + 'docs/assets/', + 'docs/public/', + 'public/', + 'src/assets/', + 'src/io/itk-dicom/emscripten-build/', + 'src/io/resample/emscripten-build/', +]; +const { base } = stagedContext(); + +const failures = []; + +// git reports "-" for added and deleted lines when a blob is binary. +const numstat = git('diff', '--cached', '--numstat', base, '--'); +numstat + .split('\n') + .filter(Boolean) + .map((line) => line.split('\t')) + .filter(([added, removed, file]) => added === '-' && removed === '-' && file) + .filter(([, , file]) => !BINARY_DIRS.some((dir) => file.startsWith(dir))) + .forEach(([, , file]) => + failures.push( + `${file} is a binary file. Generate test data (tests/specs/syntheticDicom.ts) or download it lazily; committed binaries belong in ${BINARY_DIRS.join(', ')}.` + ) + ); + +if (failures.length > 0) { + console.error('\nConventions:\n'); + failures.forEach((failure) => console.error(` ${failure}`)); + console.error( + '\nBypass additional checks for one commit with CHECKS_SKIP=1.\n' + ); + process.exitCode = 1; +} diff --git a/scripts/checks/duplication.mjs b/scripts/checks/duplication.mjs new file mode 100755 index 000000000..cd5835261 --- /dev/null +++ b/scripts/checks/duplication.mjs @@ -0,0 +1,116 @@ +#!/usr/bin/env node +// Test duplication ratchet: the staged tests may not introduce a clone that +// did not already exist at the base ref. +// +// duplication.mjs --base main compares the index with merge-base(main, HEAD). +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { createRequire } from 'node:module'; +import { git, materialize, paths, stagedContext } from './git.mjs'; +import { minCloneTokens as MIN_TOKENS } from './config.mjs'; + +const TEST = /(__tests__\/.*|\.(spec|test)|^tests\/.*)\.[cm]?[jt]sx?$/; + +const { base, baseRef, changed: entries, worktree } = stagedContext(); +const changed = entries.filter(({ file }) => TEST.test(file)); +if (changed.length === 0) process.exit(0); + +const ts = createRequire(path.join(worktree, 'package.json'))('typescript'); +const indexTests = paths(git('ls-files', '-z')).filter((f) => TEST.test(f)); +const baseTests = paths(git('ls-tree', '-rz', '--name-only', base)).filter( + (f) => TEST.test(f) +); + +const withoutImports = (file, text) => { + const source = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, false); + let result = text; + // Blank complete imports before tokenization, preserving diagnostic line numbers. + [...source.statements].reverse().forEach((statement) => { + if ( + !ts.isImportDeclaration(statement) && + !ts.isImportEqualsDeclaration(statement) + ) + return; + const start = statement.getStart(source); + const end = statement.end; + result = + result.slice(0, start) + + result.slice(start, end).replace(/[^\r\n]/g, ' ') + + result.slice(end); + }); + return result; +}; + +// jscpd reports a basename with no directory, and this repo has test files +// that share one, so each file is materialized under a flattened name that +// round-trips to its real path. +const flatten = (file) => file.replaceAll('/', '%'); +const unflatten = (name) => name.replaceAll('%', '/'); + +const jscpd = path.join(worktree, 'node_modules/.bin/jscpd'); +const run = (root, args) => + execFileSync( + jscpd, + [ + '--min-tokens', + String(MIN_TOKENS), + '--format', + 'typescript,tsx,javascript,jsx', + ...args, + '.', + ], + { cwd: root, encoding: 'utf8', stdio: ['ignore', 'ignore', 'pipe'] } + ); + +const tmp = mkdtempSync(path.join(tmpdir(), 'dupes-')); +try { + const candRoot = path.join(tmp, 'cand'); + const baseRoot = path.join(tmp, 'base'); + const baseline = path.join(tmp, 'baseline.json'); + const report = path.join(tmp, 'report'); + + materialize( + candRoot, + indexTests.map((f) => [flatten(f), withoutImports(f, git('show', `:${f}`))]) + ); + materialize( + baseRoot, + baseTests.map((f) => [ + flatten(f), + withoutImports(f, git('show', `${base}:${f}`)), + ]) + ); + + // Fingerprints are content-derived, so a baseline taken from the base tree + // identifies exactly the clones the staged tests add. + run(baseRoot, ['--baseline', baseline, '--update-baseline', '-r', 'silent']); + run(candRoot, ['--baseline', baseline, '-r', 'json', '-o', report]); + + const { duplicates } = JSON.parse( + readFileSync(path.join(report, 'jscpd-report.json'), 'utf8') + ); + const added = duplicates.filter((clone) => clone.isNew); + + if (added.length > 0) { + console.error( + '\nTest duplication ratchet: these clones are new since %s.\n', + baseRef + ); + added.forEach(({ firstFile, secondFile, tokens }) => { + console.error( + ` ${unflatten(firstFile.name)}:${firstFile.start}-${firstFile.end}` + + ` == ${unflatten(secondFile.name)}:${secondFile.start}-${secondFile.end}` + + ` (${tokens} tokens)` + ); + }); + console.error( + `\nA clone is ${MIN_TOKENS}+ identical tokens. Pull the shared setup into a helper the existing specs already use.` + + `\nBypass additional checks for one commit with CHECKS_SKIP=1.\n` + ); + process.exitCode = 1; + } +} finally { + rmSync(tmp, { recursive: true, force: true }); +} diff --git a/scripts/checks/git.mjs b/scripts/checks/git.mjs new file mode 100644 index 000000000..225592ba6 --- /dev/null +++ b/scripts/checks/git.mjs @@ -0,0 +1,61 @@ +import { execFileSync } from 'node:child_process'; +import { mkdirSync, writeFileSync } from 'node:fs'; +import path from 'node:path'; +import { parseArgs } from 'node:util'; + +export const git = (...args) => + execFileSync('git', args, { + encoding: 'utf8', + maxBuffer: 1 << 28, + stdio: ['ignore', 'pipe', 'pipe'], + }); + +export const paths = (text) => text.split('\0').filter(Boolean); + +export const resolveCommit = (ref) => + git('rev-parse', '--verify', '--end-of-options', `${ref}^{commit}`).trim(); + +export function changedFiles(args) { + const entries = paths( + git( + 'diff', + '--name-status', + '-z', + '--find-renames', + '--diff-filter=AMRT', + ...args, + '--' + ) + ); + const changed = []; + for (let i = 0; i < entries.length; ) { + const status = entries[i++]; + const previous = entries[i++]; + const file = status.startsWith('R') ? entries[i++] : previous; + changed.push({ file, before: status === 'A' ? undefined : previous }); + } + return changed; +} + +export function stagedContext() { + const { values } = parseArgs({ options: { base: { type: 'string' } } }); + const worktree = git('rev-parse', '--show-toplevel').trim(); + process.chdir(worktree); + const baseRef = values.base ?? 'HEAD'; + const base = + baseRef === 'HEAD' + ? resolveCommit('HEAD') + : git('merge-base', resolveCommit(baseRef), 'HEAD').trim(); + const changed = changedFiles(['--cached', base]); + return { base, baseRef, changed, worktree }; +} + +export function materialize(root, entries) { + mkdirSync(root, { recursive: true }); + for (const [file, text] of entries) { + if (text === undefined) continue; + const target = path.join(root, file); + mkdirSync(path.dirname(target), { recursive: true }); + writeFileSync(target, text); + } +} diff --git a/src/processing/engine/formModel.ts b/src/processing/engine/formModel.ts index cf74c0884..53da2cf40 100644 --- a/src/processing/engine/formModel.ts +++ b/src/processing/engine/formModel.ts @@ -2,7 +2,7 @@ import { taskParameterSchema } from '@/backend-contract'; import type { VolViewTaskParameter } from '@/backend-contract'; import { isRecord } from '@/src/utils'; import type { ProcessingValue } from '@/src/processing/types'; -import type { TaskSpecEnvelope } from './taskSpec'; +import type { TaskSpecEnvelope } from '@/src/processing/engine/taskSpec'; export type FormField = VolViewTaskParameter; diff --git a/src/processing/engine/jobDisplay.ts b/src/processing/engine/jobDisplay.ts index b4ca9bff0..75ab4831a 100644 --- a/src/processing/engine/jobDisplay.ts +++ b/src/processing/engine/jobDisplay.ts @@ -6,8 +6,11 @@ import type { SubmittedJobParameterDisplay, } from '@/src/processing/types'; import { plural } from '@/src/utils'; -import { fieldLabel, type TaskFormModel } from './formModel'; -import type { BoundSourceRefType } from './sourceRefs'; +import { + fieldLabel, + type TaskFormModel, +} from '@/src/processing/engine/formModel'; +import type { BoundSourceRefType } from '@/src/processing/engine/sourceRefs'; // Everything the display strings are made of, resolved by the caller in one // synchronous pass so formatting stays pure. diff --git a/src/processing/engine/mintAnnotations.ts b/src/processing/engine/mintAnnotations.ts index 72495f191..5003b7f6f 100644 --- a/src/processing/engine/mintAnnotations.ts +++ b/src/processing/engine/mintAnnotations.ts @@ -4,9 +4,19 @@ import type { InputValue } from '@/backend-contract'; import { TYPE_TAG_ANNOTATIONS } from '@/backend-contract'; -import type { FormValidationIssue, TaskFormModel } from './formModel'; -import type { SourceRefBindingState, SourceRefField } from './mintInput'; -import { ambiguousBinding, sourceRefFields, unboundBinding } from './mintInput'; +import type { + FormValidationIssue, + TaskFormModel, +} from '@/src/processing/engine/formModel'; +import type { + SourceRefBindingState, + SourceRefField, +} from '@/src/processing/engine/mintInput'; +import { + ambiguousBinding, + sourceRefFields, + unboundBinding, +} from '@/src/processing/engine/mintInput'; export const annotationsInputFields = ( model: TaskFormModel diff --git a/src/processing/engine/mintInput.ts b/src/processing/engine/mintInput.ts index 2fff4ab2f..5d4c56984 100644 --- a/src/processing/engine/mintInput.ts +++ b/src/processing/engine/mintInput.ts @@ -9,8 +9,8 @@ import type { FormField, FormValidationIssue, TaskFormModel, -} from './formModel'; -import { fieldLabel } from './formModel'; +} from '@/src/processing/engine/formModel'; +import { fieldLabel } from '@/src/processing/engine/formModel'; // A partial uri set would be processed as an incomplete volume. export const collectProvenanceUris = (ds: DataSource | undefined): string[] => { diff --git a/src/processing/engine/mintLabelmap.ts b/src/processing/engine/mintLabelmap.ts index 70e368c1e..30c7a08ab 100644 --- a/src/processing/engine/mintLabelmap.ts +++ b/src/processing/engine/mintLabelmap.ts @@ -1,14 +1,20 @@ import type { InputValue } from '@/backend-contract'; import { TYPE_TAG_LABELMAP } from '@/backend-contract'; import type { DataSource } from '@/src/io/import/dataSource'; -import type { FormValidationIssue, TaskFormModel } from './formModel'; -import type { SourceRefBindingState, SourceRefField } from './mintInput'; +import type { + FormValidationIssue, + TaskFormModel, +} from '@/src/processing/engine/formModel'; +import type { + SourceRefBindingState, + SourceRefField, +} from '@/src/processing/engine/mintInput'; import { ambiguousBinding, mintInputValue, sourceRefFields, unboundBinding, -} from './mintInput'; +} from '@/src/processing/engine/mintInput'; export const labelmapInputFields = (model: TaskFormModel): SourceRefField[] => sourceRefFields(model, TYPE_TAG_LABELMAP); diff --git a/src/processing/engine/sourceRefs.ts b/src/processing/engine/sourceRefs.ts index bdecb42d5..627660ad3 100644 --- a/src/processing/engine/sourceRefs.ts +++ b/src/processing/engine/sourceRefs.ts @@ -4,25 +4,25 @@ import { TYPE_TAG_LABELMAP, } from '@/backend-contract'; import type { DataSource } from '@/src/io/import/dataSource'; -import type { TaskFormModel } from './formModel'; +import type { TaskFormModel } from '@/src/processing/engine/formModel'; import { bindMintedImageInputs, mintInputValue, type ImageBindingResult, type SourceRefBindingState, type SourceRefField, -} from './mintInput'; +} from '@/src/processing/engine/mintInput'; import { bindResolvedLabelmapInputs, mintLabelmapReferenceImage, resolveLabelmapGroups, type LabelmapBindingResult, type SegmentGroupView, -} from './mintLabelmap'; +} from '@/src/processing/engine/mintLabelmap'; import { bindAnnotationsInputs, type AnnotationsBindingResult, -} from './mintAnnotations'; +} from '@/src/processing/engine/mintAnnotations'; export type BoundSourceRefType = | typeof TYPE_TAG_IMAGE diff --git a/src/processing/engine/transport.ts b/src/processing/engine/transport.ts index 26d210c9b..f1b1bb465 100644 --- a/src/processing/engine/transport.ts +++ b/src/processing/engine/transport.ts @@ -8,7 +8,7 @@ import type { ProcessingProviderConfig, TaskSummary, } from '@/src/processing/types'; -import { parseTaskSpecEnvelope } from './taskSpec'; +import { parseTaskSpecEnvelope } from '@/src/processing/engine/taskSpec'; import { parseJobHistoryPage, parseJobHistoryDetail, @@ -16,7 +16,7 @@ import { parseJobStatus, parseResults, parseStageResponse, -} from './wire'; +} from '@/src/processing/engine/wire'; // One malformed task summary must not kill the whole picker. const taskSummarySchema = z.object({ From cff38732a8bbb0811073b43e1d1d34fb6635f0f5 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Mon, 21 Sep 2026 15:39:12 -0400 Subject: [PATCH 2/2] fix(checks): allow deleting binary files --- scripts/checks/conventions.mjs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/checks/conventions.mjs b/scripts/checks/conventions.mjs index 8a85f726c..0d34fb75c 100644 --- a/scripts/checks/conventions.mjs +++ b/scripts/checks/conventions.mjs @@ -20,7 +20,14 @@ const { base } = stagedContext(); const failures = []; // git reports "-" for added and deleted lines when a blob is binary. -const numstat = git('diff', '--cached', '--numstat', base, '--'); +const numstat = git( + 'diff', + '--cached', + '--numstat', + '--diff-filter=AMRT', + base, + '--' +); numstat .split('\n') .filter(Boolean)