From 07119ce61d45e61c5e6b8fb63bfb0ca934ff9c96 Mon Sep 17 00:00:00 2001 From: Declan Carroll Date: Thu, 4 Jun 2026 09:27:01 +0100 Subject: [PATCH] refactor: Consolidate janitor + code-health onto a shared rules-engine AST substrate (#31417) Co-authored-by: Claude Opus 4.8 (1M context) --- packages/testing/code-health/package.json | 1 + packages/testing/code-health/src/index.ts | 10 + .../endpoint-scope-coverage.rule.test.ts | 60 +++++ .../src/rules/endpoint-scope-coverage.rule.ts | 67 +++++ packages/testing/janitor/src/cli.ts | 30 +-- .../janitor/src/cli/arg-parser.test.ts | 14 - .../testing/janitor/src/cli/arg-parser.ts | 10 - packages/testing/janitor/src/cli/help.ts | 3 - packages/testing/janitor/src/config.ts | 6 + packages/testing/janitor/src/core/reporter.ts | 56 +--- .../testing/janitor/src/core/rule-runner.ts | 255 +++++------------- .../testing/janitor/src/core/tcr-executor.ts | 4 +- packages/testing/janitor/src/index.ts | 11 +- .../janitor/src/rules/api-purity.rule.test.ts | 14 +- .../janitor/src/rules/api-purity.rule.ts | 21 +- .../testing/janitor/src/rules/base-rule.ts | 127 --------- .../rules/boundary-protection.rule.test.ts | 95 ++----- .../src/rules/boundary-protection.rule.ts | 86 +++--- .../janitor/src/rules/dead-code.rule.test.ts | 24 +- .../janitor/src/rules/dead-code.rule.ts | 171 ++---------- .../src/rules/deduplication.rule.test.ts | 10 +- .../janitor/src/rules/deduplication.rule.ts | 17 +- .../src/rules/duplicate-logic.rule.test.ts | 14 +- .../janitor/src/rules/duplicate-logic.rule.ts | 21 +- packages/testing/janitor/src/rules/index.ts | 3 +- .../no-direct-page-instantiation.rule.test.ts | 14 +- .../no-direct-page-instantiation.rule.ts | 22 +- .../src/rules/no-page-in-flow.rule.test.ts | 14 +- .../janitor/src/rules/no-page-in-flow.rule.ts | 21 +- .../src/rules/scope-lockdown.rule.test.ts | 16 +- .../janitor/src/rules/scope-lockdown.rule.ts | 19 +- .../src/rules/selector-purity.rule.test.ts | 16 +- .../janitor/src/rules/selector-purity.rule.ts | 23 +- .../src/rules/test-data-hygiene.rule.ts | 59 +--- packages/testing/janitor/src/types.ts | 23 -- packages/testing/rules-engine/package.json | 13 + .../testing/rules-engine/src/ast/ast-rule.ts | 63 +++++ .../rules-engine/src/ast/decorators.ts | 24 ++ .../testing/rules-engine/src/ast/index.ts | 4 + .../testing/rules-engine/src/ast/project.ts | 51 ++++ pnpm-lock.yaml | 7 + 41 files changed, 646 insertions(+), 873 deletions(-) create mode 100644 packages/testing/code-health/src/rules/endpoint-scope-coverage.rule.test.ts create mode 100644 packages/testing/code-health/src/rules/endpoint-scope-coverage.rule.ts delete mode 100644 packages/testing/janitor/src/rules/base-rule.ts create mode 100644 packages/testing/rules-engine/src/ast/ast-rule.ts create mode 100644 packages/testing/rules-engine/src/ast/decorators.ts create mode 100644 packages/testing/rules-engine/src/ast/index.ts create mode 100644 packages/testing/rules-engine/src/ast/project.ts diff --git a/packages/testing/code-health/package.json b/packages/testing/code-health/package.json index 7fcbc703f1e..0dac56512ac 100644 --- a/packages/testing/code-health/package.json +++ b/packages/testing/code-health/package.json @@ -32,6 +32,7 @@ "@n8n/vitest-config": "workspace:*", "@vitest/coverage-v8": "catalog:", "fast-glob": "catalog:", + "ts-morph": "catalog:", "tsx": "catalog:", "typescript": "catalog:", "vitest": "catalog:", diff --git a/packages/testing/code-health/src/index.ts b/packages/testing/code-health/src/index.ts index 2efe8f746fc..c38419c1f2b 100644 --- a/packages/testing/code-health/src/index.ts +++ b/packages/testing/code-health/src/index.ts @@ -3,12 +3,14 @@ import type { RuleSettingsMap } from '@n8n/rules-engine'; import type { CodeHealthContext } from './context.js'; import { CatalogViolationsRule } from './rules/catalog-violations.rule.js'; +import { EndpointScopeCoverageRule } from './rules/endpoint-scope-coverage.rule.js'; import { MigrationTimestampRule } from './rules/migration-timestamp.rule.js'; import { StaleOverridesRule } from './rules/stale-overrides.rule.js'; import { WorkflowPrTargetSafetyRule } from './rules/workflow-pr-target-safety.rule.js'; export type { CodeHealthContext } from './context.js'; export { CatalogViolationsRule } from './rules/catalog-violations.rule.js'; +export { EndpointScopeCoverageRule } from './rules/endpoint-scope-coverage.rule.js'; export { MigrationTimestampRule } from './rules/migration-timestamp.rule.js'; export { StaleOverridesRule } from './rules/stale-overrides.rule.js'; export { WorkflowPrTargetSafetyRule } from './rules/workflow-pr-target-safety.rule.js'; @@ -34,6 +36,13 @@ const defaultRuleSettings: RuleSettingsMap = { severity: 'warning', options: { workspaceFile: 'pnpm-workspace.yaml', lockFile: 'pnpm-lock.yaml' }, }, + 'endpoint-scope-coverage': { + // Disabled by default: enabling gates CI on ~129 existing authenticated-unscoped + // routes, which must first be reviewed and grandfathered into the baseline. + enabled: false, + severity: 'warning', + options: { packages: ['packages/cli'] }, + }, }; function mergeSettings(defaults: RuleSettingsMap, overrides?: RuleSettingsMap): RuleSettingsMap { @@ -55,6 +64,7 @@ export function createDefaultRunner(settings?: RuleSettingsMap): RuleRunner { + const rule = new EndpointScopeCoverageRule(); + + const analyze = (code: string) => { + const project = createInMemoryProject(); + project.createSourceFile('x.controller.ts', code); + return rule.analyzeProject(project); + }; + + it('flags only the authenticated, unscoped route on a controller', () => { + const violations = analyze(SOURCE); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('XController.unscoped'); + }); + + it('does not flag scoped, public, or non-controller routes', () => { + const messages = analyze(SOURCE) + .map((v) => v.message) + .join('\n'); + + expect(messages).not.toContain('XController.scoped'); // has @ProjectScope + expect(messages).not.toContain('XController.pub'); // skipAuth + expect(messages).not.toContain('XController.unauth'); // allowUnauthenticated + expect(messages).not.toContain('XController.apiKey'); // apiKeyAuth + expect(messages).not.toContain('NotAController'); // not a @RestController + }); +}); diff --git a/packages/testing/code-health/src/rules/endpoint-scope-coverage.rule.ts b/packages/testing/code-health/src/rules/endpoint-scope-coverage.rule.ts new file mode 100644 index 00000000000..d2850b6707f --- /dev/null +++ b/packages/testing/code-health/src/rules/endpoint-scope-coverage.rule.ts @@ -0,0 +1,67 @@ +import type { Violation } from '@n8n/rules-engine'; +import { + AstRule, + classHasDecorator, + getDecoratorByName, + getDecoratorObjectFlag, +} from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; +import type { Project } from 'ts-morph'; + +import type { CodeHealthContext } from '../context.js'; + +const CONTROLLER_DECORATORS = ['RestController', 'RootLevelController']; +const ROUTE_DECORATORS = ['Get', 'Post', 'Put', 'Patch', 'Delete', 'Head', 'Options']; +const SCOPE_DECORATORS = ['GlobalScope', 'ProjectScope']; +/** Route options that opt out of requiring an authenticated, scoped user. */ +const PUBLIC_FLAGS = ['skipAuth', 'allowUnauthenticated', 'apiKeyAuth']; + +/** + * Flags authenticated controller routes with no `@GlobalScope`/`@ProjectScope`. + * Baseline-gated: the existing (largely intentional) set is grandfathered, so + * only newly-added unscoped routes fail. + */ +export class EndpointScopeCoverageRule extends AstRule { + readonly id = 'endpoint-scope-coverage'; + readonly name = 'Endpoint Scope Coverage'; + readonly description = + 'Authenticated controller routes should declare an RBAC scope; new unscoped routes are flagged (existing ones grandfathered via baseline)'; + readonly severity = 'warning' as const; + + protected projectConfig(): AstProjectConfig { + const packages = (this.getOptions().packages as string[]) ?? ['packages/cli']; + return { packages }; + } + + analyze(context: CodeHealthContext): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + /** Exposed for unit tests against an in-memory project. */ + analyzeProject(project: Project): Violation[] { + const violations: Violation[] = []; + + for (const file of project.getSourceFiles()) { + for (const cls of file.getClasses()) { + if (!classHasDecorator(cls, CONTROLLER_DECORATORS)) continue; + + for (const method of cls.getMethods()) { + const route = getDecoratorByName(method, ROUTE_DECORATORS); + if (!route) continue; + if (getDecoratorByName(method, SCOPE_DECORATORS)) continue; + if (PUBLIC_FLAGS.some((flag) => getDecoratorObjectFlag(route, 1, flag))) continue; + + violations.push( + this.nodeViolation( + route, + `Authenticated route ${cls.getName()}.${method.getName()} declares no @GlobalScope/@ProjectScope.`, + 'Add a scope decorator, or mark the route { skipAuth: true } / { allowUnauthenticated: true } if it is intentionally public.', + ), + ); + } + } + } + + return violations; + } +} diff --git a/packages/testing/janitor/src/cli.ts b/packages/testing/janitor/src/cli.ts index fe570b73f1f..37e03488896 100644 --- a/packages/testing/janitor/src/cli.ts +++ b/packages/testing/janitor/src/cli.ts @@ -80,7 +80,7 @@ import { } from './core/method-usage-analyzer.js'; import { orchestrate } from './core/orchestrator.js'; import { createProject } from './core/project-loader.js'; -import { toJSON, toConsole, printFixResults } from './core/reporter.js'; +import { toJSON, toConsole } from './core/reporter.js'; import { filterToFailedSpecs } from './core/retry-filter.js'; import { computeScope, formatScope } from './core/scope-analyzer.js'; import { TcrExecutor, formatTcrResultConsole, formatTcrResultJSON } from './core/tcr-executor.js'; @@ -290,9 +290,7 @@ function runTcr(options: CliOptions): void { function runAnalyze(options: CliOptions): void { const config = getConfig(); const runner = createDefaultRunner(); - - // Create project - const { project, root } = createProject(config.rootDir); + const root = config.rootDir; // Build run options const runOptions: RunOptions = {}; @@ -301,11 +299,6 @@ function runAnalyze(options: CliOptions): void { runOptions.files = options.files; } - if (options.fix) { - runOptions.fix = true; - runOptions.write = options.write; - } - // Pass rule-specific config if (options.allowInExpect) { runOptions.ruleConfig = { @@ -315,21 +308,20 @@ function runAnalyze(options: CliOptions): void { // Run analysis let report = options.rule - ? runner.runRule(project, root, options.rule, runOptions) - : runner.run(project, root, runOptions); + ? runner.runRule(options.rule, { rootDir: root }, runOptions) + : runner.run({ rootDir: root }, runOptions); if (!report) { console.error('Failed to generate report'); process.exit(1); } - // Auto-filter by baseline if present (unless --ignore-baseline or --fix) + // Auto-filter by baseline if present (unless --ignore-baseline) const baseline = loadBaseline(config.rootDir); let baselineFiltered = false; const originalViolations = report.summary.totalViolations; - if (baseline && !options.fix && !options.ignoreBaseline) { - // Don't filter during fix mode or when baseline is ignored + if (baseline && !options.ignoreBaseline) { report = filterReportByBaseline(report, baseline, config.rootDir); baselineFiltered = true; } @@ -350,14 +342,10 @@ function runAnalyze(options: CliOptions): void { } toConsole(report, options.verbose); - - if (options.fix) { - printFixResults(report, options.write); - } } // Exit with error code if violations found - if (report.summary.totalViolations > 0 && !(options.fix && options.write)) { + if (report.summary.totalViolations > 0) { process.exit(1); } } @@ -365,12 +353,10 @@ function runAnalyze(options: CliOptions): void { function runBaseline(options: CliOptions): void { const config = getConfig(); - // Create runner and project const runner = createDefaultRunner(); - const { project, root } = createProject(config.rootDir); // Run full analysis (no file filter, no baseline filter) - const report = runner.run(project, root, {}); + const report = runner.run({ rootDir: config.rootDir }, {}); if (!report) { console.error('Failed to generate report'); diff --git a/packages/testing/janitor/src/cli/arg-parser.test.ts b/packages/testing/janitor/src/cli/arg-parser.test.ts index f81cd88edde..01d5a062e9c 100644 --- a/packages/testing/janitor/src/cli/arg-parser.test.ts +++ b/packages/testing/janitor/src/cli/arg-parser.test.ts @@ -82,18 +82,6 @@ describe('arg-parser', () => { expect(result.verbose).toBe(true); }); - it('parses --fix', () => { - setArgs(['--fix']); - const result = parseArgs(); - expect(result.fix).toBe(true); - }); - - it('parses --write', () => { - setArgs(['--write']); - const result = parseArgs(); - expect(result.write).toBe(true); - }); - it('parses --list', () => { setArgs(['--list']); const result = parseArgs(); @@ -259,8 +247,6 @@ describe('arg-parser', () => { expect(result.files).toEqual([]); expect(result.json).toBe(false); expect(result.verbose).toBe(false); - expect(result.fix).toBe(false); - expect(result.write).toBe(false); expect(result.help).toBe(false); expect(result.list).toBe(false); expect(result.execute).toBe(false); diff --git a/packages/testing/janitor/src/cli/arg-parser.ts b/packages/testing/janitor/src/cli/arg-parser.ts index a16a5d4e681..50c35ab637a 100644 --- a/packages/testing/janitor/src/cli/arg-parser.ts +++ b/packages/testing/janitor/src/cli/arg-parser.ts @@ -29,8 +29,6 @@ export interface CliOptions { files?: string[]; json: boolean; verbose: boolean; - fix: boolean; - write: boolean; help: boolean; list: boolean; // TCR-specific options @@ -110,12 +108,6 @@ const FLAG_HANDLERS: Record = { '-v': (opts) => { opts.verbose = true; }, - '--fix': (opts) => { - opts.fix = true; - }, - '--write': (opts) => { - opts.write = true; - }, '--list': (opts) => { opts.list = true; }, @@ -242,8 +234,6 @@ function createDefaultOptions(): CliOptions { files: [], json: false, verbose: false, - fix: false, - write: false, help: false, list: false, execute: false, diff --git a/packages/testing/janitor/src/cli/help.ts b/packages/testing/janitor/src/cli/help.ts index a30bf583975..7bf51e31edb 100644 --- a/packages/testing/janitor/src/cli/help.ts +++ b/packages/testing/janitor/src/cli/help.ts @@ -29,8 +29,6 @@ Analysis Options: --files= Analyze multiple files (comma-separated) --json Output as JSON --verbose, -v Detailed output with suggestions - --fix Preview fixes (dry run) - --fix --write Apply fixes to disk --list, -l List available rules --allow-in-expect Skip selector-purity violations inside expect() --ignore-baseline Show all violations, ignoring .janitor-baseline.json @@ -41,7 +39,6 @@ Examples: playwright-janitor --rule=dead-code # Run specific rule playwright-janitor inventory # Show codebase structure playwright-janitor impact --file=pages/X # Show what tests are affected - playwright-janitor --fix --write # Apply auto-fixes For command-specific help: playwright-janitor rules --help diff --git a/packages/testing/janitor/src/config.ts b/packages/testing/janitor/src/config.ts index d05b20a3e20..2ea7cc6c5c4 100644 --- a/packages/testing/janitor/src/config.ts +++ b/packages/testing/janitor/src/config.ts @@ -266,3 +266,9 @@ export function hasConfig(): boolean { export function resetConfig(): void { currentConfig = null; } + +/** Does `text` match any configured allow-pattern for the given rule? */ +export function ruleAllows(ruleId: string, text: string): boolean { + const allowPatterns = getConfig().rules?.[ruleId]?.allowPatterns ?? []; + return allowPatterns.some((pattern) => pattern.test(text)); +} diff --git a/packages/testing/janitor/src/core/reporter.ts b/packages/testing/janitor/src/core/reporter.ts index 714adb44b21..0db1bac846e 100644 --- a/packages/testing/janitor/src/core/reporter.ts +++ b/packages/testing/janitor/src/core/reporter.ts @@ -1,4 +1,4 @@ -import type { JanitorReport, Violation, FixResult } from '../types.js'; +import type { JanitorReport, Violation } from '../types.js'; import { getRelativePath } from './project-loader.js'; /** @@ -143,57 +143,3 @@ function getSeverityIcon(severity: string): string { return '[?]'; } } - -/** - * Output fix results to console - */ -export function printFixResults(report: JanitorReport, write: boolean): void { - const allFixes: FixResult[] = []; - for (const result of report.results) { - if (result.fixes) { - allFixes.push(...result.fixes); - } - } - - if (allFixes.length === 0) { - console.log('\nNo fixable violations found.\n'); - return; - } - - console.log('\n------------------------------------'); - console.log(write ? ' FIXES APPLIED' : ' FIX PREVIEW (dry run)'); - console.log('------------------------------------\n'); - - for (const fix of allFixes) { - const icon = fix.applied ? '[OK]' : '[PREVIEW]'; - const actionLabel = getActionLabel(fix.action); - const target = fix.target ? ` ${fix.target}` : ''; - console.log(` ${icon} ${actionLabel}${target}`); - console.log(` ${fix.file}`); - } - - console.log(''); - - if (!write) { - console.log(`${allFixes.length} fix(es) would be applied.`); - console.log('Use --fix --write to apply changes.\n'); - } else { - console.log(`${allFixes.length} fix(es) applied.`); - console.log('Run: pnpm typecheck && pnpm lint\n'); - } -} - -function getActionLabel(action: FixResult['action']): string { - switch (action) { - case 'remove-file': - return 'DELETE FILE'; - case 'remove-method': - return 'REMOVE METHOD'; - case 'remove-property': - return 'REMOVE PROPERTY'; - case 'edit': - return 'EDIT'; - default: - return action; - } -} diff --git a/packages/testing/janitor/src/core/rule-runner.ts b/packages/testing/janitor/src/core/rule-runner.ts index b101f0a3993..fe646dc3225 100644 --- a/packages/testing/janitor/src/core/rule-runner.ts +++ b/packages/testing/janitor/src/core/rule-runner.ts @@ -1,7 +1,5 @@ -import * as path from 'node:path'; -import type { Project, SourceFile } from 'ts-morph'; +import { performance } from 'node:perf_hooks'; -import type { BaseRule } from '../rules/base-rule.js'; import type { JanitorReport, RuleResult, @@ -9,270 +7,151 @@ import type { RunOptions, RuleConfig, RuleInfo, + Violation, } from '../types.js'; -import { getSourceFiles } from './project-loader.js'; -import { getConfig } from '../config.js'; + +/** Context passed to every rule. Rules self-build their scoped project from `rootDir`. */ +export interface JanitorRunContext { + rootDir: string; + /** Repo-relative changed files, when scoping a run (e.g. TCR). */ + changedFiles?: string[]; +} + +/** The rule surface the runner depends on — satisfied by `AstRule` subclasses + `getTargetGlobs`. */ +export interface RunnableRule { + readonly id: string; + readonly name: string; + readonly description: string; + readonly severity: Severity; + readonly fixable: boolean; + getTargetGlobs(): string[]; + analyze(context: JanitorRunContext): Violation[]; + configure(settings: { options?: Record }): void; +} export class RuleRunner { - private rules: Map = new Map(); + private rules: Map = new Map(); private enabledRules: Set = new Set(); - private ruleConfigs: Map = new Map(); - /** - * Register a rule with the runner - */ - registerRule(rule: BaseRule): void { + registerRule(rule: RunnableRule): void { this.rules.set(rule.id, rule); this.enabledRules.add(rule.id); } - /** - * Enable a specific rule - */ enableRule(ruleId: string): void { - if (this.rules.has(ruleId)) { - this.enabledRules.add(ruleId); - } + if (this.rules.has(ruleId)) this.enabledRules.add(ruleId); } - /** - * Disable a specific rule - */ disableRule(ruleId: string): void { this.enabledRules.delete(ruleId); } - /** - * Enable only specific rules (disable all others) - */ enableOnly(ruleIds: string[]): void { this.enabledRules.clear(); for (const id of ruleIds) { - if (this.rules.has(id)) { - this.enabledRules.add(id); - } + if (this.rules.has(id)) this.enabledRules.add(id); } } - /** - * Get all registered rule IDs - */ getRegisteredRules(): string[] { return Array.from(this.rules.keys()); } - /** - * Check if a rule is fixable - */ isRuleFixable(ruleId: string): boolean { return this.rules.get(ruleId)?.fixable ?? false; } - /** - * Get enabled rule IDs - */ getEnabledRules(): string[] { return Array.from(this.enabledRules); } - /** - * Get disabled rule IDs - */ getDisabledRules(): string[] { return Array.from(this.rules.keys()).filter((id) => !this.enabledRules.has(id)); } - /** - * Get detailed information about all registered rules - */ getRuleDetails(): RuleInfo[] { - return Array.from(this.rules.values()).map((rule) => ({ - id: rule.id, - name: rule.name, - description: rule.description, - severity: rule.severity, - fixable: rule.fixable, - enabled: this.enabledRules.has(rule.id), - targetGlobs: rule.getTargetGlobs(), - })); + return Array.from(this.rules.values()).map((rule) => this.toRuleInfo(rule)); } - /** - * Get detailed information about a specific rule - */ getRuleInfo(ruleId: string): RuleInfo | undefined { const rule = this.rules.get(ruleId); - if (!rule) return undefined; - - return { - id: rule.id, - name: rule.name, - description: rule.description, - severity: rule.severity, - fixable: rule.fixable, - enabled: this.enabledRules.has(rule.id), - targetGlobs: rule.getTargetGlobs(), - }; + return rule ? this.toRuleInfo(rule) : undefined; } - /** - * Configure a specific rule - */ configureRule(ruleId: string, config: RuleConfig): void { - this.ruleConfigs.set(ruleId, config); - const rule = this.rules.get(ruleId); - if (rule) { - rule.configure(config); - } + this.rules.get(ruleId)?.configure({ options: { ...config } }); } - /** - * Get files for a rule, optionally filtered by specific file paths - */ - private getFilesForRule(project: Project, rule: BaseRule, targetFiles?: string[]): SourceFile[] { - const root = getConfig().rootDir; - - if (targetFiles && targetFiles.length > 0) { - // Filter to only the specified files that match rule's globs - const ruleGlobs = rule.getTargetGlobs(); - const allRuleFiles = getSourceFiles(project, ruleGlobs); - const allRuleFilePaths = new Set(allRuleFiles.map((f) => f.getFilePath())); - - // Resolve target files to absolute paths and filter - return targetFiles - .map((f) => { - const absolutePath = path.isAbsolute(f) ? f : path.join(root, f); - return project.getSourceFile(absolutePath); - }) - .filter((f): f is SourceFile => f !== undefined && allRuleFilePaths.has(f.getFilePath())); - } - - return getSourceFiles(project, rule.getTargetGlobs()); - } - - /** - * Run all enabled rules and return results - */ - run(project: Project, projectRoot: string, options?: RunOptions): JanitorReport { - const results: RuleResult[] = []; - const allFilesAnalyzed = new Set(); - - // Apply rule configurations from options + run(context: JanitorRunContext, options?: RunOptions): JanitorReport { if (options?.ruleConfig) { for (const [ruleId, config] of Object.entries(options.ruleConfig)) { this.configureRule(ruleId, config); } } + const runContext: JanitorRunContext = { ...context, changedFiles: options?.files }; + const results: RuleResult[] = []; for (const ruleId of this.enabledRules) { const rule = this.rules.get(ruleId); - if (!rule) continue; - - // Get files for this rule (optionally filtered) - const files = this.getFilesForRule(project, rule, options?.files); - - // Track all files analyzed - for (const file of files) { - allFilesAnalyzed.add(file.getFilePath()); - } - - // Execute the rule - const result = rule.execute(project, files); - - // Apply fixes if in fix mode and rule supports it - if (options?.fix && rule.fixable) { - const fixableViolations = result.violations.filter((v) => v.fixable); - if (fixableViolations.length > 0) { - result.fixes = rule.fix(project, fixableViolations, options.write ?? false); - } - } - - results.push(result); + if (rule) results.push(this.execute(rule, runContext)); } - // Build summary - const summary = this.buildSummary(results, allFilesAnalyzed.size); - - return { - timestamp: new Date().toISOString(), - projectRoot, - rules: { - enabled: this.getEnabledRules(), - disabled: this.getDisabledRules(), - }, - results, - summary, - }; + return this.buildReport(context.rootDir, results); } - /** - * Run a specific rule by ID - */ - runRule( - project: Project, - projectRoot: string, - ruleId: string, - options?: RunOptions, - ): JanitorReport | null { + runRule(ruleId: string, context: JanitorRunContext, options?: RunOptions): JanitorReport | null { const rule = this.rules.get(ruleId); - if (!rule) { - return null; - } + if (!rule) return null; - // Apply rule configuration from options - if (options?.ruleConfig?.[ruleId]) { - rule.configure(options.ruleConfig[ruleId]); - } + if (options?.ruleConfig?.[ruleId]) this.configureRule(ruleId, options.ruleConfig[ruleId]); - const files = this.getFilesForRule(project, rule, options?.files); - const result = rule.execute(project, files); + const runContext: JanitorRunContext = { ...context, changedFiles: options?.files }; + return this.buildReport(context.rootDir, [this.execute(rule, runContext)]); + } - // Apply fixes if in fix mode and rule supports it - if (options?.fix && rule.fixable) { - const fixableViolations = result.violations.filter((v) => v.fixable); - if (fixableViolations.length > 0) { - result.fixes = rule.fix(project, fixableViolations, options.write ?? false); - } - } - - const summary = this.buildSummary([result], files.length); + private execute(rule: RunnableRule, context: JanitorRunContext): RuleResult { + const start = performance.now(); + const violations = rule.analyze(context); + const executionTimeMs = Math.round((performance.now() - start) * 100) / 100; return { - timestamp: new Date().toISOString(), - projectRoot, - rules: { - enabled: [ruleId], - disabled: this.getRegisteredRules().filter((id) => id !== ruleId), - }, - results: [result], - summary, + rule: rule.id, + violations, + filesAnalyzed: 0, + executionTimeMs, + fixable: rule.fixable, }; } - private buildSummary(results: RuleResult[], filesAnalyzed: number): JanitorReport['summary'] { - const byRule: Record = {}; - const bySeverity: Record = { - error: 0, - warning: 0, - info: 0, + private toRuleInfo(rule: RunnableRule): RuleInfo { + return { + id: rule.id, + name: rule.name, + description: rule.description, + severity: rule.severity, + fixable: rule.fixable, + enabled: this.enabledRules.has(rule.id), + targetGlobs: rule.getTargetGlobs(), }; + } + private buildReport(projectRoot: string, results: RuleResult[]): JanitorReport { + const byRule: Record = {}; + const bySeverity: Record = { error: 0, warning: 0, info: 0 }; let totalViolations = 0; for (const result of results) { byRule[result.rule] = result.violations.length; totalViolations += result.violations.length; - - for (const violation of result.violations) { - bySeverity[violation.severity]++; - } + for (const violation of result.violations) bySeverity[violation.severity]++; } return { - totalViolations, - byRule, - bySeverity, - filesAnalyzed, + timestamp: new Date().toISOString(), + projectRoot, + rules: { enabled: this.getEnabledRules(), disabled: this.getDisabledRules() }, + results, + summary: { totalViolations, byRule, bySeverity, filesAnalyzed: 0 }, }; } } diff --git a/packages/testing/janitor/src/core/tcr-executor.ts b/packages/testing/janitor/src/core/tcr-executor.ts index 5379fc0993f..dedda139d46 100644 --- a/packages/testing/janitor/src/core/tcr-executor.ts +++ b/packages/testing/janitor/src/core/tcr-executor.ts @@ -11,7 +11,6 @@ import { loadBaseline, filterNewViolations } from './baseline.js'; import { extractDiffs } from './extract-diffs.js'; import { ImpactAnalyzer } from './impact-analyzer.js'; import { MethodUsageAnalyzer, type MethodUsageIndex } from './method-usage-analyzer.js'; -import { createProject } from './project-loader.js'; import { RuleRunner } from './rule-runner.js'; import { ApiPurityRule } from '../rules/api-purity.rule.js'; import { BoundaryProtectionRule } from '../rules/boundary-protection.rule.js'; @@ -366,7 +365,6 @@ export class TcrExecutor { private runRules(changedFiles: string[]): number { const runner = this.createRuleRunner(); - const { project } = createProject(this.root); const tsFiles = changedFiles .filter((f) => f.endsWith('.ts')) @@ -374,7 +372,7 @@ export class TcrExecutor { if (tsFiles.length === 0) return 0; - const report = runner.run(project, this.root, { files: tsFiles }); + const report = runner.run({ rootDir: this.root }, { files: tsFiles }); return this.countNewViolations(report); } diff --git a/packages/testing/janitor/src/index.ts b/packages/testing/janitor/src/index.ts index fec945481a9..380da3d5785 100644 --- a/packages/testing/janitor/src/index.ts +++ b/packages/testing/janitor/src/index.ts @@ -45,12 +45,9 @@ export type { PropertyFixData, ClassFixData, EditFixData, - FixAction, - FixResult, RuleResult, ReportSummary, JanitorReport, - Rule, RuleInfo, FilePatterns, FacadeConfig, @@ -71,6 +68,7 @@ export type { export { SyntaxKind } from './types.js'; export { RuleRunner } from './core/rule-runner.js'; +export type { RunnableRule, JanitorRunContext } from './core/rule-runner.js'; export { FacadeResolver, extractTypeName, type FacadeMapping } from './core/facade-resolver.js'; export { createProject, @@ -79,10 +77,9 @@ export { getRelativePath, type ProjectContext, } from './core/project-loader.js'; -export { toJSON, toConsole, printFixResults } from './core/reporter.js'; +export { toJSON, toConsole } from './core/reporter.js'; export { - BaseRule, BoundaryProtectionRule, ScopeLockdownRule, SelectorPurityRule, @@ -201,7 +198,6 @@ export { } from './core/baseline.js'; import { setConfig, type JanitorConfig } from './config.js'; -import { createProject } from './core/project-loader.js'; import { RuleRunner } from './core/rule-runner.js'; import { ApiPurityRule } from './rules/api-purity.rule.js'; import { BoundaryProtectionRule } from './rules/boundary-protection.rule.js'; @@ -232,7 +228,6 @@ export function createDefaultRunner(): RuleRunner { export function runAnalysis(config: JanitorConfig, options?: RunOptions): JanitorReport { setConfig(config); - const { project, root } = createProject(config.rootDir); const runner = createDefaultRunner(); - return runner.run(project, root, options); + return runner.run({ rootDir: config.rootDir }, options); } diff --git a/packages/testing/janitor/src/rules/api-purity.rule.test.ts b/packages/testing/janitor/src/rules/api-purity.rule.test.ts index d3f0a038e60..c92af175a8c 100644 --- a/packages/testing/janitor/src/rules/api-purity.rule.test.ts +++ b/packages/testing/janitor/src/rules/api-purity.rule.test.ts @@ -19,7 +19,7 @@ test('creates workflow', async ({ n8n, api }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -36,7 +36,7 @@ test('gets workflow', async ({ request }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('request.get'); @@ -54,7 +54,7 @@ test('creates workflow', async ({ request }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].suggestion).toContain('api'); @@ -72,7 +72,7 @@ test('fetches data', async () => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('fetch'); @@ -92,7 +92,7 @@ test('multiple API calls', async ({ request }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(3); }); @@ -109,7 +109,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); }); @@ -127,7 +127,7 @@ test('test', async ({ request }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].line).toBe(6); diff --git a/packages/testing/janitor/src/rules/api-purity.rule.ts b/packages/testing/janitor/src/rules/api-purity.rule.ts index 674209fb954..d94c6407e81 100644 --- a/packages/testing/janitor/src/rules/api-purity.rule.ts +++ b/packages/testing/janitor/src/rules/api-purity.rule.ts @@ -1,7 +1,8 @@ +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import type { Project, SourceFile } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; -import { getConfig } from '../config.js'; +import { getConfig, ruleAllows } from '../config.js'; import type { Violation } from '../types.js'; import { truncateText } from '../utils/ast-helpers.js'; @@ -28,7 +29,7 @@ import { truncateText } from '../utils/ast-helpers.js'; * - API service files themselves (services/**) can make raw calls * - Fixture setup files can make raw calls */ -export class ApiPurityRule extends BaseRule { +export class ApiPurityRule extends AstRule<{ rootDir: string }> { readonly id = 'api-purity'; readonly name = 'API Purity'; readonly description = 'Tests and composables should use API services, not raw HTTP calls'; @@ -39,7 +40,15 @@ export class ApiPurityRule extends BaseRule { return [...config.patterns.tests, ...config.patterns.flows]; } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, files: SourceFile[] = project.getSourceFiles()): Violation[] { const violations: Violation[] = []; const config = getConfig(); @@ -62,7 +71,7 @@ export class ApiPurityRule extends BaseRule { const context = content.substring(contextStart, contextEnd); // Check if this matches any allow patterns - if (this.isAllowed(context)) { + if (ruleAllows(this.id, context)) { continue; } @@ -77,7 +86,7 @@ export class ApiPurityRule extends BaseRule { const suggestion = this.getSuggestion(matchText); violations.push( - this.createViolation( + this.fileViolation( file, line, column, diff --git a/packages/testing/janitor/src/rules/base-rule.ts b/packages/testing/janitor/src/rules/base-rule.ts deleted file mode 100644 index 1ccb4af43a8..00000000000 --- a/packages/testing/janitor/src/rules/base-rule.ts +++ /dev/null @@ -1,127 +0,0 @@ -import type { Project, SourceFile } from 'ts-morph'; - -import { getConfig } from '../config.js'; -import type { Severity, Violation, RuleResult, RuleConfig, FixResult, FixData } from '../types.js'; - -export abstract class BaseRule { - abstract readonly id: string; - abstract readonly name: string; - abstract readonly description: string; - abstract readonly severity: Severity; - - /** Whether this rule supports auto-fixing */ - readonly fixable: boolean = false; - - /** Rule-specific configuration */ - protected config: RuleConfig = {}; - - /** - * Get the effective severity for this rule (may be overridden in config) - */ - getEffectiveSeverity(): Severity { - const ruleConfig = getConfig().rules?.[this.id]; - if (ruleConfig?.severity === 'off') { - return 'info'; // Will be filtered out - } - if (ruleConfig?.severity) { - return ruleConfig.severity; - } - return this.severity; - } - - /** - * Check if this rule is enabled - */ - isEnabled(): boolean { - const ruleConfig = getConfig().rules?.[this.id]; - if (ruleConfig?.enabled === false) return false; - if (ruleConfig?.severity === 'off') return false; - return true; - } - - /** - * Check if a text matches any of the allow patterns for this rule - */ - protected isAllowed(text: string): boolean { - const ruleConfig = getConfig().rules?.[this.id]; - const allowPatterns = ruleConfig?.allowPatterns ?? []; - return allowPatterns.some((pattern) => pattern.test(text)); - } - - /** - * Define which file patterns this rule should analyze - * @returns Array of glob patterns relative to root - */ - abstract getTargetGlobs(): string[]; - - /** - * Analyze files and return violations - * @param project - ts-morph Project instance - * @param files - Source files matching the target globs - * @returns Array of violations found - */ - abstract analyze(project: Project, files: SourceFile[]): Violation[]; - - /** - * Apply fixes for violations. Override in fixable rules. - * @param project - ts-morph Project instance - * @param violations - Violations to fix - * @param write - Whether to write changes to disk - * @returns Array of fix results - */ - fix(_project: Project, _violations: Violation[], _write: boolean): FixResult[] { - return []; - } - - /** - * Configure the rule with options - */ - configure(config: RuleConfig): void { - this.config = { ...this.config, ...config }; - } - - /** - * Execute the rule with timing instrumentation - * @param project - ts-morph Project instance - * @param files - Source files matching the target globs - * @returns Rule result with violations and metadata - */ - execute(project: Project, files: SourceFile[]): RuleResult { - const startTime = performance.now(); - const violations = this.analyze(project, files); - const endTime = performance.now(); - - return { - rule: this.id, - violations, - filesAnalyzed: files.length, - executionTimeMs: Math.round((endTime - startTime) * 100) / 100, - fixable: this.fixable, - }; - } - - /** - * Helper to create a violation with consistent structure - */ - protected createViolation( - file: SourceFile, - line: number, - column: number, - message: string, - suggestion?: string, - fixable?: boolean, - fixData?: FixData, - ): Violation { - return { - file: file.getFilePath(), - line, - column, - rule: this.id, - message, - severity: this.getEffectiveSeverity(), - suggestion, - fixable, - fixData, - }; - } -} diff --git a/packages/testing/janitor/src/rules/boundary-protection.rule.test.ts b/packages/testing/janitor/src/rules/boundary-protection.rule.test.ts index b68986cb2fa..c741f6c28fe 100644 --- a/packages/testing/janitor/src/rules/boundary-protection.rule.test.ts +++ b/packages/testing/janitor/src/rules/boundary-protection.rule.test.ts @@ -3,95 +3,36 @@ import { describe } from 'vitest'; import { BoundaryProtectionRule } from './boundary-protection.rule.js'; import { test, expect } from '../test/fixtures.js'; +// Uses janitor's test fixture so getConfig() (excludeFromPages, patterns) is set — +// the rule respects configured exclusions via isExcludedPage. describe('BoundaryProtectionRule', () => { const rule = new BoundaryProtectionRule(); - test('should allow imports from components', ({ project, createFile }) => { - const file = createFile( - '/pages/CanvasPage.ts', - ` -import { NodePanel } from './components/NodePanel'; - -export class CanvasPage { - readonly nodePanel = new NodePanel(); -} -`, - ); - - const violations = rule.analyze(project, [file]); - expect(violations).toHaveLength(0); + test('allows imports from components', ({ project, createFile }) => { + createFile('/pages/CanvasPage.ts', "import { NodePanel } from './components/NodePanel';"); + expect(rule.analyzeProject(project)).toHaveLength(0); }); - test('should flag direct page imports', ({ project, createFile }) => { - const file = createFile( - '/pages/CanvasPage.ts', - ` -import { WorkflowPage } from './WorkflowPage'; - -export class CanvasPage { - readonly workflows = new WorkflowPage(); -} -`, - ); - - // Also create the imported file so ts-morph can resolve it - createFile( - '/pages/WorkflowPage.ts', - ` -export class WorkflowPage {} -`, - ); - - const violations = rule.analyze(project, [file]); + test('flags a direct page import', ({ project, createFile }) => { + createFile('/pages/CanvasPage.ts', "import { WorkflowPage } from './WorkflowPage';"); + createFile('/pages/WorkflowPage.ts', 'export class WorkflowPage {}'); + const violations = rule.analyzeProject(project); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('imports another page'); - expect(violations[0].rule).toBe('boundary-protection'); }); - test('should allow imports from base classes', ({ project, createFile }) => { - const file = createFile( - '/pages/CanvasPage.ts', - ` -import { BasePage } from './BasePage'; - -export class CanvasPage extends BasePage { -} -`, - ); - - const violations = rule.analyze(project, [file]); - expect(violations).toHaveLength(0); + test('allows imports from base classes', ({ project, createFile }) => { + createFile('/pages/CanvasPage.ts', "import { BasePage } from './BasePage';"); + expect(rule.analyzeProject(project)).toHaveLength(0); }); - test('should skip excluded files', ({ project, createFile }) => { - const file = createFile( - '/pages/BasePage.ts', - ` -import { SomePage } from './SomePage'; - -export class BasePage { -} -`, - ); - - const violations = rule.analyze(project, [file]); - expect(violations).toHaveLength(0); + test('skips files excluded via config (excludeFromPages)', ({ project, createFile }) => { + createFile('/pages/BasePage.ts', "import { SomePage } from './SomePage';"); + expect(rule.analyzeProject(project)).toHaveLength(0); }); - test('should allow external package imports', ({ project, createFile }) => { - const file = createFile( - '/pages/CanvasPage.ts', - ` -import { Page } from '@playwright/test'; -import { expect } from '@playwright/test'; - -export class CanvasPage { - constructor(private page: Page) {} -} -`, - ); - - const violations = rule.analyze(project, [file]); - expect(violations).toHaveLength(0); + test('allows external package imports', ({ project, createFile }) => { + createFile('/pages/CanvasPage.ts', "import { Page } from '@playwright/test';"); + expect(rule.analyzeProject(project)).toHaveLength(0); }); }); diff --git a/packages/testing/janitor/src/rules/boundary-protection.rule.ts b/packages/testing/janitor/src/rules/boundary-protection.rule.ts index dce001956e1..a1bdbcda2b2 100644 --- a/packages/testing/janitor/src/rules/boundary-protection.rule.ts +++ b/packages/testing/janitor/src/rules/boundary-protection.rule.ts @@ -1,73 +1,53 @@ -import type { Project, SourceFile } from 'ts-morph'; +import type { Violation } from '@n8n/rules-engine'; +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; +import type { Project } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; import { getConfig } from '../config.js'; -import type { Violation } from '../types.js'; -import { getImportPaths, isPageImport } from '../utils/ast-helpers.js'; +import { isPageImport } from '../utils/ast-helpers.js'; import { isExcludedPage } from '../utils/paths.js'; /** - * Boundary Protection Rule - * - * Pages should not import other pages directly. - * This prevents tight coupling between page objects. - * - * Allowed: - * - Imports from components directory - * - Imports from base classes (BasePage, BaseModal, etc.) - * - Imports from external packages - * - * Violations: - * - import { CanvasPage } from './CanvasPage' (in a page file) - * - import { WorkflowPage } from '../WorkflowPage' + * Pages must not import other pages directly. Janitor-owned (playwright domain + * logic via {@link isPageImport} + config-driven {@link isExcludedPage}) but + * built on the shared `@n8n/rules-engine/ast` substrate rather than a janitor base. */ -export class BoundaryProtectionRule extends BaseRule { +export class BoundaryProtectionRule extends AstRule<{ rootDir: string }> { readonly id = 'boundary-protection'; readonly name = 'Boundary Protection'; - readonly description = 'Pages should not import other pages directly'; + readonly description = 'Page objects should not import other page objects directly'; readonly severity = 'error' as const; getTargetGlobs(): string[] { - const config = getConfig(); - // Only analyze page files (excluding components) - return config.patterns.pages.filter((p) => !p.includes('components')); + return getConfig().patterns.pages.filter((p) => !p.includes('components')); } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project): Violation[] { const violations: Violation[] = []; - for (const file of files) { + for (const file of project.getSourceFiles()) { const filePath = file.getFilePath(); + if (isExcludedPage(filePath)) continue; + if (filePath.includes('/components/')) continue; - // Skip excluded files (facades, base classes) - if (isExcludedPage(filePath)) { - continue; - } - - // Skip if file is in components directory - if (filePath.includes('/components/')) { - continue; - } - - const importPaths = getImportPaths(file); - - for (const importPath of importPaths) { - if (isPageImport(importPath, filePath)) { - const importDecl = file - .getImportDeclarations() - .find((d) => d.getModuleSpecifierValue() === importPath); - - if (importDecl) { - violations.push( - this.createViolation( - file, - importDecl.getStartLineNumber(), - 0, - `Page imports another page: ${importPath}`, - 'Use composition through composables or inject via constructor instead', - ), - ); - } + for (const decl of file.getImportDeclarations()) { + const spec = decl.getModuleSpecifierValue(); + if (isPageImport(spec, filePath)) { + violations.push( + this.nodeViolation( + decl, + `Page imports another page: ${spec}`, + 'Use composition through composables or inject via constructor instead', + ), + ); } } } diff --git a/packages/testing/janitor/src/rules/dead-code.rule.test.ts b/packages/testing/janitor/src/rules/dead-code.rule.test.ts index e5d2f6808f7..f94151591b8 100644 --- a/packages/testing/janitor/src/rules/dead-code.rule.test.ts +++ b/packages/testing/janitor/src/rules/dead-code.rule.test.ts @@ -30,7 +30,7 @@ page.clickButton(); ); const pageFile = project.getSourceFile('/pages/TestPage.ts')!; - const violations = rule.analyze(project, [pageFile]); + const violations = rule.analyzeProject(project, [pageFile]); expect(violations).toHaveLength(0); }); @@ -55,7 +55,7 @@ page.usedMethod(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('unusedMethod'); @@ -82,7 +82,7 @@ console.log(page.usedProp); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('unusedProp'); @@ -98,7 +98,7 @@ export class DeadPage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('Dead class'); @@ -127,7 +127,7 @@ page.publicMethod(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -152,7 +152,7 @@ page.publicMethod(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -189,7 +189,7 @@ const h = new MyHelper(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); // archive() should NOT be flagged — text fallback finds '.archive(' // reallyUnused() SHOULD be flagged — no text match anywhere @@ -225,7 +225,7 @@ const p = new TestPage(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('deadProp'); @@ -251,7 +251,7 @@ thing.saveAll(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('save'); @@ -279,7 +279,7 @@ const arr = [obj.usedInArray, other]; `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -306,7 +306,7 @@ thing.$reset(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -329,7 +329,7 @@ const page = new TestPage(); `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); const fixData = violations[0].fixData; diff --git a/packages/testing/janitor/src/rules/dead-code.rule.ts b/packages/testing/janitor/src/rules/dead-code.rule.ts index c917c6d49f7..c5bce1a606f 100644 --- a/packages/testing/janitor/src/rules/dead-code.rule.ts +++ b/packages/testing/janitor/src/rules/dead-code.rule.ts @@ -1,32 +1,27 @@ -import * as fs from 'node:fs'; +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import { SyntaxKind, type Project, type SourceFile } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; import { getConfig } from '../config.js'; -import { isMethodFix, isPropertyFix } from '../types.js'; -import type { Violation, FixResult } from '../types.js'; -import { getRelativePath } from '../utils/paths.js'; +import type { Violation } from '../types.js'; +import { matchesPatterns } from '../utils/paths.js'; /** * Dead Code Rule * - * Finds and optionally removes unused methods, properties, and classes. - * Uses reference tracing to detect code that isn't used anywhere in the codebase. + * Finds unused methods, properties, and classes via reference tracing. * * Detects: * - Unused public methods (no external references) * - Unused public properties (no external references) * - Dead classes (entire class not referenced externally) * - Empty classes (no public members) - * - * Supports auto-fixing with --fix --write */ -export class DeadCodeRule extends BaseRule { +export class DeadCodeRule extends AstRule<{ rootDir: string }> { readonly id = 'dead-code'; readonly name = 'Dead Code'; - readonly description = 'Find and remove unused methods, properties, and classes'; + readonly description = 'Find unused methods, properties, and classes'; readonly severity = 'warning' as const; - readonly fixable = true; getTargetGlobs(): string[] { const config = getConfig(); @@ -38,7 +33,25 @@ export class DeadCodeRule extends BaseRule { ]; } - analyze(project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + // Reference resolution needs the whole suite loaded (a method used only by a + // test must be seen as referenced) — so load broadly and filter to targets below. + return { + packages: ['.'], + spec: { globs: ['**/*.ts', '!**/*.d.ts', '!node_modules/**'], resolveDependencies: true }, + }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject( + project: Project, + files: SourceFile[] = project + .getSourceFiles() + .filter((f) => matchesPatterns(f.getFilePath(), this.getTargetGlobs())), + ): Violation[] { const violations: Violation[] = []; for (const file of files) { @@ -75,7 +88,7 @@ export class DeadCodeRule extends BaseRule { classDecl: ReturnType[0], className: string, ): Violation { - return this.createViolation( + return this.fileViolation( file, classDecl.getStartLineNumber(), 0, @@ -103,7 +116,7 @@ export class DeadCodeRule extends BaseRule { !this.hasTextUsage(project, file, methodName) ) { violations.push( - this.createViolation( + this.fileViolation( file, method.getStartLineNumber(), 0, @@ -133,7 +146,7 @@ export class DeadCodeRule extends BaseRule { const propName = prop.getName(); if (!this.hasExternalReferences(file, prop) && !this.hasTextUsage(project, file, propName)) { violations.push( - this.createViolation( + this.fileViolation( file, prop.getStartLineNumber(), 0, @@ -189,130 +202,4 @@ export class DeadCodeRule extends BaseRule { ref.getStartLineNumber() !== node.getStartLineNumber(), ); } - - fix(project: Project, violations: Violation[], write: boolean): FixResult[] { - const results: FixResult[] = []; - const byFile = this.groupViolationsByFile(violations); - const filesToDelete = new Set(); - - for (const [filePath, fileViolations] of byFile) { - const sourceFile = project.getSourceFile(filePath); - if (!sourceFile) continue; - - // Check if entire file should be deleted - if (this.shouldDeleteFile(sourceFile, fileViolations)) { - filesToDelete.add(filePath); - results.push(this.createFixResult(filePath, 'remove-file', write)); - continue; - } - - // Process individual member removals - results.push(...this.processFileViolations(sourceFile, filePath, fileViolations, write)); - } - - // Apply changes - if (write) { - project.saveSync(); - this.deleteFiles(filesToDelete); - } - - return results; - } - - private groupViolationsByFile(violations: Violation[]): Map { - const byFile = new Map(); - for (const v of violations) { - if (!v.fixable || !v.fixData) continue; - const existing = byFile.get(v.file) ?? []; - existing.push(v); - byFile.set(v.file, existing); - } - return byFile; - } - - private shouldDeleteFile(sourceFile: SourceFile, fileViolations: Violation[]): boolean { - const deadClassViolations = fileViolations.filter((v) => v.fixData?.type === 'class'); - const allClassesInFile = sourceFile.getClasses(); - return deadClassViolations.length === allClassesInFile.length && allClassesInFile.length > 0; - } - - private createFixResult( - filePath: string, - action: 'remove-file' | 'remove-method' | 'remove-property', - write: boolean, - target?: string, - ): FixResult { - return { - file: getRelativePath(filePath), - action, - target, - applied: write, - }; - } - - private processFileViolations( - sourceFile: SourceFile, - filePath: string, - fileViolations: Violation[], - write: boolean, - ): FixResult[] { - const results: FixResult[] = []; - - for (const violation of fileViolations) { - const fixData = violation.fixData; - if (!fixData) continue; - if (!isMethodFix(fixData) && !isPropertyFix(fixData)) continue; - - const classDecl = sourceFile.getClass(fixData.className); - if (!classDecl) continue; - - const result = this.removeMember(classDecl, filePath, fixData, write); - if (result) results.push(result); - } - - return results; - } - - private removeMember( - classDecl: ReturnType, - filePath: string, - fixData: { type: 'method' | 'property'; className: string; memberName: string }, - write: boolean, - ): FixResult | null { - if (!classDecl) return null; - - if (fixData.type === 'method') { - const method = classDecl.getMethod(fixData.memberName); - if (method) { - if (write) method.remove(); - return this.createFixResult( - filePath, - 'remove-method', - write, - `${fixData.className}.${fixData.memberName}()`, - ); - } - } else if (fixData.type === 'property') { - const prop = classDecl.getProperty(fixData.memberName); - if (prop) { - if (write) prop.remove(); - return this.createFixResult( - filePath, - 'remove-property', - write, - `${fixData.className}.${fixData.memberName}`, - ); - } - } - - return null; - } - - private deleteFiles(filesToDelete: Set): void { - for (const filePath of filesToDelete) { - if (fs.existsSync(filePath)) { - fs.unlinkSync(filePath); - } - } - } } diff --git a/packages/testing/janitor/src/rules/deduplication.rule.test.ts b/packages/testing/janitor/src/rules/deduplication.rule.test.ts index ca58041427d..c1f757a4a11 100644 --- a/packages/testing/janitor/src/rules/deduplication.rule.test.ts +++ b/packages/testing/janitor/src/rules/deduplication.rule.test.ts @@ -28,7 +28,7 @@ export class PageB { `, ); - const violations = rule.analyze(project, [file1, file2]); + const violations = rule.analyzeProject(project, [file1, file2]); expect(violations.length).toBeGreaterThanOrEqual(1); expect(violations[0].message).toContain('my-button'); @@ -56,7 +56,7 @@ export class PageB { `, ); - const violations = rule.analyze(project, [file1, file2]); + const violations = rule.analyzeProject(project, [file1, file2]); expect(violations).toHaveLength(0); }); @@ -77,7 +77,7 @@ export class PageA { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -104,7 +104,7 @@ export class ComponentA { `, ); - const violations = rule.analyze(project, [pageFile, componentFile]); + const violations = rule.analyzeProject(project, [pageFile, componentFile]); expect(violations).toHaveLength(0); }); @@ -131,7 +131,7 @@ export class PageB { `, ); - const violations = rule.analyze(project, [file1, file2]); + const violations = rule.analyzeProject(project, [file1, file2]); expect(violations).toHaveLength(0); }); diff --git a/packages/testing/janitor/src/rules/deduplication.rule.ts b/packages/testing/janitor/src/rules/deduplication.rule.ts index 1e0f3d289bc..3ad86b4db21 100644 --- a/packages/testing/janitor/src/rules/deduplication.rule.ts +++ b/packages/testing/janitor/src/rules/deduplication.rule.ts @@ -1,6 +1,7 @@ +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import { SyntaxKind, type Project, type SourceFile, type CallExpression } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; import { getConfig } from '../config.js'; import type { Violation } from '../types.js'; import { isComponentFile, getRelativePath } from '../utils/paths.js'; @@ -30,7 +31,7 @@ interface TestIdUsage { * * Only flags when the same {root}:{testId} combination appears in multiple files. */ -export class DeduplicationRule extends BaseRule { +export class DeduplicationRule extends AstRule<{ rootDir: string }> { readonly id = 'deduplication'; readonly name = 'Deduplication'; readonly description = 'Test IDs should be unique within their scope'; @@ -41,7 +42,15 @@ export class DeduplicationRule extends BaseRule { return config.patterns.pages; } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, files: SourceFile[] = project.getSourceFiles()): Violation[] { const { pagesScope, componentsScope } = this.buildSelectorMaps(files); return [ @@ -139,7 +148,7 @@ export class DeduplicationRule extends BaseRule { if (!file) continue; violations.push( - this.createViolation( + this.fileViolation( file, usage.line, usage.column, diff --git a/packages/testing/janitor/src/rules/duplicate-logic.rule.test.ts b/packages/testing/janitor/src/rules/duplicate-logic.rule.test.ts index 6a686398ab1..8b809d3efa0 100644 --- a/packages/testing/janitor/src/rules/duplicate-logic.rule.test.ts +++ b/packages/testing/janitor/src/rules/duplicate-logic.rule.test.ts @@ -58,7 +58,7 @@ describe('DuplicateLogicRule', () => { }); const files = project.getSourceFiles(); - const violations = rule.analyze(project, files); + const violations = rule.analyzeProject(project, files); expect(violations.length).toBe(1); expect(violations[0].message).toContain('Duplicate method logic'); @@ -91,7 +91,7 @@ describe('DuplicateLogicRule', () => { }); const files = project.getSourceFiles(); - const violations = rule.analyze(project, files); + const violations = rule.analyzeProject(project, files); expect(violations.length).toBe(0); }); @@ -116,7 +116,7 @@ describe('DuplicateLogicRule', () => { }); const files = project.getSourceFiles(); - const violations = rule.analyze(project, files); + const violations = rule.analyzeProject(project, files); // Same file duplicates are allowed (may be intentional) expect(violations.length).toBe(0); @@ -144,7 +144,7 @@ describe('DuplicateLogicRule', () => { }); const files = project.getSourceFiles(); - const violations = rule.analyze(project, files); + const violations = rule.analyzeProject(project, files); expect(violations.length).toBe(1); expect(violations[0].message).toContain('Duplicate test logic'); @@ -175,7 +175,7 @@ describe('DuplicateLogicRule', () => { }); const files = project.getSourceFiles(); - const violations = rule.analyze(project, files); + const violations = rule.analyzeProject(project, files); // Should detect that test duplicates HomePage.login() expect(violations.length).toBe(1); @@ -204,7 +204,7 @@ describe('DuplicateLogicRule', () => { }); const files = project.getSourceFiles(); - const violations = rule.analyze(project, files); + const violations = rule.analyzeProject(project, files); // Only 1 statement, below threshold of 2 expect(violations.length).toBe(0); @@ -231,7 +231,7 @@ describe('DuplicateLogicRule', () => { }); const files = project.getSourceFiles(); - const violations = rule.analyze(project, files); + const violations = rule.analyzeProject(project, files); // 2 statements meets the threshold expect(violations.length).toBe(1); diff --git a/packages/testing/janitor/src/rules/duplicate-logic.rule.ts b/packages/testing/janitor/src/rules/duplicate-logic.rule.ts index 7652581773d..e15c61525f3 100644 --- a/packages/testing/janitor/src/rules/duplicate-logic.rule.ts +++ b/packages/testing/janitor/src/rules/duplicate-logic.rule.ts @@ -1,3 +1,5 @@ +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import { SyntaxKind, type Project, @@ -6,7 +8,6 @@ import { type MethodDeclaration, } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; import { getConfig } from '../config.js'; import type { Violation } from '../types.js'; import { getRelativePath, matchesPatterns } from '../utils/paths.js'; @@ -35,7 +36,7 @@ interface TestFingerprint { * Detects duplicate code using AST structural fingerprinting. * Normalizes code to structural patterns, ignoring variable names and literals. */ -export class DuplicateLogicRule extends BaseRule { +export class DuplicateLogicRule extends AstRule<{ rootDir: string }> { readonly id = 'duplicate-logic'; readonly name = 'Duplicate Logic'; readonly description = 'Detects duplicate code patterns across tests and page objects'; @@ -54,7 +55,15 @@ export class DuplicateLogicRule extends BaseRule { ]; } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, files: SourceFile[] = project.getSourceFiles()): Violation[] { const config = getConfig(); // Separate files by type @@ -138,7 +147,7 @@ export class DuplicateLogicRule extends BaseRule { if (!file) continue; violations.push( - this.createViolation( + this.fileViolation( file, method.line, 0, @@ -173,7 +182,7 @@ export class DuplicateLogicRule extends BaseRule { if (!file) continue; violations.push( - this.createViolation( + this.fileViolation( file, test.line, 0, @@ -218,7 +227,7 @@ export class DuplicateLogicRule extends BaseRule { if (!file) continue; violations.push( - this.createViolation( + this.fileViolation( file, test.line, 0, diff --git a/packages/testing/janitor/src/rules/index.ts b/packages/testing/janitor/src/rules/index.ts index 301ec4f42b7..f462a8bd00f 100644 --- a/packages/testing/janitor/src/rules/index.ts +++ b/packages/testing/janitor/src/rules/index.ts @@ -1,4 +1,3 @@ -export { BaseRule } from './base-rule.js'; export { BoundaryProtectionRule } from './boundary-protection.rule.js'; export { ScopeLockdownRule } from './scope-lockdown.rule.js'; export { SelectorPurityRule } from './selector-purity.rule.js'; @@ -11,4 +10,4 @@ export { DuplicateLogicRule } from './duplicate-logic.rule.js'; export { NoDirectPageInstantiationRule } from './no-direct-page-instantiation.rule.js'; // Re-export types for convenience -export type { Violation, FixResult, RuleResult, RuleConfig } from '../types.js'; +export type { Violation, RuleResult, RuleConfig } from '../types.js'; diff --git a/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.test.ts b/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.test.ts index 63d01a170f5..4dacc8a1bc1 100644 --- a/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.test.ts +++ b/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.test.ts @@ -19,7 +19,7 @@ test('my test', async ({ page }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('Direct page instantiation'); @@ -40,7 +40,7 @@ test('my test', async ({ page }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(2); }); @@ -55,7 +55,7 @@ test('my test', async ({ page }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].suggestion).toContain('n8n.canvas'); @@ -74,7 +74,7 @@ test('my test', async () => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -101,7 +101,7 @@ test('my test', async ({ page }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(2); expect(violations[0].suggestion).toContain('n8n.settingsPersonal'); @@ -119,7 +119,7 @@ test('my test', async () => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -139,7 +139,7 @@ test('my test', async ({ page }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('CanvasPage'); diff --git a/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.ts b/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.ts index fe0b5dbf157..66eb739d2e4 100644 --- a/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.ts +++ b/packages/testing/janitor/src/rules/no-direct-page-instantiation.rule.ts @@ -23,14 +23,16 @@ * - Page instantiation in page object files (for composition) * - Page instantiation in composables (if they need to create pages) */ + +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import { SyntaxKind, type Project, type SourceFile } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; -import { getConfig } from '../config.js'; +import { getConfig, ruleAllows } from '../config.js'; import type { Violation } from '../types.js'; import { truncateText } from '../utils/ast-helpers.js'; -export class NoDirectPageInstantiationRule extends BaseRule { +export class NoDirectPageInstantiationRule extends AstRule<{ rootDir: string }> { readonly id = 'no-direct-page-instantiation'; readonly name = 'No Direct Page Instantiation'; readonly description = @@ -42,7 +44,15 @@ export class NoDirectPageInstantiationRule extends BaseRule { return config.patterns.tests; } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, files: SourceFile[] = project.getSourceFiles()): Violation[] { const violations: Violation[] = []; for (const file of files) { @@ -53,7 +63,7 @@ export class NoDirectPageInstantiationRule extends BaseRule { const className = expr.getText(); if (this.isPageInstantiation(className)) { - if (this.isAllowed(className)) { + if (ruleAllows(this.id, className)) { continue; } @@ -62,7 +72,7 @@ export class NoDirectPageInstantiationRule extends BaseRule { const truncated = truncateText(newExpr.getText(), 60); violations.push( - this.createViolation( + this.fileViolation( file, startLine, startColumn, diff --git a/packages/testing/janitor/src/rules/no-page-in-flow.rule.test.ts b/packages/testing/janitor/src/rules/no-page-in-flow.rule.test.ts index b5fbfcfdb65..e84120d7a53 100644 --- a/packages/testing/janitor/src/rules/no-page-in-flow.rule.test.ts +++ b/packages/testing/janitor/src/rules/no-page-in-flow.rule.test.ts @@ -21,7 +21,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -40,7 +40,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].message).toContain('page.getByTestId'); @@ -60,7 +60,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); }); @@ -79,7 +79,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].suggestion).toContain('navigation'); @@ -101,7 +101,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(3); }); @@ -120,7 +120,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].line).toBe(6); @@ -140,7 +140,7 @@ export class WorkflowComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(1); expect(violations[0].suggestion).toBeDefined(); diff --git a/packages/testing/janitor/src/rules/no-page-in-flow.rule.ts b/packages/testing/janitor/src/rules/no-page-in-flow.rule.ts index 51db6dda937..61001b7877f 100644 --- a/packages/testing/janitor/src/rules/no-page-in-flow.rule.ts +++ b/packages/testing/janitor/src/rules/no-page-in-flow.rule.ts @@ -1,7 +1,8 @@ +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import { SyntaxKind, type Project, type SourceFile } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; -import { getConfig } from '../config.js'; +import { getConfig, ruleAllows } from '../config.js'; import type { Violation } from '../types.js'; import { LOCATOR_METHODS } from '../utils/ast-helpers.js'; @@ -28,7 +29,7 @@ import { LOCATOR_METHODS } from '../utils/ast-helpers.js'; * This is stricter than selector-purity, which allows page-level methods * like keyboard, evaluate, etc. in both composables and tests. */ -export class NoPageInFlowRule extends BaseRule { +export class NoPageInFlowRule extends AstRule<{ rootDir: string }> { readonly id = 'no-page-in-flow'; readonly name = 'No Page In Flow'; readonly severity = 'warning' as const; @@ -42,7 +43,15 @@ export class NoPageInFlowRule extends BaseRule { return getConfig().patterns.flows; } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, files: SourceFile[] = project.getSourceFiles()): Violation[] { const violations: Violation[] = []; const config = getConfig(); const fixtureName = config.fixtureObjectName; @@ -61,7 +70,7 @@ export class NoPageInFlowRule extends BaseRule { if (match) { // Check if this matches any allow patterns - if (this.isAllowed(text)) { + if (ruleAllows(this.id, text)) { continue; } @@ -77,7 +86,7 @@ export class NoPageInFlowRule extends BaseRule { const methodName = match[1]; violations.push( - this.createViolation( + this.fileViolation( file, startLine, startColumn, diff --git a/packages/testing/janitor/src/rules/scope-lockdown.rule.test.ts b/packages/testing/janitor/src/rules/scope-lockdown.rule.test.ts index 6276327a488..60c71cd6dca 100644 --- a/packages/testing/janitor/src/rules/scope-lockdown.rule.test.ts +++ b/packages/testing/janitor/src/rules/scope-lockdown.rule.test.ts @@ -22,7 +22,7 @@ export class TestPage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); const unscopedCall = violations.find((v) => v.message.includes('Unscoped locator')); expect(unscopedCall).toBeDefined(); @@ -44,7 +44,7 @@ export class TestPage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -68,7 +68,7 @@ export class TestPage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -87,7 +87,7 @@ export class TestComponent { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -104,7 +104,7 @@ export class BasePage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -125,7 +125,7 @@ export class LoginPage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); // Has navigation method = explicit standalone page, no violations expect(violations).toHaveLength(0); @@ -146,7 +146,7 @@ export class AmbiguousPage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); // No container AND no navigation method = ambiguous expect(violations).toHaveLength(1); @@ -169,7 +169,7 @@ export class SettingsPage { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); // 'navigate' is in default config, so this should pass expect(violations).toHaveLength(0); diff --git a/packages/testing/janitor/src/rules/scope-lockdown.rule.ts b/packages/testing/janitor/src/rules/scope-lockdown.rule.ts index ea5dcadc51a..335afb68f22 100644 --- a/packages/testing/janitor/src/rules/scope-lockdown.rule.ts +++ b/packages/testing/janitor/src/rules/scope-lockdown.rule.ts @@ -1,3 +1,5 @@ +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import { SyntaxKind, type Project, @@ -6,7 +8,6 @@ import { type ClassDeclaration, } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; import { getConfig } from '../config.js'; import type { Violation } from '../types.js'; import { @@ -69,7 +70,7 @@ function hasNavigationMethod(classDecl: ClassDeclaration, methodNames: string[]) * - this.page.goto(), this.page.waitForURL() etc. (page-level operations) * - Pages with navigation methods (explicit standalone pages) */ -export class ScopeLockdownRule extends BaseRule { +export class ScopeLockdownRule extends AstRule<{ rootDir: string }> { readonly id = 'scope-lockdown'; readonly name = 'Scope Lockdown'; readonly description = 'Page locators must be scoped to container'; @@ -85,7 +86,15 @@ export class ScopeLockdownRule extends BaseRule { return config.patterns.pages; } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, files: SourceFile[] = project.getSourceFiles()): Violation[] { const violations: Violation[] = []; const navigationMethods = this.getNavigationMethods(); @@ -113,7 +122,7 @@ export class ScopeLockdownRule extends BaseRule { const startColumn = classDecl.getStart() - classDecl.getStartLinePos(); violations.push( - this.createViolation( + this.fileViolation( file, startLine, startColumn, @@ -149,7 +158,7 @@ export class ScopeLockdownRule extends BaseRule { const startColumn = call.getStart() - call.getStartLinePos(); violations.push( - this.createViolation( + this.fileViolation( file, startLine, startColumn, diff --git a/packages/testing/janitor/src/rules/selector-purity.rule.test.ts b/packages/testing/janitor/src/rules/selector-purity.rule.test.ts index 3800ab487a7..9df6e4c09f8 100644 --- a/packages/testing/janitor/src/rules/selector-purity.rule.test.ts +++ b/packages/testing/janitor/src/rules/selector-purity.rule.test.ts @@ -20,7 +20,7 @@ export class TestComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations.length).toBeGreaterThan(0); }); @@ -40,7 +40,7 @@ export class TestComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -62,7 +62,7 @@ export class TestComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); @@ -77,7 +77,7 @@ test('my test', async ({ n8n }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations.length).toBeGreaterThan(0); }); @@ -98,7 +98,7 @@ export class TestComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations.length).toBeGreaterThanOrEqual(3); }); @@ -115,7 +115,7 @@ test('my test', async ({ n8n }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations.length).toBeGreaterThan(0); expect(violations[0].message).toContain('Chained locator call'); @@ -134,7 +134,7 @@ test('my test', async ({ n8n }) => { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations.length).toBeGreaterThan(0); expect(violations[0].message).toContain('Chained locator call'); @@ -159,7 +159,7 @@ export class TestComposer { `, ); - const violations = rule.analyze(project, [file]); + const violations = rule.analyzeProject(project, [file]); expect(violations).toHaveLength(0); }); diff --git a/packages/testing/janitor/src/rules/selector-purity.rule.ts b/packages/testing/janitor/src/rules/selector-purity.rule.ts index 2fdb2801f3b..801306ae3fd 100644 --- a/packages/testing/janitor/src/rules/selector-purity.rule.ts +++ b/packages/testing/janitor/src/rules/selector-purity.rule.ts @@ -1,6 +1,7 @@ +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import { SyntaxKind, type Project, type SourceFile, type CallExpression } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; import { getConfig } from '../config.js'; import type { Violation } from '../types.js'; import { LOCATOR_METHODS, PAGE_LEVEL_METHODS, truncateText } from '../utils/ast-helpers.js'; @@ -29,7 +30,7 @@ import { matchesPatterns } from '../utils/paths.js'; * Configuration: * - allowInExpect: Skip violations inside expect() calls (default: false) */ -export class SelectorPurityRule extends BaseRule { +export class SelectorPurityRule extends AstRule<{ rootDir: string }> { readonly id = 'selector-purity'; readonly name = 'Selector Purity'; readonly description = 'Composables and tests should use page objects, not direct locators'; @@ -40,7 +41,15 @@ export class SelectorPurityRule extends BaseRule { return [...config.patterns.flows, ...config.patterns.tests]; } - analyze(_project: Project, files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, files: SourceFile[] = project.getSourceFiles()): Violation[] { const violations: Violation[] = []; const config = getConfig(); @@ -70,7 +79,7 @@ export class SelectorPurityRule extends BaseRule { // Skip violations inside expect() if allowInExpect is enabled // Check both runtime config and config file settings const ruleSettings = getConfig().rules?.[this.id]; - const allowInExpect = this.config.allowInExpect ?? ruleSettings?.allowInExpect; + const allowInExpect = this.getOptions().allowInExpect ?? ruleSettings?.allowInExpect; if (allowInExpect && this.isInsideExpect(call)) { continue; } @@ -83,7 +92,7 @@ export class SelectorPurityRule extends BaseRule { const suggestion = this.getSuggestion(text); violations.push( - this.createViolation( + this.fileViolation( file, startLine, startColumn, @@ -102,7 +111,7 @@ export class SelectorPurityRule extends BaseRule { // Skip violations inside expect() if allowInExpect is enabled const ruleSettings = getConfig().rules?.[this.id]; - const allowInExpect = this.config.allowInExpect ?? ruleSettings?.allowInExpect; + const allowInExpect = this.getOptions().allowInExpect ?? ruleSettings?.allowInExpect; if (allowInExpect && this.isInsideExpect(call)) { continue; } @@ -112,7 +121,7 @@ export class SelectorPurityRule extends BaseRule { const truncated = truncateText(call.getText(), 60); violations.push( - this.createViolation( + this.fileViolation( file, startLine, startColumn, diff --git a/packages/testing/janitor/src/rules/test-data-hygiene.rule.ts b/packages/testing/janitor/src/rules/test-data-hygiene.rule.ts index e4eb70d4d51..93f50b9c020 100644 --- a/packages/testing/janitor/src/rules/test-data-hygiene.rule.ts +++ b/packages/testing/janitor/src/rules/test-data-hygiene.rule.ts @@ -1,12 +1,11 @@ -import * as fs from 'node:fs'; +import { AstRule } from '@n8n/rules-engine/ast'; +import type { AstProjectConfig } from '@n8n/rules-engine/ast'; import * as path from 'node:path'; import { SyntaxKind, type Project, type SourceFile } from 'ts-morph'; -import { BaseRule } from './base-rule.js'; import { getConfig } from '../config.js'; -import { isEditFix } from '../types.js'; -import type { Violation, FixResult } from '../types.js'; -import { getRootDir, getRelativePath, getTestDataFiles } from '../utils/paths.js'; +import type { Violation } from '../types.js'; +import { getRootDir, getTestDataFiles } from '../utils/paths.js'; /** * Test Data Hygiene Rule @@ -24,12 +23,11 @@ import { getRootDir, getRelativePath, getTestDataFiles } from '../utils/paths.js * - Workflows: filename must be referenced in test files * - Expectations: folder must be referenced via loadExpectations() */ -export class TestDataHygieneRule extends BaseRule { +export class TestDataHygieneRule extends AstRule<{ rootDir: string }> { readonly id = 'test-data-hygiene'; readonly name = 'Test Data Hygiene'; readonly description = 'Ensure test data files are well-named and actually used'; readonly severity = 'warning' as const; - readonly fixable = true; // Patterns that indicate poor naming private readonly badNamePatterns = [ @@ -58,7 +56,15 @@ export class TestDataHygieneRule extends BaseRule { ]; } - analyze(project: Project, _files: SourceFile[]): Violation[] { + protected projectConfig(): AstProjectConfig { + return { packages: ['.'], spec: { globs: this.getTargetGlobs() } }; + } + + analyze(context: { rootDir: string }): Violation[] { + return this.projects(context).flatMap(({ project }) => this.analyzeProject(project)); + } + + analyzeProject(project: Project, _files?: SourceFile[]): Violation[] { const violations: Violation[] = []; const config = getConfig(); const root = getRootDir(); @@ -99,8 +105,6 @@ export class TestDataHygieneRule extends BaseRule { root, `Orphaned test data: ${fileName} is not referenced in any test`, 'Remove the file or add a test that uses it', - true, // fixable - { filePath: dataFile }, ), ); } @@ -207,8 +211,6 @@ export class TestDataHygieneRule extends BaseRule { root: string, message: string, suggestion: string, - fixable?: boolean, - fixData?: { filePath: string }, ): Violation { return { file: path.join(root, relativePath), @@ -218,39 +220,6 @@ export class TestDataHygieneRule extends BaseRule { message, severity: this.severity, suggestion, - fixable, - fixData: fixData ? { type: 'edit', replacement: JSON.stringify(fixData) } : undefined, }; } - - fix(_project: Project, violations: Violation[], write: boolean): FixResult[] { - const results: FixResult[] = []; - - for (const violation of violations) { - if (!violation.fixable || !violation.fixData) continue; - - if (!isEditFix(violation.fixData)) continue; - - try { - const data = JSON.parse(violation.fixData.replacement) as { filePath: string }; - const filePath = data.filePath; - const relativePath = getRelativePath(filePath); - - results.push({ - file: relativePath, - action: 'remove-file', - target: path.basename(filePath), - applied: write, - }); - - if (write && fs.existsSync(filePath)) { - fs.unlinkSync(filePath); - } - } catch { - // Skip invalid fix data - } - } - - return results; - } } diff --git a/packages/testing/janitor/src/types.ts b/packages/testing/janitor/src/types.ts index 8319e1c5d05..4cc8168e399 100644 --- a/packages/testing/janitor/src/types.ts +++ b/packages/testing/janitor/src/types.ts @@ -1,6 +1,3 @@ -import type { Violation, RuleResult, FixResult } from '@n8n/rules-engine'; -import type { Project, SourceFile } from 'ts-morph'; - export type { Project, SourceFile, @@ -17,8 +14,6 @@ export type { Violation, RuleResult, ReportSummary, - FixAction, - FixResult, FixData, } from '@n8n/rules-engine'; export type { Report as JanitorReport } from '@n8n/rules-engine'; @@ -57,8 +52,6 @@ export type RuleSettingsMap = { export interface RunOptions { files?: string[]; ruleConfig?: Record; - fix?: boolean; - write?: boolean; } // Janitor-specific fix data narrowing @@ -107,22 +100,6 @@ export function isEditFix(data: { type: string }): data is EditFixData { return data.type === 'edit'; } -export interface Rule { - readonly id: string; - readonly name: string; - readonly description: string; - readonly severity: 'error' | 'warning' | 'info'; - readonly fixable: boolean; - - getTargetGlobs(): string[]; - analyze(project: Project, files: SourceFile[]): Violation[]; - fix?(project: Project, violations: Violation[], write: boolean): FixResult[]; - isEnabled(): boolean; - getEffectiveSeverity(): 'error' | 'warning' | 'info'; - configure(config: RuleConfig): void; - execute(project: Project, files: SourceFile[]): RuleResult; -} - export interface FilePatterns { pages: string[]; components: string[]; diff --git a/packages/testing/rules-engine/package.json b/packages/testing/rules-engine/package.json index f6100580de0..03668e75675 100644 --- a/packages/testing/rules-engine/package.json +++ b/packages/testing/rules-engine/package.json @@ -5,6 +5,16 @@ "description": "Lightweight rules engine for registering, running, and reporting rule violations", "main": "dist/index.js", "types": "dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "default": "./dist/index.js" + }, + "./ast": { + "types": "./dist/ast/index.d.ts", + "default": "./dist/ast/index.js" + } + }, "files": [ "dist" ], @@ -23,6 +33,9 @@ "typecheck": "tsc --noEmit" }, "license": "MIT", + "dependencies": { + "ts-morph": "catalog:" + }, "devDependencies": { "@n8n/vitest-config": "workspace:*", "@vitest/coverage-v8": "catalog:", diff --git a/packages/testing/rules-engine/src/ast/ast-rule.ts b/packages/testing/rules-engine/src/ast/ast-rule.ts new file mode 100644 index 00000000000..9d75398e7a3 --- /dev/null +++ b/packages/testing/rules-engine/src/ast/ast-rule.ts @@ -0,0 +1,63 @@ +import type { Node, SourceFile } from 'ts-morph'; + +import { BaseRule } from '../base-rule.js'; +import type { FixData, Violation } from '../types.js'; +import { buildPackageProjects } from './project.js'; +import type { AstProjectConfig, PackageProject } from './project.js'; + +/** + * Base for AST-backed rules. A rule opts into ts-morph by overriding + * {@link projectConfig}; rules needing no AST leave it undefined and pay nothing. + * The engine core stays ts-morph-free — this lives behind the `/ast` entry point. + */ +export abstract class AstRule< + TContext extends { rootDir: string } = { rootDir: string }, +> extends BaseRule { + protected projectConfig(_context: TContext): AstProjectConfig | undefined { + return undefined; + } + + protected projects(context: TContext): PackageProject[] { + const config = this.projectConfig(context); + return config ? buildPackageProjects(context.rootDir, config) : []; + } + + protected nodeViolation( + node: Node, + message: string, + suggestion?: string, + fixable?: boolean, + fixData?: FixData, + ): Violation { + return this.createViolation( + node.getSourceFile().getFilePath(), + node.getStartLineNumber(), + 0, + message, + suggestion, + fixable, + fixData, + ); + } + + /** Create a violation for a source file at an explicit line/column. */ + protected fileViolation( + file: SourceFile, + line: number, + column: number, + message: string, + suggestion?: string, + fixable?: boolean, + fixData?: FixData, + ): Violation { + return this.createViolation( + file.getFilePath(), + line, + column, + message, + suggestion, + fixable, + fixData, + ); + } +} diff --git a/packages/testing/rules-engine/src/ast/decorators.ts b/packages/testing/rules-engine/src/ast/decorators.ts new file mode 100644 index 00000000000..aea7cc6122b --- /dev/null +++ b/packages/testing/rules-engine/src/ast/decorators.ts @@ -0,0 +1,24 @@ +import { Node } from 'ts-morph'; +import type { ClassDeclaration, Decorator, MethodDeclaration } from 'ts-morph'; + +type Decoratable = ClassDeclaration | MethodDeclaration; + +export function getDecoratorByName( + node: Decoratable, + names: Iterable, +): Decorator | undefined { + const set = names instanceof Set ? names : new Set(names); + return node.getDecorators().find((d) => set.has(d.getName())); +} + +export function classHasDecorator(cls: ClassDeclaration, names: Iterable): boolean { + return getDecoratorByName(cls, names) !== undefined; +} + +/** Read a boolean flag from a decorator's options-object arg, e.g. `@Get('/x', { skipAuth: true })`. */ +export function getDecoratorObjectFlag(deco: Decorator, index: number, flag: string): boolean { + const arg = deco.getArguments()[index]; + if (!arg || !Node.isObjectLiteralExpression(arg)) return false; + const prop = arg.getProperty(flag); + return !!prop && Node.isPropertyAssignment(prop) && prop.getInitializer()?.getText() === 'true'; +} diff --git a/packages/testing/rules-engine/src/ast/index.ts b/packages/testing/rules-engine/src/ast/index.ts new file mode 100644 index 00000000000..c346885b24c --- /dev/null +++ b/packages/testing/rules-engine/src/ast/index.ts @@ -0,0 +1,4 @@ +export { AstRule } from './ast-rule.js'; +export { buildPackageProjects, createInMemoryProject } from './project.js'; +export type { AstProjectConfig, PackageProjectSpec, PackageProject } from './project.js'; +export { classHasDecorator, getDecoratorByName, getDecoratorObjectFlag } from './decorators.js'; diff --git a/packages/testing/rules-engine/src/ast/project.ts b/packages/testing/rules-engine/src/ast/project.ts new file mode 100644 index 00000000000..3023219a4b6 --- /dev/null +++ b/packages/testing/rules-engine/src/ast/project.ts @@ -0,0 +1,51 @@ +import { Project } from 'ts-morph'; +import type { SourceFile } from 'ts-morph'; + +export interface PackageProjectSpec { + /** Globs relative to the package dir; `!`-prefixed entries exclude. Defaults to source `.ts`, no tests. */ + globs?: string[]; + /** Resolve cross-file imports — required by reference-based rules (dead-code, impact). Off by default. */ + resolveDependencies?: boolean; +} + +/** A rule's project requirement. Rules needing no AST return undefined. */ +export interface AstProjectConfig { + /** Package dirs relative to the repo root, e.g. `'packages/cli'`. */ + packages: string[]; + spec?: PackageProjectSpec; + /** Per-package overrides, merged over `spec`. */ + overrides?: Record; +} + +export interface PackageProject { + package: string; + project: Project; + files: SourceFile[]; +} + +const DEFAULT_GLOBS = ['src/**/*.ts', '!src/**/*.test.ts']; + +/** POSIX-join glob segments — `\` breaks globby/minimatch on Windows. */ +const toGlob = (...segments: string[]): string => + segments.join('/').replace(/\\/g, '/').replace(/\/+/g, '/'); + +/** One scoped Project per package — bounded memory, and `resolveDependencies` can differ per package. */ +export function buildPackageProjects(rootDir: string, config: AstProjectConfig): PackageProject[] { + return config.packages.map((pkg) => { + const spec: PackageProjectSpec = { ...config.spec, ...config.overrides?.[pkg] }; + const project = new Project({ + skipAddingFilesFromTsConfig: true, + skipFileDependencyResolution: !spec.resolveDependencies, + }); + const patterns = (spec.globs ?? DEFAULT_GLOBS).map((g) => + g.startsWith('!') ? `!${toGlob(rootDir, pkg, g.slice(1))}` : toGlob(rootDir, pkg, g), + ); + const files = project.addSourceFilesAtPaths(patterns); + return { package: pkg, project, files }; + }); +} + +/** In-memory Project for unit tests — no real filesystem. */ +export function createInMemoryProject(): Project { + return new Project({ useInMemoryFileSystem: true }); +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f045eca2447..e1cae6330f4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -5030,6 +5030,9 @@ importers: fast-glob: specifier: 'catalog:' version: 3.2.12 + ts-morph: + specifier: 'catalog:' + version: 27.0.2 tsx: specifier: 'catalog:' version: 4.19.3 @@ -5234,6 +5237,10 @@ importers: version: 3.25.67 packages/testing/rules-engine: + dependencies: + ts-morph: + specifier: 'catalog:' + version: 27.0.2 devDependencies: '@n8n/vitest-config': specifier: workspace:*