From a8b651da9ff86766119e14c0b61652be8a7b453a Mon Sep 17 00:00:00 2001 From: centdix <40307056+centdix@users.noreply.github.com> Date: Fri, 27 Mar 2026 20:27:56 +0100 Subject: [PATCH] fix(cli): preserve inline script files during flow generate-locks (#8561) * fix(cli): preserve inline script files during flow generate-locks Three bugs caused `wmill flow generate-locks` to destroy inline script content and rename files: 1. YAML parser stripped unquoted `!inline` tags (treated as YAML tag, not string prefix), leaving just the filename as script content. Fix: register custom YAML tags for `!inline` and `!inline_fileset`. 2. Inline script files were renamed based on step summaries because `extractInlineScriptsForFlows` was called with empty mapping `{}`. Fix: call existing `extractCurrentMapping()` before replacement and pass the mapping to preserve original filenames. 3. Lock file paths were derived from the assigner instead of the mapped content path, causing inconsistent naming. Fix: derive lock base path from mapped content path when available. Co-Authored-By: Claude Opus 4.6 (1M context) * test(cli): add unit tests for !inline YAML tag and mapping preservation - YAML tag tests: unquoted/quoted !inline parsing, !inline_fileset, nested structures, round-trip stability - Mapping tests: path preservation with mapping, fallthrough without mapping, lock path derivation from mapped content path, mixed mapped/unmapped modules, dotted path handling Co-Authored-By: Claude Opus 4.6 (1M context) * fix(cli): correct yaml parse type cast and inline prefix check Co-Authored-By: Claude Opus 4.6 (1M context) * fix(cli): harden lock path for extensionless files and merge customTags Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- cli/src/commands/flow/flow_metadata.ts | 19 ++++- cli/src/utils/yaml.ts | 39 ++++++++-- ..._scripts_failure_preprocessor_unit.test.ts | 78 +++++++++++++++++++ cli/test/yaml_inline_tag.test.ts | 58 ++++++++++++++ .../src/inline-scripts/extractor.ts | 13 +++- 5 files changed, 195 insertions(+), 12 deletions(-) create mode 100644 cli/test/yaml_inline_tag.test.ts diff --git a/cli/src/commands/flow/flow_metadata.ts b/cli/src/commands/flow/flow_metadata.ts index 1f5d86b8ea..9805391d05 100644 --- a/cli/src/commands/flow/flow_metadata.ts +++ b/cli/src/commands/flow/flow_metadata.ts @@ -18,7 +18,7 @@ import { filterWorkspaceDependenciesForScripts, } from "../../utils/metadata.ts"; import { ScriptLanguage } from "../../utils/script_common.ts"; -import { extractInlineScripts as extractInlineScriptsForFlows } from "../../../windmill-utils-internal/src/inline-scripts/extractor.ts"; +import { extractInlineScripts as extractInlineScriptsForFlows, extractCurrentMapping } from "../../../windmill-utils-internal/src/inline-scripts/extractor.ts"; import { newPathAssigner } from "../../../windmill-utils-internal/src/path-utils/path-assigner.ts"; import { generateHash, getHeaders, writeIfChanged } from "../../utils/utils.ts"; @@ -188,6 +188,17 @@ export async function generateFlowLockInternal( log.info(`Recomputing locks of ${changedScripts.join(", ")} in ${folder}`); } const fileReader = async (path: string) => await readFile(folder + SEP + path, "utf-8"); + + // Capture existing module-ID-to-file-path mapping before replaceInlineScripts + // overwrites the !inline references with actual file content. This preserves + // the original filenames when re-extracting inline scripts after lock generation. + const currentMapping = extractCurrentMapping( + flowValue.value.modules, + {}, + flowValue.value.failure_module, + flowValue.value.preprocessor_module, + ); + // In tree mode, use the tree's staleness info (which includes transitive dependency changes) // to determine which scripts need relocking, instead of only content-changed ones. const locksToRemove = (tree && !legacyBehaviour) @@ -228,16 +239,16 @@ export async function generateFlowLockInternal( }); const inlineScripts = extractInlineScriptsForFlows( flowValue.value.modules, - {}, + currentMapping, SEP, opts.defaultTs, lockAssigner ); if (flowValue.value.failure_module) { - inlineScripts.push(...extractInlineScriptsForFlows([flowValue.value.failure_module], {}, SEP, opts.defaultTs, lockAssigner)); + inlineScripts.push(...extractInlineScriptsForFlows([flowValue.value.failure_module], currentMapping, SEP, opts.defaultTs, lockAssigner)); } if (flowValue.value.preprocessor_module) { - inlineScripts.push(...extractInlineScriptsForFlows([flowValue.value.preprocessor_module], {}, SEP, opts.defaultTs, lockAssigner)); + inlineScripts.push(...extractInlineScriptsForFlows([flowValue.value.preprocessor_module], currentMapping, SEP, opts.defaultTs, lockAssigner)); } inlineScripts.forEach((s) => { writeIfChanged(process.cwd() + SEP + folder + SEP + s.path, s.content); diff --git a/cli/src/utils/yaml.ts b/cli/src/utils/yaml.ts index 9ad247c1fd..52ec682067 100644 --- a/cli/src/utils/yaml.ts +++ b/cli/src/utils/yaml.ts @@ -1,9 +1,35 @@ -import { parse as yamlParse, type ParseOptions } from "yaml"; +import { parse as yamlParse } from "yaml"; +import type { ParseOptions, DocumentOptions, SchemaOptions, ToJSOptions, ScalarTag } from "yaml"; import { readFile } from "node:fs/promises"; -export async function yamlParseFile(path: string, options: ParseOptions = {}) { +// Custom YAML tags that resolve `!inline value` and `!inline_fileset value` +// back to their string-prefix form ("!inline value"). +// Without these, the yaml parser strips the tag and returns just the scalar, +// breaking the string-prefix-based !inline detection used throughout the CLI. +const inlineTag: ScalarTag = { + tag: "!inline", + resolve(value: string) { + return "!inline " + value; + }, +}; + +const inlineFilesetTag: ScalarTag = { + tag: "!inline_fileset", + resolve(value: string) { + return "!inline_fileset " + value; + }, +}; + +const WINDMILL_CUSTOM_TAGS: ScalarTag[] = [inlineTag, inlineFilesetTag]; + +type YamlParseOptions = ParseOptions & DocumentOptions & SchemaOptions & ToJSOptions; + +export async function yamlParseFile(path: string, options: YamlParseOptions = {}) { try { - return yamlParse(await readFile(path, "utf-8"), options); + return yamlParse(await readFile(path, "utf-8"), { + ...options, + customTags: [...WINDMILL_CUSTOM_TAGS, ...((options.customTags as ScalarTag[] | undefined) ?? [])], + }); } catch (e) { throw new Error(`Error parsing yaml ${path}`, { cause: e }); } @@ -12,10 +38,13 @@ export async function yamlParseFile(path: string, options: ParseOptions = {}) { export function yamlParseContent( path: string, content: string, - options: ParseOptions = {}, + options: YamlParseOptions = {}, ) { try { - return yamlParse(content, options); + return yamlParse(content, { + ...options, + customTags: [...WINDMILL_CUSTOM_TAGS, ...((options.customTags as ScalarTag[] | undefined) ?? [])], + }); } catch (e) { throw new Error(`Error parsing yaml ${path}`, { cause: e }); } diff --git a/cli/test/inline_scripts_failure_preprocessor_unit.test.ts b/cli/test/inline_scripts_failure_preprocessor_unit.test.ts index 1a9150a257..78af37a02e 100644 --- a/cli/test/inline_scripts_failure_preprocessor_unit.test.ts +++ b/cli/test/inline_scripts_failure_preprocessor_unit.test.ts @@ -496,3 +496,81 @@ describe("extractCurrentMapping for failure_module / preprocessor_module", () => expect(mapping["failure"]).toBeUndefined(); }); }); + +// --------------------------------------------------------------------------- +// extractInlineScripts with mapping — path preservation +// --------------------------------------------------------------------------- + +describe("extractInlineScripts with mapping preserves file paths", () => { + test("uses mapped path instead of assigner-generated path", () => { + const mod = makeRawscriptModule("a", "console.log('hi')", "bun"); + mod.summary = "Get Users Data"; + + const mapping = { a: "get_users.ts" }; + const scripts = extractInlineScripts([mod], mapping, "/", "bun"); + + const contentScript = scripts.find((s) => !s.is_lock); + expect(contentScript!.path).toBe("get_users.ts"); + // Module content should reference the mapped path + expect(mod.value.content).toBe("!inline get_users.ts"); + }); + + test("falls through to assigner when module ID not in mapping", () => { + const mod = makeRawscriptModule("a", "console.log('hi')", "bun"); + mod.summary = "Get Users Data"; + + const mapping = { other_id: "other.ts" }; + const scripts = extractInlineScripts([mod], mapping, "/", "bun"); + + const contentScript = scripts.find((s) => !s.is_lock); + // Should use assigner path based on summary, not mapped + expect(contentScript!.path).toContain("get_users_data"); + }); + + test("mapped modules and unmapped modules coexist", () => { + const modA = makeRawscriptModule("a", "code_a", "bun"); + modA.summary = "Step A"; + const modB = makeRawscriptModule("b", "code_b", "bun"); + modB.summary = "Step B"; + + const mapping = { a: "my_custom_name.ts" }; // only a is mapped + const scripts = extractInlineScripts([modA, modB], mapping, "/", "bun"); + + const paths = scripts.filter((s) => !s.is_lock).map((s) => s.path); + expect(paths[0]).toBe("my_custom_name.ts"); + expect(paths[1]).toContain("step_b"); // assigner-generated from summary + }); + + test("lock path is derived from mapped content path", () => { + const mod = makeRawscriptModule("a", "code", "bun", "lock-content"); + mod.summary = "Get Users Data"; + + const mapping = { a: "get_users.ts" }; + const scripts = extractInlineScripts([mod], mapping, "/", "bun"); + + const lockScript = scripts.find((s) => s.is_lock); + expect(lockScript!.path).toBe("get_users.lock"); + expect((mod.value as any).lock).toBe("!inline get_users.lock"); + }); + + test("lock path uses assigner basePath when no mapping", () => { + const mod = makeRawscriptModule("a", "code", "bun", "lock-content"); + mod.summary = "Get Users Data"; + + const scripts = extractInlineScripts([mod], {}, "/", "bun"); + + const lockScript = scripts.find((s) => s.is_lock); + expect(lockScript!.path).toContain("get_users_data"); + expect(lockScript!.path).toEndWith(".lock"); + }); + + test("lock path handles dotted content paths correctly", () => { + const mod = makeRawscriptModule("a", "code", "bun", "lock-content"); + + const mapping = { a: "my.inline_script.ts" }; + const scripts = extractInlineScripts([mod], mapping, "/", "bun"); + + const lockScript = scripts.find((s) => s.is_lock); + expect(lockScript!.path).toBe("my.inline_script.lock"); + }); +}); diff --git a/cli/test/yaml_inline_tag.test.ts b/cli/test/yaml_inline_tag.test.ts new file mode 100644 index 0000000000..79638c7b50 --- /dev/null +++ b/cli/test/yaml_inline_tag.test.ts @@ -0,0 +1,58 @@ +/** + * Unit tests for custom !inline and !inline_fileset YAML tag handling. + * These tests require no backend — they test YAML parsing logic. + */ + +import { expect, test, describe } from "bun:test"; +import { yamlParseContent } from "../src/utils/yaml.ts"; +import { stringify as yamlStringify } from "yaml"; + +describe("YAML !inline tag resolution", () => { + test("unquoted !inline resolves to string with prefix", () => { + const result = yamlParseContent("test.yaml", "content: !inline get_users.ts"); + expect(result.content).toBe("!inline get_users.ts"); + }); + + test("quoted !inline is preserved as-is", () => { + const result = yamlParseContent("test.yaml", 'content: "!inline get_users.ts"'); + expect(result.content).toBe("!inline get_users.ts"); + }); + + test("unquoted and quoted produce identical results", () => { + const unquoted = yamlParseContent("test.yaml", "content: !inline script.ts"); + const quoted = yamlParseContent("test.yaml", 'content: "!inline script.ts"'); + expect(unquoted.content).toBe(quoted.content); + }); + + test("unquoted !inline_fileset resolves to string with prefix", () => { + const result = yamlParseContent("test.yaml", "value: !inline_fileset my_resource.fileset"); + expect(result.value).toBe("!inline_fileset my_resource.fileset"); + }); + + test("works within nested flow.yaml structure", () => { + const yaml = ` +value: + modules: + - id: a + value: + type: rawscript + content: !inline get_users.ts + language: bun + - id: b + value: + type: rawscript + content: !inline send_mail.ts + language: bun`; + const result = yamlParseContent("flow.yaml", yaml); + expect(result.value.modules[0].value.content).toBe("!inline get_users.ts"); + expect(result.value.modules[1].value.content).toBe("!inline send_mail.ts"); + }); + + test("round-trip: parse unquoted → stringify → parse preserves value", () => { + const yaml = "content: !inline my_script.ts"; + const parsed = yamlParseContent("test.yaml", yaml); + const serialized = yamlStringify(parsed); + const reparsed = yamlParseContent("test.yaml", serialized); + expect(reparsed.content).toBe("!inline my_script.ts"); + }); +}); diff --git a/cli/windmill-utils-internal/src/inline-scripts/extractor.ts b/cli/windmill-utils-internal/src/inline-scripts/extractor.ts index 0ad1bb8302..e472372a99 100644 --- a/cli/windmill-utils-internal/src/inline-scripts/extractor.ts +++ b/cli/windmill-utils-internal/src/inline-scripts/extractor.ts @@ -23,14 +23,21 @@ function extractRawscriptInline( assigner: PathAssigner ): InlineScript[] { const [basePath, ext] = assigner.assignPath(summary ?? id, rawscript.language); - const path = mapping[id] ?? basePath + ext; + const mappedPath = mapping[id]; + const path = mappedPath ?? basePath + ext; const language = rawscript.language; const content = rawscript.content; const r = [{ path: path, content: content, language, is_lock: false}]; rawscript.content = "!inline " + path.replaceAll(separator, "/"); const lock = rawscript.lock; if (lock && lock != "") { - const lockPath = basePath + "lock"; + // Derive lock path base from the mapped content path when available, + // so lock files are named consistently with their content files. + const dotIdx = mappedPath ? mappedPath.lastIndexOf('.') : -1; + const lockBasePath = mappedPath + ? (dotIdx > 0 ? mappedPath.substring(0, dotIdx + 1) : mappedPath + '.') + : basePath; + const lockPath = lockBasePath + "lock"; rawscript.lock = "!inline " + lockPath.replaceAll(separator, "/"); r.push({ path: lockPath, content: lock, language, is_lock: true}); } @@ -191,7 +198,7 @@ export function extractCurrentMapping( } else if (m.value.type === "aiagent") { (m.value.tools ?? []).forEach((tool) => { const toolValue = tool.value; - if (!toolValue || toolValue.tool_type !== 'flowmodule' || toolValue.type !== 'rawscript' || !toolValue.content || !toolValue.content.startsWith("!inline")) { + if (!toolValue || toolValue.tool_type !== 'flowmodule' || toolValue.type !== 'rawscript' || !toolValue.content || !toolValue.content.startsWith("!inline ")) { return; } mapping[tool.id] = toolValue.content.trim().split(" ")[1];