diff --git a/cli/src/commands/lint/lint.ts b/cli/src/commands/lint/lint.ts index f6a35429ef..d031c16b3f 100644 --- a/cli/src/commands/lint/lint.ts +++ b/cli/src/commands/lint/lint.ts @@ -31,10 +31,18 @@ import { import { isFlowInlineScriptPath, isAppInlineScriptPath, - isRawAppPath, + isFolderResourcePathAnyFormat, getFolderSuffix, + getScriptBasePathFromModulePath, } from "../../utils/resource_folders.ts"; -import { exts } from "../script/script.ts"; +import { isFilesetResource } from "../../utils/utils.ts"; +import { + exts, + findContentFile, + hasScriptExt, + isModuleEntryMetadata, + UnresolvableScriptContentFileError, +} from "../script/script.ts"; interface LintOptions extends GlobalOptions { json?: boolean; @@ -67,6 +75,9 @@ export interface LintReport { const YAML_FILE_REGEX = /\.ya?ml$/i; const NATIVE_TRIGGER_REGEX = /\.[^.]+_native_trigger\.ya?ml$/i; +// The metadata suffixes `findContentFile` resolves a flat script from. `.yml` is +// deliberately absent, since the push does not accept it there either. +const FLAT_SCRIPT_METADATA_REGEX = /\.script\.(yaml|json|lock)$/; function normalizePath(p: string): string { return p.replaceAll(SEP, "/"); @@ -643,6 +654,83 @@ export async function checkMissingLocks( return issues; } +/** + * Whether a path is a script's own metadata, as opposed to metadata the push + * deploys through some parent: a folder resource's inline scripts, a fileset's + * children (arbitrarily named, so one may be spelled exactly like a script's + * metadata) and the files of a module or dbt bundle all belong to that parent. + * + * Takes the path as the SYNC ROOT spells it, like the push. Relative to the + * lint target the enclosing folder is gone whenever the target IS that folder; + * absolute, the classifiers match their suffixes ANYWHERE in the string, so a + * checkout under `acme.app` reads as one app and nothing is ever reported. + */ +function isStandaloneScriptMetadata(rootedPath: string): boolean { + // Both suffix formats, because the dotted/non-dotted setting is read from the + // invocation directory and an explicit lint target may not share it. + if ( + isFolderResourcePathAnyFormat(rootedPath) || + isFilesetResource(rootedPath) + ) { + return false; + } + // A module folder keeps its metadata inside itself (`__mod/script.yaml`), + // which is standalone even though every other path under `__mod/` is not. + if (isModuleEntryMetadata(rootedPath)) return true; + if (getScriptBasePathFromModulePath(rootedPath) !== undefined) return false; + return FLAT_SCRIPT_METADATA_REGEX.test(rootedPath); +} + +/** + * `findContentFile` quotes the paths it was given back in its errors, so the + * lint target's own prefix comes off them again. Anchored at a path start: a + * plain substring replace of `f/` also eats the one inside `conf/`, mangling + * the very filename the message is telling the reader to delete. + */ +function relativizeMessage(message: string, prefix: string): string { + if (!prefix) return message; + const escaped = prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + return message.replaceAll(new RegExp(`(^|[\\s(])${escaped}/`, "g"), "$1"); +} + +/** + * Script metadata files that cannot be paired with exactly one content file: + * the push refuses those, and no metadata format makes them deployable, so the + * inactive twin of a format switch (`foo.script.json` in a yaml repo) is dead + * weight worth reporting even though the push skips it rather than refusing it. + * + * Resolved through `findContentFile` rather than by probing `exts` directly, so + * lint and push agree on what counts as paired: a dbt project's descriptor is + * optional and its absence is not an orphan, while two content files beside one + * metadata file is just as undeployable as none. It classifies what it is given + * and looks under `syncRoot`, so where the command was invoked from is not part + * of the answer. + */ +async function checkOrphanScriptMetadata( + syncRoot: string, + prefix: string, + metadataPaths: string[], +): Promise { + const issues: FileIssue[] = []; + for (const metadataPath of metadataPaths) { + const rootedPath = prefix ? `${prefix}/${metadataPath}` : metadataPath; + try { + await findContentFile(rootedPath, syncRoot); + } catch (e) { + if (!(e instanceof UnresolvableScriptContentFileError)) { + log.debug(`Failed to resolve content file for ${rootedPath}: ${e}`); + continue; + } + issues.push({ + path: metadataPath, + target: "script", + errors: [relativizeMessage(e.message, prefix)], + }); + } + } + return issues; +} + export async function runLint( opts: LintOptions, directory?: string, @@ -674,8 +762,16 @@ export async function runLint( const root = await FSFSElement(targetDirectory, [], false); const validator = new WindmillYamlValidator(); + // Walked paths are relative to the lint target; this puts them back the way + // the sync root spells them, which is what the two below are written against. + const syncRoot = await findSyncRoot(targetDirectory); + const metadataPrefix = normalizePath( + path.relative(syncRoot, targetDirectory), + ); + const warnings: LintWarning[] = []; const issues: FileIssue[] = []; + const scriptMetadataPaths: string[] = []; let scannedFiles = 0; let validatedFiles = 0; let validFiles = 0; @@ -689,6 +785,17 @@ export async function runLint( const normalizedPath = normalizePath(entry.path); scannedFiles += 1; + + // Collected before the YAML filter below: `.script.lock` and `.script.json` + // are metadata too, and both fail the push when nothing pairs with them. + if ( + isStandaloneScriptMetadata( + metadataPrefix ? `${metadataPrefix}/${normalizedPath}` : normalizedPath, + ) + ) { + scriptMetadataPaths.push(normalizedPath); + } + if (!YAML_FILE_REGEX.test(normalizedPath)) { continue; } @@ -727,6 +834,16 @@ export async function runLint( } } + // Unconditional: unlike a missing lock, metadata with no content file fails + // every push, so there is no mode in which it is acceptable. + issues.push( + ...(await checkOrphanScriptMetadata( + syncRoot, + metadataPrefix, + scriptMetadataPaths, + )), + ); + // Check for missing locks if --locks-required is set if (opts.locksRequired) { const lockIssues = await checkMissingLocks(opts, explicitTargetDirectory); @@ -820,6 +937,15 @@ async function lint(opts: LintOptions & { watch?: boolean }, directory?: string) } } +/** + * Whether a changed file can change what a lint run reports: metadata in any of + * its formats, and the content files whose presence is what keeps that metadata + * from being an orphan. + */ +function affectsLint(filename: string): boolean { + return /\.(ya?ml|json|lock)$/i.test(filename) || hasScriptExt(filename); +} + async function lintWatch(opts: LintOptions, directory?: string) { const { watch } = await import("node:fs"); const targetDir = directory ? path.resolve(process.cwd(), directory) : process.cwd(); @@ -842,7 +968,7 @@ async function lintWatch(opts: LintOptions, directory?: string) { let debounce: ReturnType | null = null; watch(targetDir, { recursive: true }, (_event, filename) => { - if (!filename || !filename.toString().endsWith(".yaml") && !filename.toString().endsWith(".yml")) return; + if (!filename || !affectsLint(filename.toString())) return; if (debounce) clearTimeout(debounce); debounce = setTimeout(runAndReport, 300); }); @@ -853,7 +979,7 @@ async function lintWatch(opts: LintOptions, directory?: string) { const command = new Command() .description( - "Validate Windmill flow, schedule, and trigger YAML files in a directory", + "Validate Windmill flow, schedule, and trigger YAML files in a directory, and report script metadata that has no deployable content file", ) .arguments("[directory:string]") .option("--json", "Output results in JSON format") diff --git a/cli/src/commands/script/script.ts b/cli/src/commands/script/script.ts index c44d9928f6..fe193e5d85 100644 --- a/cli/src/commands/script/script.ts +++ b/cli/src/commands/script/script.ts @@ -1075,10 +1075,12 @@ export class DbtPathCollisionError extends UnresolvableScriptContentFileError {} * guard on one of them leaves the other silently overwriting. */ export async function collidingDbtProject( - basePath: string + basePath: string, + baseDir?: string ): Promise { const project = basePath + "__dbt/dbt_project.yml"; - return (await stat(project).then(() => true).catch(() => false)) + const onDisk = baseDir ? path.join(baseDir, project) : project; + return (await stat(onDisk).then(() => true).catch(() => false)) ? project : undefined; } @@ -1139,7 +1141,14 @@ async function readScriptContent(filePath: string): Promise { } } -export async function findContentFile(filePath: string) { +/** + * The script file `filePath`'s metadata belongs to. `baseDir`, when given, is + * where the disk lookups happen, leaving `filePath` classified as written: the + * layout helpers below match their suffixes ANYWHERE in a path, so a caller + * that prefixed a checkout named `repo__mod` would have it read as the module. + */ +export async function findContentFile(filePath: string, baseDir?: string) { + const onDisk = (p: string) => (baseDir ? path.join(baseDir, p) : p); // Folder layout: __mod/script.yaml -> __mod/script.ts const isModuleFolderMeta = isModuleEntryMetadata(filePath); const toCandidate = (ext: string) => @@ -1163,7 +1172,7 @@ export async function findContentFile(filePath: string) { const validCandidates = ( await Promise.all( candidates.map((x) => { - return stat(x) + return stat(onDisk(x)) .catch(() => undefined) .then((x) => x?.isFile()) .then((e) => { @@ -1183,6 +1192,7 @@ export async function findContentFile(filePath: string) { const dbtCandidate = toCandidate("__dbt/" + DBT_DESCRIPTOR_NAME); const dbtProject = await collidingDbtProject( dbtCandidate.slice(0, -("__dbt/" + DBT_DESCRIPTOR_NAME).length), + baseDir, ); const nonDbtCandidates = validCandidates.filter((c) => c !== dbtCandidate); if (dbtProject && nonDbtCandidates.length > 0) { diff --git a/cli/src/guidance/skills.gen.ts b/cli/src/guidance/skills.gen.ts index a8790f35a7..176d98eaa6 100644 --- a/cli/src/guidance/skills.gen.ts +++ b/cli/src/guidance/skills.gen.ts @@ -7403,7 +7403,7 @@ Manage jobs (import/export) ### lint -Validate Windmill flow, schedule, and trigger YAML files in a directory +Validate Windmill flow, schedule, and trigger YAML files in a directory, and report script metadata that has no deployable content file **Arguments:** \`[directory:string]\` diff --git a/cli/test/lint_orphan_metadata_unit.test.ts b/cli/test/lint_orphan_metadata_unit.test.ts new file mode 100644 index 0000000000..7066d10dca --- /dev/null +++ b/cli/test/lint_orphan_metadata_unit.test.ts @@ -0,0 +1,166 @@ +import { expect, test, describe } from "bun:test"; +import { mkdtemp, rm, mkdir, writeFile } from "node:fs/promises"; +import os from "node:os"; +import * as path from "node:path"; +import { runLint } from "../src/commands/lint/lint.ts"; + +const WMILL_YAML = "defaultTs: bun\nincludes:\n - f/**\nexcludes: []\n"; +const METADATA = "summary: test\nlock: ''\nschema:\n properties: {}\n"; + +async function write(dir: string, rel: string, content: string) { + const full = path.join(dir, rel); + await mkdir(path.dirname(full), { recursive: true }); + await writeFile(full, content, "utf-8"); +} + +/** + * Runs `fn` with a sync root at `/`, from which lint resolves + * every walked path. The name is a parameter because it is load-bearing: the + * folder suffixes lint classifies by (`.app`, `__mod`, …) are matched anywhere + * in a path, so a root carrying one must not change what lint reports. + */ +async function withSyncRoot( + rootName: string, + fn: (syncRoot: string) => Promise, + opts: { runFromParent?: boolean } = {}, +): Promise { + const tempDir = await mkdtemp(path.join(os.tmpdir(), "wmill_lint_orphan_")); + const syncRoot = path.join(tempDir, rootName); + const originalCwd = process.cwd(); + try { + await write(syncRoot, "wmill.yaml", WMILL_YAML); + process.chdir(opts.runFromParent ? tempDir : syncRoot); + await fn(syncRoot); + } finally { + process.chdir(originalCwd); + await rm(tempDir, { recursive: true }); + } +} + +describe("orphan script metadata", () => { + test("reports metadata with no content file, with locks not required", async () => { + await withSyncRoot("repo", async (syncRoot) => { + await write(syncRoot, "f/paired.py", "def main():\n pass\n"); + await write(syncRoot, "f/paired.script.yaml", METADATA); + await write(syncRoot, "f/orphan.script.yaml", METADATA); + await write(syncRoot, "f/orphan_json.script.json", "{}\n"); + await write(syncRoot, "f/orphan_lock.script.lock", "some-dep==1.0.0\n"); + + const report = await runLint({} as any, syncRoot); + + expect(report.exitCode).toBe(1); + expect(report.issues.map((i) => i.path).sort()).toEqual([ + "f/orphan.script.yaml", + "f/orphan_json.script.json", + "f/orphan_lock.script.lock", + ]); + expect(report.issues[0].target).toBe("script"); + expect(report.issues[0].errors[0]).toContain("No script file found next to"); + }); + }); + + test("reports a module folder's own metadata with no content file", async () => { + await withSyncRoot("repo", async (syncRoot) => { + await write(syncRoot, "f/orphan__mod/script.yaml", METADATA); + + const report = await runLint({} as any, syncRoot); + + expect(report.issues.map((i) => i.path)).toEqual([ + "f/orphan__mod/script.yaml", + ]); + + // Linting the module folder itself: the walked paths no longer carry the + // `__mod/` boundary that says this is a module's metadata. + const inFolder = await runLint( + {} as any, + path.join(syncRoot, "f/orphan__mod"), + ); + + expect(inFolder.issues.map((i) => i.path)).toEqual(["script.yaml"]); + }); + }); + + test("reports orphans under a sync root named like a resource folder", async () => { + await withSyncRoot("acme.app", async (syncRoot) => { + await write(syncRoot, "f/orphan.script.yaml", METADATA); + + const report = await runLint({} as any, syncRoot); + + expect(report.issues.map((i) => i.path)).toEqual(["f/orphan.script.yaml"]); + }); + }); + + test("does not report a paired module under a sync root named like a module folder", async () => { + // Run from OUTSIDE the checkout, the one invocation whose paths carry the + // root's own name: nothing above the sync root may be classified. + await withSyncRoot( + "repo__mod", + async (syncRoot) => { + await write(syncRoot, "f/example__mod/script.yaml", METADATA); + await write( + syncRoot, + "f/example__mod/script.ts", + "export function main() {}\n", + ); + + const report = await runLint({} as any, syncRoot); + + expect(report.issues).toEqual([]); + }, + { runFromParent: true }, + ); + }); + + test("does not report a non-dotted folder resource's child", async () => { + // The dotted/non-dotted setting is read from the invocation directory, so + // an explicit target configured the other way must still be recognized. + await withSyncRoot( + "repo", + async (syncRoot) => { + await write(syncRoot, "f/a__raw_app/raw_app.yaml", "value: {}\n"); + await write(syncRoot, "f/a__raw_app/backend/config.script.lock", "x\n"); + + const report = await runLint({} as any, syncRoot); + + expect(report.issues).toEqual([]); + }, + { runFromParent: true }, + ); + }); + + test("keeps the reported path whole when the lint target is a path segment", async () => { + await withSyncRoot("repo", async (syncRoot) => { + await write(syncRoot, "f/conf/orphan.script.yaml", METADATA); + + const report = await runLint({} as any, path.join(syncRoot, "f")); + + expect(report.issues.map((i) => i.path)).toEqual([ + "conf/orphan.script.yaml", + ]); + expect(report.issues[0].errors[0]).toContain("conf/orphan.script.yaml"); + }); + }); + + test("does not report a fileset child spelled like script metadata", async () => { + await withSyncRoot("repo", async (syncRoot) => { + await write(syncRoot, "f/data.resource.yaml", "value: {}\n"); + await write(syncRoot, "f/data.fileset/config.script.yaml", "a: 1\n"); + + const report = await runLint({} as any, syncRoot); + + expect(report.issues).toEqual([]); + }); + }); + + test("does not report a dbt project whose optional descriptor is absent", async () => { + await withSyncRoot("repo", async (syncRoot) => { + await write(syncRoot, "f/proj.script.yaml", METADATA); + await write(syncRoot, "f/proj__dbt/dbt_project.yml", "name: proj\n"); + await write(syncRoot, "f/proj__dbt/models/a.sql", "select 1\n"); + + const report = await runLint({} as any, syncRoot); + + expect(report.issues).toEqual([]); + }); + }); +}); diff --git a/system_prompts/auto-generated/cli/cli-commands.md b/system_prompts/auto-generated/cli/cli-commands.md index acf6622dc9..833c039ea8 100644 --- a/system_prompts/auto-generated/cli/cli-commands.md +++ b/system_prompts/auto-generated/cli/cli-commands.md @@ -375,7 +375,7 @@ Manage jobs (import/export) ### lint -Validate Windmill flow, schedule, and trigger YAML files in a directory +Validate Windmill flow, schedule, and trigger YAML files in a directory, and report script metadata that has no deployable content file **Arguments:** `[directory:string]` diff --git a/system_prompts/auto-generated/prompts.ts b/system_prompts/auto-generated/prompts.ts index c4b2f5f6cf..e13986771c 100644 --- a/system_prompts/auto-generated/prompts.ts +++ b/system_prompts/auto-generated/prompts.ts @@ -3555,7 +3555,7 @@ Manage jobs (import/export) ### lint -Validate Windmill flow, schedule, and trigger YAML files in a directory +Validate Windmill flow, schedule, and trigger YAML files in a directory, and report script metadata that has no deployable content file **Arguments:** \`[directory:string]\` diff --git a/system_prompts/auto-generated/skills/cli-commands/SKILL.md b/system_prompts/auto-generated/skills/cli-commands/SKILL.md index fef40d3341..e562938f48 100644 --- a/system_prompts/auto-generated/skills/cli-commands/SKILL.md +++ b/system_prompts/auto-generated/skills/cli-commands/SKILL.md @@ -380,7 +380,7 @@ Manage jobs (import/export) ### lint -Validate Windmill flow, schedule, and trigger YAML files in a directory +Validate Windmill flow, schedule, and trigger YAML files in a directory, and report script metadata that has no deployable content file **Arguments:** `[directory:string]`