From 65728aa39d9ab6910a1301350297a68cf16b6268 Mon Sep 17 00:00:00 2001 From: stack72 Date: Thu, 26 Mar 2026 15:48:58 +0100 Subject: [PATCH] fix: surface extension loading errors and improve method extraction (#875) Extension loading errors (model validation failures, bundling errors) were silently swallowed because loadUserModels runs before logging is initialized during CLI startup. Users saw only "type not found" with no explanation of why their model failed to load. - Defer extension loading warnings and emit them after logging init - Fix extractMethods to handle shorthand (`methods,`) and variable reference (`methods: importedVar`) patterns in content metadata extraction - Pass only entry point files to extractContentMetadata, not companion files Closes #875 Co-Authored-By: Claude Opus 4.6 (1M context) --- src/cli/mod.ts | 119 +++++++++--------- .../extensions/extension_content_extractor.ts | 91 +++++++++++++- .../extension_content_extractor_test.ts | 82 ++++++++++++ src/libswamp/extensions/push.ts | 2 +- 4 files changed, 231 insertions(+), 63 deletions(-) diff --git a/src/cli/mod.ts b/src/cli/mod.ts index 62fe44f5..9f176947 100644 --- a/src/cli/mod.ts +++ b/src/cli/mod.ts @@ -191,6 +191,13 @@ export function commandNeedsExtensions(args: string[]): boolean { return !SKIP_EXTENSION_COMMANDS.has(commandInfo.command); } +/** A deferred warning message to emit after logging is initialized. */ +export interface DeferredWarning { + kind: "model" | "vault" | "driver" | "datastore" | "report"; + file: string; + error: string; +} + /** * Load user models from configured directory. */ @@ -198,6 +205,7 @@ async function loadUserModels( repoDir: string, marker: RepoMarkerData | null, denoRuntime: EmbeddedDenoRuntime, + deferredWarnings: DeferredWarning[], ): Promise { try { const modelsDir = resolveModelsDir(marker); @@ -209,18 +217,16 @@ async function loadUserModels( const loader = new UserModelLoader(denoRuntime, repoDir); const result = await loader.loadModels(absoluteModelsDir); - // Log extension successes at debug level - for (const file of result.extended) { - logger.debug`Extended model type from ${file}`; - } - - // Log failures as warnings (don't block CLI startup) + // Collect failures for deferred logging (logging not yet initialized) for (const failure of result.failed) { - logger.warn`Failed to load user model ${failure.file}: ${failure.error}`; + deferredWarnings.push({ + kind: "model", + file: failure.file, + error: failure.error, + }); } - } catch (error) { - // Not in a swamp repo or other error - log at debug level for troubleshooting - logger.debug`Skipping user models: ${error}`; + } catch { + // Not in a swamp repo or models dir doesn't exist — not an error } } @@ -231,6 +237,7 @@ async function loadUserVaults( repoDir: string, marker: RepoMarkerData | null, denoRuntime: EmbeddedDenoRuntime, + deferredWarnings: DeferredWarning[], ): Promise { try { const vaultsDir = resolveVaultsDir(marker); @@ -241,18 +248,15 @@ async function loadUserVaults( const loader = new UserVaultLoader(denoRuntime, repoDir); const result = await loader.loadVaults(absoluteVaultsDir); - // Log successes at debug level - for (const file of result.loaded) { - logger.debug`Loaded user vault type from ${file}`; - } - - // Log failures as warnings (don't block CLI startup) for (const failure of result.failed) { - logger.warn`Failed to load user vault ${failure.file}: ${failure.error}`; + deferredWarnings.push({ + kind: "vault", + file: failure.file, + error: failure.error, + }); } - } catch (error) { - // Not in a swamp repo or other error - log at debug level for troubleshooting - logger.debug`Skipping user vaults: ${error}`; + } catch { + // Not in a swamp repo or vaults dir doesn't exist — not an error } } @@ -260,6 +264,7 @@ async function loadUserDrivers( repoDir: string, marker: RepoMarkerData | null, denoRuntime: EmbeddedDenoRuntime, + deferredWarnings: DeferredWarning[], ): Promise { try { const driversDir = resolveDriversDir(marker); @@ -270,18 +275,15 @@ async function loadUserDrivers( const loader = new UserDriverLoader(denoRuntime, repoDir); const result = await loader.loadDrivers(absoluteDriversDir); - // Log successes at debug level - for (const file of result.loaded) { - logger.debug`Loaded user driver type from ${file}`; - } - - // Log failures as warnings (don't block CLI startup) for (const failure of result.failed) { - logger.warn`Failed to load user driver ${failure.file}: ${failure.error}`; + deferredWarnings.push({ + kind: "driver", + file: failure.file, + error: failure.error, + }); } - } catch (error) { - // Not in a swamp repo or other error - log at debug level for troubleshooting - logger.debug`Skipping user drivers: ${error}`; + } catch { + // Not in a swamp repo or drivers dir doesn't exist — not an error } } @@ -289,6 +291,7 @@ async function loadUserDatastores( repoDir: string, marker: RepoMarkerData | null, denoRuntime: EmbeddedDenoRuntime, + deferredWarnings: DeferredWarning[], ): Promise { try { const datastoresDir = resolveDatastoresDir(marker); @@ -299,19 +302,15 @@ async function loadUserDatastores( const loader = new UserDatastoreLoader(denoRuntime, repoDir); const result = await loader.loadDatastores(absoluteDatastoresDir); - // Log successes at debug level - for (const file of result.loaded) { - logger.debug`Loaded user datastore type from ${file}`; - } - - // Log failures as warnings (don't block CLI startup) for (const failure of result.failed) { - logger - .warn`Failed to load user datastore ${failure.file}: ${failure.error}`; + deferredWarnings.push({ + kind: "datastore", + file: failure.file, + error: failure.error, + }); } - } catch (error) { - // Not in a swamp repo or other error - log at debug level for troubleshooting - logger.debug`Skipping user datastores: ${error}`; + } catch { + // Not in a swamp repo or datastores dir doesn't exist — not an error } } @@ -319,6 +318,7 @@ async function loadUserReports( repoDir: string, marker: RepoMarkerData | null, denoRuntime: EmbeddedDenoRuntime, + deferredWarnings: DeferredWarning[], ): Promise { try { const reportsDir = resolveReportsDir(marker); @@ -329,18 +329,15 @@ async function loadUserReports( const loader = new UserReportLoader(denoRuntime, repoDir); const result = await loader.loadReports(absoluteReportsDir); - // Log successes at debug level - for (const file of result.loaded) { - logger.debug`Loaded user report from ${file}`; - } - - // Log failures as warnings (don't block CLI startup) for (const failure of result.failed) { - logger.warn`Failed to load user report ${failure.file}: ${failure.error}`; + deferredWarnings.push({ + kind: "report", + file: failure.file, + error: failure.error, + }); } - } catch (error) { - // Not in a swamp repo or other error - log at debug level for troubleshooting - logger.debug`Skipping user reports: ${error}`; + } catch { + // Not in a swamp repo or reports dir doesn't exist — not an error } } @@ -496,15 +493,17 @@ export async function runCli(args: string[]): Promise { // Not in a swamp repo - marker stays null } - // Load user extensions in parallel (skip for commands that don't need them) + // Load user extensions in parallel (skip for commands that don't need them). + // Collect warnings because logging is not yet initialized at this point. + const deferredWarnings: DeferredWarning[] = []; if (commandNeedsExtensions(args)) { const denoRuntime = new EmbeddedDenoRuntime(); await Promise.all([ - loadUserModels(repoDir, marker, denoRuntime), - loadUserVaults(repoDir, marker, denoRuntime), - loadUserDrivers(repoDir, marker, denoRuntime), - loadUserDatastores(repoDir, marker, denoRuntime), - loadUserReports(repoDir, marker, denoRuntime), + loadUserModels(repoDir, marker, denoRuntime, deferredWarnings), + loadUserVaults(repoDir, marker, denoRuntime, deferredWarnings), + loadUserDrivers(repoDir, marker, denoRuntime, deferredWarnings), + loadUserDatastores(repoDir, marker, denoRuntime, deferredWarnings), + loadUserReports(repoDir, marker, denoRuntime, deferredWarnings), ]); } @@ -602,6 +601,12 @@ export async function runCli(args: string[]): Promise { jsonMode: options.json ?? false, noColor, }); + + // Emit deferred warnings now that logging is initialized + for (const warning of deferredWarnings) { + logger + .warn`Failed to load user ${warning.kind} ${warning.file}: ${warning.error}`; + } }) .error(unknownCommandErrorHandler) .action(function () { diff --git a/src/domain/extensions/extension_content_extractor.ts b/src/domain/extensions/extension_content_extractor.ts index 5e58c669..c6d137d3 100644 --- a/src/domain/extensions/extension_content_extractor.ts +++ b/src/domain/extensions/extension_content_extractor.ts @@ -325,19 +325,45 @@ function extractVaultFromSource( /** * Extracts method definitions from the source. - * Matches the `methods: { name: { description: "..." } }` pattern. + * Handles three patterns: + * 1. Inline: `methods: { name: { description: "..." } }` + * 2. Shorthand property: `methods,` (references a variable named `methods`) + * 3. Variable reference: `methods: someVar` (references a named variable) */ function extractMethods(content: string): ExtractedMethod[] { + // Try inline methods block first: methods: { ... } + const inlineMethods = extractMethodsFromBlock(content, content); + if (inlineMethods.length > 0) return inlineMethods; + + // Try variable reference: methods: someVar or shorthand methods, + const refName = resolveMethodsReference(content); + if (!refName) return []; + + // Look up the referenced variable in the same file + const varBlock = findVariableObjectBody(content, refName); + if (varBlock) { + return extractMethodsFromBlock(varBlock, content); + } + + return []; +} + +/** + * Extracts methods from an inline `methods: { ... }` block within the content. + */ +function extractMethodsFromBlock( + searchContent: string, + fullContent: string, +): ExtractedMethod[] { const methods: ExtractedMethod[] = []; - // Find the methods block - const methodsBlockMatch = content.match(/methods:\s*\{/); + const methodsBlockMatch = searchContent.match(/methods:\s*\{/); if (!methodsBlockMatch || methodsBlockMatch.index === undefined) { return methods; } const methodsStart = methodsBlockMatch.index + methodsBlockMatch[0].length; - const methodsBlock = extractBalancedBraces(content, methodsStart); + const methodsBlock = extractBalancedBraces(searchContent, methodsStart); if (!methodsBlock) return methods; // Match individual method entries: methodName: { description: "..." } @@ -351,7 +377,7 @@ function extractMethods(content: string): ExtractedMethod[] { // Try to extract arguments for this method const methodEntry = extractMethodEntry(methodsBlock, name); const args = methodEntry - ? extractMethodArguments(methodEntry, content) + ? extractMethodArguments(methodEntry, fullContent) : []; methods.push({ name, description, arguments: args }); @@ -360,6 +386,61 @@ function extractMethods(content: string): ExtractedMethod[] { return methods; } +/** + * Resolves the variable name referenced by the `methods` property. + * Returns the variable name for `methods: someVar` or `methods` (shorthand). + * Returns null if methods are defined inline or not found. + */ +function resolveMethodsReference(content: string): string | null { + // Match shorthand property: methods, or methods } (end of object) + // This pattern appears when an imported variable named `methods` is used + // as a shorthand property in the model object literal. + const shorthandMatch = content.match( + /(?:export\s+const\s+(?:model|extension)\s*=\s*\{[\s\S]*?)(? }` so the method extraction regex works + return `methods: {${body}}`; +} + /** * Extracts the full text of a single method entry from the methods block. */ diff --git a/src/domain/extensions/extension_content_extractor_test.ts b/src/domain/extensions/extension_content_extractor_test.ts index 83602824..ebc939f5 100644 --- a/src/domain/extensions/extension_content_extractor_test.ts +++ b/src/domain/extensions/extension_content_extractor_test.ts @@ -1389,3 +1389,85 @@ Deno.test("extractContentMetadata skips datastore without type", async () => { await Deno.remove(tmpDir, { recursive: true }); } }); + +Deno.test("extractContentMetadata: extracts methods from shorthand property", async () => { + const tmpDir = await Deno.makeTempDir(); + try { + const modelsDir = join(tmpDir, "models"); + await Deno.mkdir(modelsDir, { recursive: true }); + + const modelFile = join(modelsDir, "mymodel.ts"); + await Deno.writeTextFile( + modelFile, + [ + 'import { z } from "npm:zod@4";', + "const methods = {", + " list: {", + ' description: "List resources",', + " arguments: z.object({}),", + " execute: async () => ({ dataHandles: [] }),", + " },", + " create: {", + ' description: "Create a resource",', + " arguments: z.object({}),", + " execute: async () => ({ dataHandles: [] }),", + " },", + "};", + "export const model = {", + ' type: "@test/mymodel",', + ' version: "2026.03.26.1",', + " methods,", + "};", + ].join("\n"), + ); + + const result = await extractContentMetadata([modelFile], modelsDir, []); + assertEquals(result.models.length, 1); + assertEquals(result.models[0].methods.length, 2); + assertEquals(result.models[0].methods[0].name, "list"); + assertEquals(result.models[0].methods[0].description, "List resources"); + assertEquals(result.models[0].methods[1].name, "create"); + assertEquals( + result.models[0].methods[1].description, + "Create a resource", + ); + } finally { + await Deno.remove(tmpDir, { recursive: true }); + } +}); + +Deno.test("extractContentMetadata: extracts methods from variable reference", async () => { + const tmpDir = await Deno.makeTempDir(); + try { + const modelsDir = join(tmpDir, "models"); + await Deno.mkdir(modelsDir, { recursive: true }); + + const modelFile = join(modelsDir, "refmodel.ts"); + await Deno.writeTextFile( + modelFile, + [ + 'import { z } from "npm:zod@4";', + "const myMethods = {", + " sync: {", + ' description: "Sync data",', + " arguments: z.object({}),", + " execute: async () => ({ dataHandles: [] }),", + " },", + "};", + "export const model = {", + ' type: "@test/refmodel",', + ' version: "2026.03.26.1",', + " methods: myMethods,", + "};", + ].join("\n"), + ); + + const result = await extractContentMetadata([modelFile], modelsDir, []); + assertEquals(result.models.length, 1); + assertEquals(result.models[0].methods.length, 1); + assertEquals(result.models[0].methods[0].name, "sync"); + assertEquals(result.models[0].methods[0].description, "Sync data"); + } finally { + await Deno.remove(tmpDir, { recursive: true }); + } +}); diff --git a/src/libswamp/extensions/push.ts b/src/libswamp/extensions/push.ts index 5c88d891..e585d1d4 100644 --- a/src/libswamp/extensions/push.ts +++ b/src/libswamp/extensions/push.ts @@ -418,7 +418,7 @@ export async function extensionPushPrepare( let contentMetadata: ExtensionContentMetadata | undefined; try { contentMetadata = await deps.extractContentMetadata( - input.allModelFiles, + input.modelEntryPoints, input.modelsDir, input.workflowFiles, input.allVaultFiles,