mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-07 08:02:40 +00:00
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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* fix(cli): correct yaml parse type cast and inline prefix check
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(cli): harden lock path for extensionless files and merge customTags
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
3959fe8297
commit
a8b651da9f
@@ -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);
|
||||
|
||||
+34
-5
@@ -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 });
|
||||
}
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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];
|
||||
|
||||
Reference in New Issue
Block a user