canonicalize a script module path before the add-file checks (#10445)

* fix: canonicalize a script module path before the add-file checks

* fix: match bundle keys without trimming, and stop offering a no-op rename

frontend/src/lib/components/ScriptBuilder.svelte

@@ -43,6 +43,7 @@
 	import { invalidateWorkspacePaths } from './PathNameAutocomplete.svelte'
 	import { notifyContractWarnings } from './assets/AssetGraph/schemaContracts'
 	import ScriptEditor from './ScriptEditor.svelte'
+	import { findModulePathClash } from './scriptModulePath'
 	import { Alert, Button, Drawer, SecondsInput, Tab, TabContent, Tabs } from './common'
 	import LanguageIcon from './common/languageIcons/LanguageIcon.svelte'
 	import type { SupportedLanguage, Schema } from '$lib/common'
@@ -984,7 +985,10 @@
 		// draft that grew modules under another language carries none of what dbt
 		// needs, and the worker refuses a bundle with no `dbt_project.yml` — so
 		// that draft reached dbt in a state it could neither deploy nor run.
-		if (script.modules?.['dbt_project.yml']) return
+		// Matched on the canonical path: a bundle pushed with `./dbt_project.yml`
+		// already has the project file, and seeding a second spelling of it is the
+		// two-keys-one-file collision the editor's add-file checks refuse.
+		if (findModulePathClash(script.modules, 'dbt_project.yml')) return
 		script.modules = {
 			'dbt_project.yml': {
 				content:

frontend/src/lib/components/ScriptEditor.svelte

@@ -23,6 +23,7 @@
 		dbtFileLang,
 		dbtModelSelector
 	} from '$lib/components/dbt/DbtProjectPanel.svelte'
+	import { canonicalModulePath, findModulePathClash } from './scriptModulePath'
 	import SchemaForm from './SchemaForm.svelte'
 	import PowerShellCommonParams from './PowerShellCommonParams.svelte'
 	import LogPanel from './scriptEditor/LogPanel.svelte'
@@ -533,35 +534,43 @@
 	/// The descriptor is the script's CONTENT, not a module. A module at that same
 	/// path would be a second, independent value for one file: the export writes
 	/// the content there, and the bundle would emit over it.
-	function reservedDbtPath(path: string): string | undefined {
-		return lang === 'dbt' && path.trim() === 'wm_dbt.yaml'
+	function reservedDbtPath(canonicalPath: string): string | undefined {
+		return lang === 'dbt' && canonicalPath === 'wm_dbt.yaml'
 			? `wm_dbt.yaml is the descriptor, edited from the tree — it cannot also be a file`
 			: undefined
 	}
 
 	function validateModulePath(path: string): string {
 		if (!path.trim()) return ''
-		const reserved = reservedDbtPath(path)
+		const canonical = canonicalModulePath(path)
+		if ('error' in canonical) return canonical.error
+		const reserved = reservedDbtPath(canonical.path)
 		if (reserved) return reserved
-		const moduleLang = inferModuleLang(path)
+		const moduleLang = inferModuleLang(canonical.path)
 		if (!moduleLang) {
 			const exts = allowedModuleExtensions.join(', ')
 			return `File must end with a supported extension: ${exts}`
 		}
-		const matchedExt = allowedModuleExtensions.find((ext) => path.endsWith(ext))
+		const matchedExt = allowedModuleExtensions.find((ext) => canonical.path.endsWith(ext))
 		if (!matchedExt) {
 			const exts = allowedModuleExtensions.join(', ')
 			return `File must end with a supported extension for this language: ${exts}`
 		}
-		if (modules?.[path.trim()]) {
-			return `Module ${path.trim()} already exists`
+		const clash = findModulePathClash(modules, canonical.path)
+		if (clash) {
+			return `Module ${clash} already exists`
 		}
 		return ''
 	}
 
 	function addModule() {
-		const modulePath = modulePathInput.trim()
-		if (!modulePath) return
+		if (!modulePathInput.trim()) return
+		const canonical = canonicalModulePath(modulePathInput)
+		if ('error' in canonical) {
+			modulePathError = canonical.error
+			return
+		}
+		const modulePath = canonical.path
 		const error = validateModulePath(modulePath)
 		if (error) {
 			modulePathError = error
@@ -592,29 +601,43 @@
 
 	function validateRenameModulePath(newPath: string, oldPath: string): string {
 		if (!newPath.trim()) return ''
-		const reserved = reservedDbtPath(newPath)
+		const canonical = canonicalModulePath(newPath)
+		if ('error' in canonical) return canonical.error
+		const reserved = reservedDbtPath(canonical.path)
 		if (reserved) return reserved
-		const moduleLang = inferModuleLang(newPath)
+		const moduleLang = inferModuleLang(canonical.path)
 		if (!moduleLang) {
 			const exts = allowedModuleExtensions.join(', ')
 			return `File must end with a supported extension: ${exts}`
 		}
-		const matchedExt = allowedModuleExtensions.find((ext) => newPath.endsWith(ext))
+		const matchedExt = allowedModuleExtensions.find((ext) => canonical.path.endsWith(ext))
 		if (!matchedExt) {
 			const exts = allowedModuleExtensions.join(', ')
 			return `File must end with a supported extension for this language: ${exts}`
 		}
-		if (newPath.trim() !== oldPath && modules?.[newPath.trim()]) {
-			return `Module ${newPath.trim()} already exists`
+		const clash = findModulePathClash(modules, canonical.path, oldPath)
+		if (clash) {
+			return `Module ${clash} already exists`
 		}
 		return ''
 	}
 
+	/// A spelling of the name the module already has. Nothing to do, so the button
+	/// that would submit it stays disabled rather than being a dead click.
+	function renameIsNoop(input: string, oldPath: string): boolean {
+		const canonical = canonicalModulePath(input)
+		return 'path' in canonical && canonical.path === oldPath
+	}
+
 	function renameModule(oldPath: string) {
-		const newPath = renameModuleInput.trim()
-		if (!newPath || newPath === oldPath) {
+		if (!renameModuleInput.trim()) return
+		const canonical = canonicalModulePath(renameModuleInput)
+		if ('error' in canonical) {
+			renameModuleError = canonical.error
 			return
 		}
+		const newPath = canonical.path
+		if (newPath === oldPath) return
 		const error = validateRenameModulePath(newPath, oldPath)
 		if (error) {
 			renameModuleError = error
@@ -2590,7 +2613,7 @@
 					close()
 				}}
 				disabled={!renameModuleInput.trim() ||
-					renameModuleInput.trim() === oldPath ||
+					renameIsNoop(renameModuleInput, oldPath) ||
 					!!renameModuleError}>Rename</Button
 			>
 		</div>

frontend/src/lib/components/scriptModulePath.test.ts

@@ -0,0 +1,57 @@
+import { describe, it, expect } from 'vitest'
+import { canonicalModulePath, findModulePathClash } from './scriptModulePath'
+
+describe('canonicalModulePath', () => {
+	// The pair the duplicate and reserved-name checks exist to catch: both
+	// spellings resolve to the same file in the job directory.
+	it('rewrites a redundant spelling to the file it names', () => {
+		expect(canonicalModulePath('./dbt_project.yml')).toEqual({ path: 'dbt_project.yml' })
+		expect(canonicalModulePath('models//x.sql')).toEqual({ path: 'models/x.sql' })
+		expect(canonicalModulePath('  ./models/./sub//x.sql  ')).toEqual({
+			path: 'models/sub/x.sql'
+		})
+		expect(canonicalModulePath('models/x.sql')).toEqual({ path: 'models/x.sql' })
+	})
+
+	it('refuses a path that leaves the bundle', () => {
+		expect(canonicalModulePath('../x.sql')).toHaveProperty('error')
+		expect(canonicalModulePath('models/../../x.sql')).toHaveProperty('error')
+		expect(canonicalModulePath('/etc/x.sql')).toHaveProperty('error')
+		expect(canonicalModulePath('./')).toHaveProperty('error')
+	})
+
+	// Matches the worker's own rule: `..` is traversal only as a whole segment.
+	it('takes dots inside a name as part of the name', () => {
+		expect(canonicalModulePath('models/weird..name.sql')).toEqual({
+			path: 'models/weird..name.sql'
+		})
+	})
+})
+
+describe('findModulePathClash', () => {
+	// A bundle pushed by the CLI can hold a non-canonical key, so the clash has to
+	// be found from either side, and named the way the tree shows it.
+	it('finds a key that resolves to the same file, however either is spelled', () => {
+		const modules = { './dbt_project.yml': {}, 'models/x.sql': {} }
+		expect(findModulePathClash(modules, 'dbt_project.yml')).toBe('./dbt_project.yml')
+		expect(findModulePathClash(modules, 'models/x.sql')).toBe('models/x.sql')
+		expect(findModulePathClash(modules, 'models/y.sql')).toBeUndefined()
+		expect(findModulePathClash(undefined, 'models/x.sql')).toBeUndefined()
+	})
+
+	// The worker does not trim path components, so an imported `x.sql ` is its
+	// own file and must not stand in the way of adding `x.sql`.
+	it('does not fold a key whose name carries whitespace', () => {
+		expect(findModulePathClash({ 'models/x.sql ': {} }, 'models/x.sql')).toBeUndefined()
+	})
+
+	// A rename must not stop at the module being renamed: with both spellings in
+	// the bundle, that would hide the other one and overwrite its content.
+	it('keeps looking past the key being renamed', () => {
+		const modules = { './models/x.sql': {}, 'models/x.sql': {} }
+		expect(findModulePathClash(modules, 'models/x.sql', './models/x.sql')).toBe('models/x.sql')
+		expect(
+			findModulePathClash({ './models/x.sql': {} }, 'models/x.sql', './models/x.sql')
+		).toBeUndefined()
+	})
+})

frontend/src/lib/components/scriptModulePath.ts

@@ -0,0 +1,64 @@
+function canonicalize(path: string): { path: string } | { error: string } {
+	if (path.startsWith('/')) {
+		return { error: `File path must be relative, without a leading /` }
+	}
+	const segments = path.split('/').filter((s) => s !== '' && s !== '.')
+	if (segments.includes('..')) {
+		return { error: `File path cannot contain ..` }
+	}
+	if (segments.length === 0) {
+		return { error: `File name cannot be empty` }
+	}
+	return { path: segments.join('/') }
+}
+
+/**
+ * The canonical spelling of a script module's path (the key of the module
+ * bundle), or the reason it cannot be one.
+ *
+ * The worker resolves `.` and `//` away when it materialises the bundle into
+ * the job directory, so `./dbt_project.yml` and `dbt_project.yml` are two keys
+ * for one file on disk; the bundle is a Rust `HashMap`, so which content lands
+ * there is undefined. Canonicalising before the duplicate and reserved-name
+ * checks is what keeps them from being walked past.
+ *
+ * Redundant spellings are rewritten rather than refused: they name the file the
+ * user meant, and the tree shows the canonical form regardless. `..` and
+ * absolute paths name a file OUTSIDE the bundle, which has no canonical form
+ * inside it, so they are refused here — the worker drops them, which would
+ * otherwise show up as a file that was added and then silently never written.
+ *
+ * Surrounding whitespace is dropped because this is what someone typed into a
+ * text box. A key already in a bundle gets no such courtesy — see
+ * `findModulePathClash`.
+ */
+export function canonicalModulePath(path: string): { path: string } | { error: string } {
+	return canonicalize(path.trim())
+}
+
+/**
+ * The existing module key that would land on the same file as `canonicalPath`,
+ * spelled as the bundle holds it (which is what the file tree shows).
+ *
+ * Keys already in the bundle are not canonical either: nothing on the push path
+ * rewrites them, so a project imported with `./dbt_project.yml` in it must still
+ * refuse a second `dbt_project.yml`. They are matched WITHOUT trimming, because
+ * the worker does not trim path components: an imported `x.sql ` is its own file
+ * on disk and must not stand in the way of adding `x.sql`.
+ *
+ * `ignoreKey` is the module being renamed. It has to be skipped inside the
+ * search rather than compared against the result: a bundle can hold BOTH
+ * spellings, and stopping at the renamed one would hide the other and let the
+ * rename overwrite it.
+ */
+export function findModulePathClash(
+	modules: Record<string, unknown> | null | undefined,
+	canonicalPath: string,
+	ignoreKey?: string
+): string | undefined {
+	return Object.keys(modules ?? {}).find((key) => {
+		if (key === ignoreKey) return false
+		const canonical = canonicalize(key)
+		return 'path' in canonical && canonical.path === canonicalPath
+	})
+}