From 2b88c4d0546a53a84f42b64ec40e5747494e92a1 Mon Sep 17 00:00:00 2001 From: JayceeB1 <45721154+JayceeB1@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:16:06 +0200 Subject: [PATCH 1/7] integrate(#370): fail-closed storage operations (experimental preview) Source: lightningpixel/modly#370; preserves released Modly 0.4.3. --- electron/main/ipc-handlers.ts | 22 +- electron/main/storage-directory-guard.test.ts | 172 +++++++++++++++ electron/main/storage-directory-guard.ts | 199 ++++++++++++++++++ package.json | 2 +- 4 files changed, 377 insertions(+), 18 deletions(-) create mode 100644 electron/main/storage-directory-guard.test.ts create mode 100644 electron/main/storage-directory-guard.ts diff --git a/electron/main/ipc-handlers.ts b/electron/main/ipc-handlers.ts index e9cac040..f6243967 100644 --- a/electron/main/ipc-handlers.ts +++ b/electron/main/ipc-handlers.ts @@ -42,6 +42,7 @@ import { weightStorageHasLocalData, } from './model-sources' import { getSettings, setSettings } from './settings-store' +import { deleteStorageDirectory, moveStorageDirectory } from './storage-directory-guard' import { checkSetupNeeded, markSetupDone, runFullSetup, getVenvPythonExe, ensureSslPatch } from './python-setup' import { logger } from './logger' import { getProcessRunner, getPythonProcessRunner, getExtPythonExe, terminateProcessRunner, terminateAllProcessRunners } from './process-runner' @@ -1091,9 +1092,8 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe ipcMain.handle('fs:moveDirectory', async (_, { src, dest }: { src: string; dest: string }) => { try { - await mkdir(dest, { recursive: true }) - await cp(src, dest, { recursive: true }) - await rmAsync(src, { recursive: true, force: true }) + const userData = app.getPath('userData') + await moveStorageDirectory(src, dest, getSettings(userData), userData, { appDir: app.getAppPath() }) return { success: true } } catch (err) { return { success: false, error: String(err) } @@ -1101,21 +1101,9 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe }) ipcMain.handle('fs:deleteDirectory', async (_, dirPath: string) => { - const userData = app.getPath('userData') - const settings = getSettings(userData) - const allowedRoots = [ - settings.modelsDir, - settings.workspaceDir, - settings.extensionsDir, - join(userData, 'gen-cache'), - ] - const resolved = join(dirPath) - const isAllowed = allowedRoots.some((root) => resolved.startsWith(root)) - if (!isAllowed) { - return { success: false, error: 'Path is outside allowed directories' } - } try { - await rmAsync(resolved, { recursive: true, force: true }) + const userData = app.getPath('userData') + await deleteStorageDirectory(dirPath, getSettings(userData), userData, { appDir: app.getAppPath() }) return { success: true } } catch (err) { return { success: false, error: String(err) } diff --git a/electron/main/storage-directory-guard.test.ts b/electron/main/storage-directory-guard.test.ts new file mode 100644 index 00000000..d40aa3fa --- /dev/null +++ b/electron/main/storage-directory-guard.test.ts @@ -0,0 +1,172 @@ +import assert from 'node:assert/strict' +import test from 'node:test' +import { mkdtemp, mkdir, rm, symlink, writeFile, readFile, access } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { + validateStorageDeletion, + validateStorageMove, + assertNoSymlinkAncestors, + assertEmptyDestination, + deleteStorageDirectory, + moveStorageDirectory, +} from './storage-directory-guard' + +const dirs = { + modelsDir: 'E:\\modly\\models', + workspaceDir: 'E:\\modly\\workspace', + workflowsDir: 'E:\\modly\\workflows', + extensionsDir: 'E:\\modly\\extensions', + dependenciesDir: 'E:\\modly\\dependencies', + agentDir: 'E:\\modly\\agent', +} +const home = 'C:\\Users\\Tester' +const userData = 'C:\\Users\\Tester\\AppData\\Roaming\\modly' +const opts = { platform: 'win32' as const, homeDir: home, appDir: 'C:\\Program Files\\Modly\\resources\\app.asar' } + +test('deletion permits an exact configured managed directory', () => { + assert.equal(validateStorageDeletion('e:\\MODLY\\models\\\\', dirs, userData, opts), dirs.modelsDir) + assert.equal(validateStorageDeletion(dirs.workflowsDir, dirs, userData, opts), dirs.workflowsDir) + assert.equal(validateStorageDeletion('E:\\modly\\workspace\\tmp', dirs, userData, opts), 'E:\\modly\\workspace\\tmp') +}) + +test('deletion rejects filesystem roots, shallow folders and relative paths', () => { + for (const bad of ['E:\\\\', 'C:\\\\', 'E:\\Photos', 'models', '.', '']) { + assert.throws(() => validateStorageDeletion(bad, { ...dirs, modelsDir: bad }, userData, opts)) + } + assert.throws(() => validateStorageDeletion('\\\\server\\share\\models', dirs, userData, opts)) +}) + +test('deletion requires an exact configured root or workspace/tmp (not a prefix)', () => { + for (const bad of [ + 'E:\\modly\\models-backup', 'E:\\modly\\models\\elsewhere', + 'E:\\modly\\workspace\\tmp-fake', 'E:\\modly\\workflows2', + 'E:\\elsewhere\\models', + ]) { + assert.throws(() => validateStorageDeletion(bad, dirs, userData, opts), /not an approved/i) + } +}) + +test('deletion rejects home, application and userData ancestors even if configured', () => { + for (const bad of [home, 'C:\\Users', 'C:\\Program Files', 'C:\\Users\\Tester\\AppData', userData]) { + assert.throws(() => validateStorageDeletion(bad, { ...dirs, modelsDir: bad }, userData, opts)) + } +}) + +test('deletion refuses one configured folder enclosing another', () => { + assert.throws(() => validateStorageDeletion(dirs.modelsDir, { + ...dirs, workspaceDir: 'E:\\modly\\models\\my-workspace', + }, userData, opts), /another configured/i) +}) + +test('move allows separate configured source and empty destination path', () => { + assert.deepEqual(validateStorageMove(dirs.modelsDir, 'D:\\modly-assets\\models', dirs, userData, opts), { + src: dirs.modelsDir, dest: 'D:\\modly-assets\\models', + }) +}) + +test('move rejects rogue sources, root, parent, child and overlapping destinations', () => { + const badSources = ['E:\\\\', home, 'E:\\modly\\models-other', 'E:\\modly\\models\\child'] + for (const bad of badSources) { + assert.throws(() => validateStorageMove(bad, 'D:\\modly\\models', dirs, userData, opts)) + } + for (const bad of [ + dirs.modelsDir, 'E:\\modly', 'E:\\modly\\models\\new', 'E:\\modly\\workflows', + 'E:\\\\', 'D:\\Backups', 'E:\\modly\\workspace\\inside', + ]) { + assert.throws(() => validateStorageMove(dirs.modelsDir, bad, dirs, userData, opts)) + } +}) + +test('POSIX root and relative path are rejected', () => { + const linuxDirs = { + modelsDir: '/home/tester/modly/models', + workspaceDir: '/home/tester/modly/workspace', + workflowsDir: '/home/tester/modly/workflows', + extensionsDir: '/home/tester/modly/extensions', + dependenciesDir: '/home/tester/modly/dependencies', + agentDir: '/home/tester/modly/agent', + } + const linuxOpts = { platform: 'linux' as const, homeDir: '/home/tester' } + assert.equal(validateStorageDeletion(linuxDirs.modelsDir, linuxDirs, '/home/tester/.config/modly', linuxOpts), linuxDirs.modelsDir) + assert.throws(() => validateStorageDeletion('/', { ...linuxDirs, modelsDir: '/' }, '/home/tester/.config/modly', linuxOpts)) + assert.throws(() => validateStorageMove('relative', '/tmp/safe/new', linuxDirs, '/home/tester/.config/modly', linuxOpts)) +}) + +test('destination must be empty, missing is fine', async () => { + const base = await mkdtemp(join(tmpdir(), 'modly-storage-')) + try { + await assertEmptyDestination(join(base, 'new')) + await mkdir(join(base, 'empty')) + await assertEmptyDestination(join(base, 'empty')) + await writeFile(join(base, 'empty', 'personal.txt'), 'do not overwrite') + await assert.rejects(() => assertEmptyDestination(join(base, 'empty')), /empty/i) + } finally { + await rm(base, { recursive: true, force: true }) + } +}) + +test('symlinked ancestors and target are refused', { skip: process.platform === 'win32' }, async () => { + const base = await mkdtemp(join(tmpdir(), 'modly-storage-link-')) + try { + await mkdir(join(base, 'real', 'models'), { recursive: true }) + await symlink(join(base, 'real'), join(base, 'alias'), 'dir') + await assertNoSymlinkAncestors(join(base, 'real', 'models')) + await assert.rejects(() => assertNoSymlinkAncestors(join(base, 'alias', 'models')), /symbolic link/i) + await assert.rejects(() => assertNoSymlinkAncestors(join(base, 'alias')), /symbolic link/i) + } finally { + await rm(base, { recursive: true, force: true }) + } +}) + +test('unsafe real deletion refuses a storage root and preserves personal files', async () => { + const base = await mkdtemp(join(tmpdir(), 'modly-real-delete-')) + const managed = join(base, 'models') + const personal = join(base, 'personal.txt') + const unixDirs = { + modelsDir: managed, workspaceDir: join(base, 'workspace'), + workflowsDir: join(base, 'workflows'), extensionsDir: join(base, 'extensions'), + dependenciesDir: join(base, 'dependencies'), agentDir: join(base, 'agent'), + } + const options = { platform: process.platform, homeDir: join(base, 'other-home') } + try { + await mkdir(managed) + await writeFile(personal, 'important') + await assert.rejects(() => deleteStorageDirectory(base, { ...unixDirs, modelsDir: base }, + join(base, 'userData'), options), /ancestor|another configured|approved/i) + assert.equal(await readFile(personal, 'utf8'), 'important') + await deleteStorageDirectory(managed, unixDirs, join(base, 'userData'), options) + await assert.rejects(() => access(managed)) + assert.equal(await readFile(personal, 'utf8'), 'important') + } finally { + await rm(base, { recursive: true, force: true }) + } +}) + +test('real move does not overwrite destination files or erase unrelated data', async () => { + const base = await mkdtemp(join(tmpdir(), 'modly-real-move-')) + const source = join(base, 'models') + const destination = join(base, 'migrated', 'models') + const unixDirs = { + modelsDir: source, workspaceDir: join(base, 'workspace'), + workflowsDir: join(base, 'workflows'), extensionsDir: join(base, 'extensions'), + dependenciesDir: join(base, 'dependencies'), agentDir: join(base, 'agent'), + } + const options = { platform: process.platform, homeDir: join(base, 'other-home') } + try { + await mkdir(source) + await mkdir(destination, { recursive: true }) + await writeFile(join(source, 'asset.glb'), 'model') + await writeFile(join(destination, 'personal.txt'), 'preserve') + await assert.rejects(() => moveStorageDirectory(source, destination, unixDirs, + join(base, 'userData'), options), /empty/i) + assert.equal(await readFile(join(destination, 'personal.txt'), 'utf8'), 'preserve') + assert.equal(await readFile(join(source, 'asset.glb'), 'utf8'), 'model') + await rm(destination, { recursive: true }) + await moveStorageDirectory(source, destination, unixDirs, join(base, 'userData'), options) + assert.equal(await readFile(join(destination, 'asset.glb'), 'utf8'), 'model') + await assert.rejects(() => access(source)) + } finally { + await rm(base, { recursive: true, force: true }) + } +}) diff --git a/electron/main/storage-directory-guard.ts b/electron/main/storage-directory-guard.ts new file mode 100644 index 00000000..fb1e9b91 --- /dev/null +++ b/electron/main/storage-directory-guard.ts @@ -0,0 +1,199 @@ +/** Fail-closed validation for settings-driven directory moves and deletions. */ +import { cp, lstat, mkdir, readdir, rm } from 'node:fs/promises' +import { homedir } from 'node:os' +import path from 'node:path' + +type StorageDirs = { + modelsDir: string + workspaceDir: string + workflowsDir: string + extensionsDir: string + dependenciesDir: string + agentDir: string +} + +type Options = { + platform?: NodeJS.Platform + homeDir?: string + appDir?: string +} + +type PathApi = typeof path.win32 + +function pathApi(options: Options): PathApi { + return options.platform === 'win32' || (!options.platform && process.platform === 'win32') + ? path.win32 + : path.posix +} + +function absoluteDirectory(value: string, api: PathApi): string { + if (typeof value !== 'string' || !value.trim() || value.includes('\0') || !api.isAbsolute(value)) { + throw new Error('Storage path must be a non-empty absolute path') + } + // Drive-relative, extended Windows and UNC paths need separate review. + if (api === path.win32 && !/^[a-z]:[\\/]/i.test(value)) { + throw new Error('Storage operations require a local drive-absolute path') + } + const resolved = api.normalize(value) + if (resolved === api.parse(resolved).root) { + throw new Error('Refusing a filesystem or volume root') + } + // Never recursively delete a bare top-level user folder, even if configured. + const pieces = resolved.slice(api.parse(resolved).root.length).split(api.sep).filter(Boolean) + if (pieces.length < 2) { + throw new Error('Storage directory must be at least two levels below its volume root') + } + return resolved +} + +function equalPath(api: PathApi, a: string, b: string): boolean { + return api.relative(a, b) === '' +} + +function sameOrInside(api: PathApi, parent: string, candidate: string): boolean { + const relative = api.relative(parent, candidate) + return relative === '' || (relative !== '..' && !relative.startsWith('..' + api.sep) && !api.isAbsolute(relative)) +} + +function storageEntries(dirs: StorageDirs): Array<[string, string]> { + return [ + ['models', dirs.modelsDir], + ['workspace', dirs.workspaceDir], + ['workflows', dirs.workflowsDir], + ['extensions', dirs.extensionsDir], + ['dependencies', dirs.dependenciesDir], + ['agent', dirs.agentDir], + ] +} + +function verifySource(source: string, userData: string, options: Options): string { + const api = pathApi(options) + const safe = absoluteDirectory(source, api) + const protectedRoots = [userData, options.homeDir ?? homedir(), options.appDir] + for (const item of protectedRoots) { + if (!item) continue + const protectedPath = api.normalize(item) + if (api.isAbsolute(protectedPath) && sameOrInside(api, safe, protectedPath)) { + throw new Error('Refusing to remove an application, home, or user-data ancestor') + } + } + return safe +} + +function verifyNoOtherStorageInside( + source: string, selectedKey: string, dirs: StorageDirs, options: Options, +): void { + const api = pathApi(options) + for (const [key, value] of storageEntries(dirs)) { + if (key === selectedKey || !value) continue + const other = api.normalize(value) + if (api.isAbsolute(other) && sameOrInside(api, source, other)) { + throw new Error('Refusing to remove another configured storage directory (' + key + ')') + } + } +} + +/** Only exact configured roots, plus the specific workspace/tmp cleanup, are deletable. */ +export function validateStorageDeletion( + directory: string, dirs: StorageDirs, userData: string, options: Options = {}, +): string { + const api = pathApi(options) + const target = absoluteDirectory(directory, api) + const roots: Array<[string, string]> = [ + ['models', dirs.modelsDir], + ['workspace', dirs.workspaceDir], + ['workflows', dirs.workflowsDir], + ['extensions', dirs.extensionsDir], + ['cache', api.join(userData, 'gen-cache')], + ] + const selected = roots.find(([, root]) => root && equalPath(api, api.normalize(root), target)) + if (selected) { + const safe = verifySource(selected[1], userData, options) + verifyNoOtherStorageInside(safe, selected[0], dirs, options) + return safe + } + const workspace = verifySource(dirs.workspaceDir, userData, options) + if (equalPath(api, target, api.join(workspace, 'tmp'))) { + verifyNoOtherStorageInside(target, 'workspace', dirs, options) + return target + } + throw new Error('Path is not an approved Modly storage directory') +} + +/** A folder move may delete its source: restrict source and destination equally. */ +export function validateStorageMove( + source: string, destination: string, dirs: StorageDirs, userData: string, options: Options = {}, +): { src: string; dest: string } { + const api = pathApi(options) + const src = verifySource(source, userData, options) + const selected = storageEntries(dirs).find(([key, val]) => + ['models', 'workspace', 'workflows'].includes(key) && equalPath(api, api.normalize(val), src)) + if (!selected) throw new Error('Source is not a configured Modly storage directory') + verifyNoOtherStorageInside(src, selected[0], dirs, options) + const dest = verifySource(destination, userData, options) + if (sameOrInside(api, src, dest) || sameOrInside(api, dest, src)) { + throw new Error('Source and destination must be separate, non-overlapping directories') + } + // Reject destinations overlapping ANY configured storage root. + for (const [, value] of storageEntries(dirs)) { + if (!value) continue + const other = api.normalize(value) + if (sameOrInside(api, other, dest) || sameOrInside(api, dest, other)) { + throw new Error('Destination overlaps configured storage') + } + } + return { src, dest } +} + +/** Reject symlinks or Windows junctions in existing path ancestors. */ +export async function assertNoSymlinkAncestors(directory: string): Promise { + const api = process.platform === 'win32' ? path.win32 : path.posix + const root = api.parse(directory).root + let current = root + for (const part of directory.slice(root.length).split(api.sep).filter(Boolean)) { + current = api.join(current, part) + try { + const info = await lstat(current) + if (info.isSymbolicLink()) throw new Error('Storage path contains a symbolic link: ' + current) + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') break + throw error + } + } +} + +export async function assertEmptyDestination(directory: string): Promise { + try { + if ((await readdir(directory)).length > 0) { + throw new Error('Destination must be empty to avoid overwriting existing files') + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return + throw error + } +} + +/** Main-process IPC handlers must use guarded operations, never raw rm/cp. */ +export async function deleteStorageDirectory( + directory: string, dirs: StorageDirs, userData: string, options: Options = {}, +): Promise { + const safe = validateStorageDeletion(directory, dirs, userData, options) + await assertNoSymlinkAncestors(safe) + await rm(safe, { recursive: true, force: true }) +} + +export async function moveStorageDirectory( + source: string, destination: string, dirs: StorageDirs, userData: string, options: Options = {}, +): Promise { + const paths = validateStorageMove(source, destination, dirs, userData, options) + await assertNoSymlinkAncestors(paths.src) + await assertNoSymlinkAncestors(paths.dest) + await assertEmptyDestination(paths.dest) + await mkdir(paths.dest, { recursive: true }) + // Refuse races that add files to the destination after the emptiness check. + await cp(paths.src, paths.dest, { recursive: true, force: false, errorOnExist: true }) + // Re-validate before deleting the source. Never delete it after a copy error. + validateStorageMove(paths.src, paths.dest, dirs, userData, options) + await assertNoSymlinkAncestors(paths.src) + await rm(paths.src, { recursive: true, force: true }) +} diff --git a/package.json b/package.json index 8c3a985b..06d82a3b 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "prepare-resources": "node scripts/download-python-embed.js", "test": "npm run test:py && npm run test:node", "test:py": "node scripts/run-pytests.mjs", - "test:node": "node --test --experimental-strip-types --experimental-loader ./scripts/node-ts-extensionless-loader.mjs src/shared/types/assetLibrary.test.ts src/areas/generate/assetLibraryProjection.test.ts src/areas/generate/assetLibraryService.test.ts src/areas/generate/assetLibraryUi.test.ts src/areas/generate/orcaSlicerLink.test.ts electron/main/artifact-registry-service.test.ts electron/main/extension-path-guard.test.ts electron/preload/artifact-registry-preload.test.ts && node --test electron/main/*.test.mjs src/**/*.test.mjs", + "test:node": "node --test --experimental-strip-types --experimental-loader ./scripts/node-ts-extensionless-loader.mjs src/shared/types/assetLibrary.test.ts src/areas/generate/assetLibraryProjection.test.ts src/areas/generate/assetLibraryService.test.ts src/areas/generate/assetLibraryUi.test.ts src/areas/generate/orcaSlicerLink.test.ts electron/main/artifact-registry-service.test.ts electron/main/extension-path-guard.test.ts electron/main/storage-directory-guard.test.ts electron/preload/artifact-registry-preload.test.ts && node --test electron/main/*.test.mjs src/**/*.test.mjs", "package": "cross-env CSC_IDENTITY_AUTO_DISCOVERY=false npm run build && npm run prepare-resources && electron-builder", "package:mac": "cross-env CSC_IDENTITY_AUTO_DISCOVERY=false npm run build && npm run prepare-resources && electron-builder --mac --arm64", "lint": "eslint ." From fb87f0435f5827a5d564fbb7e0b73e99c443288d Mon Sep 17 00:00:00 2001 From: JayceeB1 <45721154+JayceeB1@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:16:29 +0200 Subject: [PATCH 2/7] integrate(#333): keep CLI exports intact on failed downloads Adapted from lightningpixel/modly#333; unmodified context preserved. --- tools/modly-cli/agent.py | 18 ++++++++-- tools/modly-cli/test_agent.py | 63 ++++++++++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/tools/modly-cli/agent.py b/tools/modly-cli/agent.py index e424aa25..c7f37248 100644 --- a/tools/modly-cli/agent.py +++ b/tools/modly-cli/agent.py @@ -86,16 +86,25 @@ def _request_json( def _download(url: str, dest: Path, *, timeout: float) -> int: - dest.parent.mkdir(parents=True, exist_ok=True) + temporary_path: Path | None = None try: - with urllib.request.urlopen(url, timeout=timeout) as resp, dest.open("wb") as fh: + dest.parent.mkdir(parents=True, exist_ok=True) + # Keep the previous export intact until the download has finished. + # A sibling temporary file allows replacement on the same filesystem. + with urllib.request.urlopen(url, timeout=timeout) as resp, tempfile.NamedTemporaryFile( + dir=dest.parent, prefix=".modly-download-", suffix=".tmp", delete=False + ) as fh: + temporary_path = Path(fh.name) total = 0 while True: chunk = resp.read(1024 * 1024) if not chunk: - return total + break fh.write(chunk) total += len(chunk) + # Close the temporary file before replacing it, including on Windows. + os.replace(temporary_path, dest) + return total except urllib.error.HTTPError as exc: detail = exc.read().decode("utf-8", errors="replace") raise ModlyCliError(f"HTTP {exc.code} while downloading {url}: {detail}", code=f"HTTP_{exc.code}", http_status=exc.code) from exc @@ -103,6 +112,9 @@ def _download(url: str, dest: Path, *, timeout: float) -> int: raise ModlyCliError(f"Cannot download {url}: {exc.reason}", code="DOWNLOAD_FAILED") from exc except OSError as exc: raise ModlyCliError(f"Cannot write to {dest}: {exc}", code="WRITE_FAILED") from exc + finally: + if temporary_path is not None: + temporary_path.unlink(missing_ok=True) def _multipart_form(fields: dict[str, str], file_field: str, file_path: Path) -> tuple[bytes, str]: diff --git a/tools/modly-cli/test_agent.py b/tools/modly-cli/test_agent.py index 9537ec1a..5598d3a0 100644 --- a/tools/modly-cli/test_agent.py +++ b/tools/modly-cli/test_agent.py @@ -11,7 +11,7 @@ from contextlib import redirect_stdout from pathlib import Path from types import SimpleNamespace -from unittest.mock import patch +from unittest.mock import MagicMock, patch MODULE_PATH = Path(__file__).with_name("agent.py") SPEC = importlib.util.spec_from_file_location("modly_agent", MODULE_PATH) @@ -28,6 +28,67 @@ def test_compact_json_is_one_line(self) -> None: self.assertEqual(buf.getvalue(), '{"nested":{"x":1},"ok":true}\n') +class DownloadTests(unittest.TestCase): + def test_success_replaces_destination_only_after_download(self) -> None: + with tempfile.TemporaryDirectory() as td: + dest = Path(td) / "mesh.glb" + dest.write_bytes(b"original") + chunks = iter([b"new ", b"mesh", b""]) + + def read(_size: int) -> bytes: + self.assertEqual(dest.read_bytes(), b"original") + return next(chunks) + + response = MagicMock() + response.__enter__.return_value.read.side_effect = read + with patch.object(agent.urllib.request, "urlopen", return_value=response): + self.assertEqual(agent._download("http://example.test/mesh", dest, timeout=1), 8) + self.assertEqual(dest.read_bytes(), b"new mesh") + self.assertEqual(list(Path(td).iterdir()), [dest]) + + def test_interrupted_download_preserves_destination_and_cleans_temporary_file(self) -> None: + for existing in (False, True): + for failure in (ConnectionResetError("connection lost"), KeyboardInterrupt()): + with self.subTest(existing=existing, failure=type(failure).__name__): + with tempfile.TemporaryDirectory() as td: + dest = Path(td) / "mesh.glb" + if existing: + dest.write_bytes(b"original") + response = MagicMock() + response.__enter__.return_value.read.side_effect = [b"partial", failure] + expected = KeyboardInterrupt if isinstance(failure, KeyboardInterrupt) else agent.ModlyCliError + with patch.object(agent.urllib.request, "urlopen", return_value=response): + with self.assertRaises(expected): + agent._download("http://example.test/mesh", dest, timeout=1) + if existing: + self.assertEqual(dest.read_bytes(), b"original") + else: + self.assertFalse(dest.exists()) + self.assertEqual(list(Path(td).iterdir()), [dest] if existing else []) + + def test_replace_failure_preserves_destination_and_cleans_temporary_file(self) -> None: + with tempfile.TemporaryDirectory() as td: + dest = Path(td) / "mesh.glb" + dest.write_bytes(b"original") + with ( + patch.object(agent.urllib.request, "urlopen", return_value=io.BytesIO(b"new mesh")), + patch.object(agent.os, "replace", side_effect=PermissionError("destination locked")), + ): + with self.assertRaises(agent.ModlyCliError) as ctx: + agent._download("http://example.test/mesh", dest, timeout=1) + self.assertEqual(ctx.exception.code, "WRITE_FAILED") + self.assertEqual(dest.read_bytes(), b"original") + self.assertEqual(list(Path(td).iterdir()), [dest]) + + def test_success_creates_destination_in_new_directory(self) -> None: + with tempfile.TemporaryDirectory() as td: + dest = Path(td) / "exports" / "mesh.glb" + with patch.object(agent.urllib.request, "urlopen", return_value=io.BytesIO(b"new mesh")): + self.assertEqual(agent._download("http://example.test/mesh", dest, timeout=1), 8) + self.assertEqual(dest.read_bytes(), b"new mesh") + self.assertEqual(list(dest.parent.iterdir()), [dest]) + + class CommandTests(unittest.TestCase): def test_status_combines_health_and_model(self) -> None: calls: list[tuple[str, str]] = [] From e01661cb85c97a607d4c808391f8a32feb0dd47a Mon Sep 17 00:00:00 2001 From: JayceeB1 <45721154+JayceeB1@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:16:48 +0200 Subject: [PATCH 3/7] integrate(#366): preserve active workers after completed runs Test-only contribution adapted from lightningpixel/modly#366. --- api/tests/test_workflow_runs_lifecycle.py | 159 ++++++++++++++++++++++ 1 file changed, 159 insertions(+) diff --git a/api/tests/test_workflow_runs_lifecycle.py b/api/tests/test_workflow_runs_lifecycle.py index 47433573..e8aa355c 100644 --- a/api/tests/test_workflow_runs_lifecycle.py +++ b/api/tests/test_workflow_runs_lifecycle.py @@ -200,5 +200,164 @@ async def run_cross_route_jobs(): self.assertEqual(fake_registry.active_id, "demo/generate") +class _FakeProc: + """Stands in for a loaded extension worker's subprocess.""" + + def __init__(self) -> None: + self.killed = False + + def poll(self): + return -9 if self.killed else None + + def kill(self) -> None: + self.killed = True + + +class _FakeWorker: + """A loaded generator with a live subprocess, as _run_generation binds it.""" + + def __init__(self) -> None: + self._proc = _FakeProc() + self._loaded = True + self.outputs_dir = None + + def generate(self, image_bytes, params, progress_cb, cancel_event=None): + output = Path(self.outputs_dir) / "result.glb" + output.write_bytes(b"glb") + return output + + +class _FailingWorker(_FakeWorker): + def generate(self, image_bytes, params, progress_cb, cancel_event=None): + raise RuntimeError("inference failed") + + +class _RegistryWithLoadedWorker: + """Keeps one worker loaded and active, the state the worker is left in + after a job ends normally.""" + + def __init__(self, worker: _FakeWorker) -> None: + self.worker = worker + self._generators = {"ext/model": worker} + self._active_id = "ext/model" + + def assert_weight_variant_installed(self, params, model_id) -> None: + pass + + def get_generator(self, model_id: str): + return self.worker + + def model_status(self, model_id: str) -> dict: + return {"name": model_id, "downloaded": True, "loaded": True} + + def activate_ready_generator(self, model_id: str): + return self.worker + + +class CancelEndedJobTests(unittest.TestCase): + """cancel_job kills the subprocess bound to the job in _job_generators so + inference stops at once. Once the job has ended, that binding is gone and + the worker holds the warm model (or another job's generation), so a late + cancel of the ended job/run must leave the worker alone.""" + + def setUp(self) -> None: + self._prev_generation_registry = generation.generator_registry + self._prev_runs_registry = workflow_runs.generator_registry + self._prev_workspace = registry_module.WORKSPACE_DIR + self._tmp = tempfile.TemporaryDirectory() + registry_module.WORKSPACE_DIR = Path(self._tmp.name) + _clear_job_stores() + + def tearDown(self) -> None: + generation.generator_registry = self._prev_generation_registry + workflow_runs.generator_registry = self._prev_runs_registry + registry_module.WORKSPACE_DIR = self._prev_workspace + self._tmp.cleanup() + _clear_job_stores() + + def _use_worker(self, worker: _FakeWorker) -> _FakeWorker: + registry = _RegistryWithLoadedWorker(worker) + generation.generator_registry = registry + workflow_runs.generator_registry = registry + return worker + + def _file_job(self, job_id: str, status: str = "pending") -> None: + generation._jobs[job_id] = JobStatus(job_id=job_id, status=status, progress=0) + generation._cancel_events[job_id] = threading.Event() + + def _run_to_end(self, job_id: str) -> None: + self._file_job(job_id) + asyncio.run(generation._run_generation(job_id, b"img", {}, "Default", "mesh", "ext/model")) + # The job's generator binding is released as soon as it ends. + self.assertNotIn(job_id, generation._job_generators) + + def _assert_worker_untouched(self, worker: _FakeWorker, proc: _FakeProc) -> None: + self.assertFalse(proc.killed) + self.assertIs(worker._proc, proc) + self.assertTrue(worker._loaded) + + def _assert_worker_stopped(self, worker: _FakeWorker, proc: _FakeProc) -> None: + self.assertTrue(proc.killed) + self.assertIsNone(worker._proc) + self.assertFalse(worker._loaded) + + def test_cancelling_a_finished_job_leaves_the_active_worker_alone(self) -> None: + worker = self._use_worker(_FakeWorker()) + proc = worker._proc + self._run_to_end("finished-job") + self.assertEqual(generation._jobs["finished-job"].status, "done") + + asyncio.run(generation.cancel_job("finished-job")) + + self.assertEqual(generation._jobs["finished-job"].status, "done") + self._assert_worker_untouched(worker, proc) + + def test_cancelling_a_failed_run_leaves_the_active_worker_alone(self) -> None: + worker = self._use_worker(_FailingWorker()) + proc = worker._proc + self._run_to_end("failed-run") + self.assertEqual(generation._jobs["failed-run"].status, "error") + + asyncio.run(workflow_runs.cancel_run("failed-run")) + + self.assertEqual(generation._jobs["failed-run"].status, "error") + self._assert_worker_untouched(worker, proc) + + def test_cancelling_an_ended_job_leaves_another_jobs_worker_alone(self) -> None: + worker = self._use_worker(_FakeWorker()) + proc = worker._proc + self._run_to_end("finished-job") + # A later job now owns the same worker. + self._file_job("running-job", "running") + generation._job_generators["running-job"] = worker + + asyncio.run(generation.cancel_job("finished-job")) + + self.assertEqual(generation._jobs["running-job"].status, "running") + self._assert_worker_untouched(worker, proc) + + def test_cancelling_a_running_job_still_stops_the_worker(self) -> None: + worker = self._use_worker(_FakeWorker()) + proc = worker._proc + self._file_job("running-job", "running") + generation._job_generators["running-job"] = worker + + asyncio.run(generation.cancel_job("running-job")) + + self.assertEqual(generation._jobs["running-job"].status, "cancelled") + self._assert_worker_stopped(worker, proc) + + def test_cancelling_a_running_run_still_stops_the_worker(self) -> None: + worker = self._use_worker(_FakeWorker()) + proc = worker._proc + self._file_job("running-run", "running") + generation._job_generators["running-run"] = worker + + asyncio.run(workflow_runs.cancel_run("running-run")) + + self.assertEqual(generation._jobs["running-run"].status, "cancelled") + self._assert_worker_stopped(worker, proc) + + if __name__ == "__main__": unittest.main() From 3315db4e29e584b67d0ebd4f1c09841e28f5c907 Mon Sep 17 00:00:00 2001 From: JayceeB1 <45721154+JayceeB1@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:17:08 +0200 Subject: [PATCH 4/7] integrate(#364): export live transformed 3D meshes Adapted from lightningpixel/modly#364 for experimental validation. --- src/areas/generate/GeneratePage.tsx | 61 +++++++-- src/areas/generate/components/Viewer3D.tsx | 145 ++++++++++++++++++++- 2 files changed, 189 insertions(+), 17 deletions(-) diff --git a/src/areas/generate/GeneratePage.tsx b/src/areas/generate/GeneratePage.tsx index 76a13119..073ea487 100644 --- a/src/areas/generate/GeneratePage.tsx +++ b/src/areas/generate/GeneratePage.tsx @@ -5,7 +5,7 @@ import type { GenerationJob, LightSettings, PointLight } from '@shared/stores/ap import { useApi } from '@shared/hooks/useApi' import { ColorPicker } from '@shared/components/ui' import GenerationHUD from './components/GenerationHUD' -import Viewer3D from './components/Viewer3D' +import Viewer3D, { type MeshExportFormat, type MeshExportHandler } from './components/Viewer3D' import WorkflowPanel from './components/WorkflowPanel' import { getDefaultAssetLibraryService } from './assetLibraryService' import { buildOrcaSlicerDeepLink, canOpenInOrcaSlicer } from './orcaSlicerLink' @@ -680,6 +680,8 @@ export default function GeneratePage(): JSX.Element { const [selectedPointLightId, setSelectedPointLightId] = useState(null) // Populated by Viewer3D — undoes the latest live gizmo transform, if any. const gizmoUndoRef = useRef<(() => boolean) | null>(null) + const gizmoResetRef = useRef<(() => boolean) | null>(null) + const meshExportRef = useRef(null) const lightSettings = useAppStore((s) => s.lightSettings) const setLightSettings = useAppStore((s) => s.setLightSettings) @@ -762,18 +764,38 @@ export default function GeneratePage(): JSX.Element { setTimeout(() => setUnloadStatus('idle'), 2000) } - function handleExport(format: 'glb' | 'obj' | 'stl' | 'ply') { + async function handleExport(format: MeshExportFormat) { if (!currentJob?.outputUrl) return const stem = `modly-${Date.now()}` const link = document.createElement('a') - if (format === 'glb') { - link.href = `${apiUrl}${currentJob.outputUrl}` - } else { - const path = encodeURIComponent(currentJob.outputUrl.replace('/workspace/', '')) - link.href = `${apiUrl}/optimize/export?path=${path}&format=${format}` + + try { + const transformedMesh = await meshExportRef.current?.(format) + if (transformedMesh) { + const objectUrl = URL.createObjectURL(transformedMesh) + link.href = objectUrl + link.download = `${stem}.${format}` + link.click() + window.setTimeout(() => URL.revokeObjectURL(objectUrl), 1000) + return + } + + // Gaussian splats are not mesh objects and continue through the existing + // source-file export path. + if (!/\.(ply|splat)(?:$|[?#])/i.test(currentJob.outputUrl)) { + throw new Error('The mesh viewer is still loading. Try exporting again once it appears.') + } + if (format === 'glb') { + link.href = `${apiUrl}${currentJob.outputUrl}` + } else { + const path = encodeURIComponent(currentJob.outputUrl.replace('/workspace/', '')) + link.href = `${apiUrl}/optimize/export?path=${path}&format=${format}` + } + link.download = `${stem}.${format}` + link.click() + } catch (err) { + showError(err instanceof Error ? err.message : String(err)) } - link.download = `${stem}.${format}` - link.click() } async function handleOpenInOrcaSlicer() { @@ -1100,7 +1122,7 @@ export default function GeneratePage(): JSX.Element { {openPanel === 'export' && ( void} + onExport={handleExport} onClose={() => setOpenPanel(null)} onOpenInSlicer={() => { void handleOpenInOrcaSlicer() }} canOpenInSlicer={showOpenInSlicer} @@ -1255,6 +1277,23 @@ export default function GeneratePage(): JSX.Element { + {meshSelected && ( + <> +