From 1907b36df6eb4edeff5ea448c8f1f5d3f5b99831 Mon Sep 17 00:00:00 2001 From: stack72 Date: Sat, 11 Apr 2026 21:42:31 +0100 Subject: [PATCH] fix: promote lazy-loaded reports in executeReports() before filtering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #81. Regression from #1089 where workflow-scope (and method/model scope) user extension reports silently failed to execute on the second and subsequent runs after the extension catalog was populated. executeReports() calls registry.getAll(), which returns only fully-loaded entries from the reports Map. User reports registered lazily from the bundle catalog live in lazyTypes and were silently dropped by filterReports() before their bundles were ever imported. Builtin reports like @swamp/workflow-summary kept working because they are registered eagerly in builtin/mod.ts. Fix: in executeReports(), before calling registry.getAll(), build a deduped set of candidate names from selection.require (via getReportRefName() to handle the ReportRef | string union) and modelTypeReports, then await Promise.all(registry.ensureTypeLoaded(name)) for each. This promotes matching lazy entries to fully loaded so they pass through filterReports(). One central fix covers all four executeReports() call sites (workflow scope, method scope, model scope, and failed-method summary). Errors from ensureTypeLoaded() propagate unchanged — a broken bundle for a required report fails loudly rather than being silently skipped. Promotion is unconditional for every candidate name (we cannot inspect report.scope until the bundle is imported), which may waste one import on scope-mismatched reports; this is bounded by the candidate set size and typeLoadPromises dedupes concurrent callers. Also updates design/reports.md "Report Registry" section to describe the post-#1089 two-state model (fully loaded vs lazy), ensureTypeLoaded, setLoader/setTypeLoader, and the promotion contract that domain services must honor before iterating via getAll(). Verified end-to-end against a minimal reproduction: a workflow requiring a workflow-scope user extension report now runs the report on both the first (catalog bootstrap) and second (catalog populated) invocations. --- design/reports.md | 42 +++- .../reports/report_execution_service.ts | 32 +++ .../reports/report_execution_service_test.ts | 212 ++++++++++++++++++ 3 files changed, 284 insertions(+), 2 deletions(-) diff --git a/design/reports.md b/design/reports.md index bae340c6..9b0c106d 100644 --- a/design/reports.md +++ b/design/reports.md @@ -152,7 +152,45 @@ See `src/domain/reports/user_report_loader.ts`. ## Report Registry The `ReportRegistry` is a `Map`-backed registry of report definitions keyed by -name. It provides `register`, `get`, `getAll`, `getByScope`, and `has` methods. +name. Every report type exists in one of two states: + +- **Fully loaded** — the bundle has been imported and the `ReportDefinition` + (including its `execute` function) is available in the internal `reports` + map. `register`, `get`, `getAll`, `getByScope`, and `has` all operate on + fully-loaded entries. +- **Lazy** — the type is known to exist from the extension bundle catalog, + but its bundle has not been imported yet. Lazy entries live in a separate + `lazyTypes` map and are materialized from the on-disk catalog on second + and subsequent process starts without touching the bundle files. + `registerLazy`, `isLazy`, `getAllLazy`, and the `LazyReportEntry` type + describe this state. + +`ensureTypeLoaded(name)` promotes a single lazy entry to fully loaded by +importing its bundle on demand and invoking `promoteFromLazy`. Concurrent +callers for the same type share a single in-flight promise via an internal +`typeLoadPromises` map, so a burst of promotions still triggers at most one +bundle import per type. `ensureTypeLoaded` is a no-op for types that are +already loaded or not registered at all. + +The CLI wires two hooks into the registry at startup (see `src/cli/mod.ts`): + +- `setLoader` — a full eager-load fallback that walks the reports directory + and imports every bundle. Triggered by `ensureLoaded()` and used when no + catalog is available. +- `setTypeLoader` — a per-type loader that imports a single bundle via the + catalog entry for that type. Backs `ensureTypeLoaded` in normal operation. + +**Promotion contract for iteration.** Because `getAll()` returns only +fully-loaded entries, any domain service that iterates the registry (most +notably `executeReports` in `report_execution_service.ts`) must first call +`ensureTypeLoaded` for every candidate report name — typically the union of +`selection.require` and the model type's declared report defaults — before +calling `getAll()` and filtering the result. Skipping this promotion step +causes lazy user-extension reports to be silently filtered out of the +applicable set on the second and subsequent process runs (the catalog is +populated, so the fully-loaded map contains only eagerly-registered builtin +reports). Iteration without promotion is the regression fixed by issue #81 +after the lazy-loading rework in #1089. The global singleton uses `globalThis` so the same registry is shared across module boundaries (important when extensions are loaded outside the bundle): @@ -164,7 +202,7 @@ export const reportRegistry: ReportRegistry = ``` Duplicate name registration throws an error. See -`src/domain/reports/report_registry.ts`. +`src/domain/reports/report_registry.ts` for the full API. ## Three-Level Control Model diff --git a/src/domain/reports/report_execution_service.ts b/src/domain/reports/report_execution_service.ts index bd95bd80..e86a322c 100644 --- a/src/domain/reports/report_execution_service.ts +++ b/src/domain/reports/report_execution_service.ts @@ -332,6 +332,38 @@ export async function executeReports( modelTypeReports?: string[], varySuffix?: string, ): Promise { + // Promote lazy-registered reports for every candidate name before calling + // getAll(). getAll() only returns fully-loaded entries from the registry's + // `reports` Map; lazy entries in `lazyTypes` are excluded. Without this + // step, user extension reports registered lazily from the bundle catalog + // (second+ process run) would be silently filtered out of the applicable + // set — the regression fixed by #81 after the lazy loading rework in + // #1089. + // + // We promote every candidate name unconditionally, including ones whose + // scope will end up mismatched by filterReports() below. We cannot inspect + // `report.scope` until the bundle is imported, and extending + // LazyReportEntry with scope would duplicate the bundle catalog and + // defeat the point of lazy loading for scope-mismatched reports anyway. + // Wasted imports are bounded by |selection.require ∪ modelTypeReports| + // and only happen once per process — ensureTypeLoaded() dedupes concurrent + // callers via typeLoadPromises, and is a no-op for already-loaded and + // unknown names. Errors from ensureTypeLoaded() propagate unchanged: a + // broken bundle for a required report must fail loudly rather than be + // silently skipped. + const candidateNames = new Set(); + for (const ref of selection?.require ?? []) { + candidateNames.add(getReportRefName(ref)); + } + if (modelTypeReports) { + for (const name of modelTypeReports) { + candidateNames.add(name); + } + } + await Promise.all( + Array.from(candidateNames, (name) => registry.ensureTypeLoaded(name)), + ); + const allReports = registry.getAll(); const applicable = filterReports( allReports, diff --git a/src/domain/reports/report_execution_service_test.ts b/src/domain/reports/report_execution_service_test.ts index 821497c3..1fbb185c 100644 --- a/src/domain/reports/report_execution_service_test.ts +++ b/src/domain/reports/report_execution_service_test.ts @@ -1077,3 +1077,215 @@ Deno.test("buildRedactSensitiveArgs: returns args unchanged for workflow scope", assertEquals(capturedResult !== null, true); assertEquals(capturedResult!.apiKey, "sk-secret"); }); + +// --- executeReports lazy-report promotion tests (regression: #81) --- + +/** + * Builds a minimal MethodReportContext for lazy-promotion tests. + */ +function makeMethodContext( + repo: ReturnType["repo"], + modelType: ModelType, +): MethodReportContext { + return { + scope: "method", + repoDir: "/tmp/test", + logger: { + debug: () => {}, + info: () => {}, + warn: () => {}, + error: () => {}, + fatal: () => {}, + } as unknown as MethodReportContext["logger"], + // deno-lint-ignore no-explicit-any + dataRepository: repo as any, + // deno-lint-ignore no-explicit-any + definitionRepository: {} as any, + modelType, + modelId: "test-id", + definition: { id: "test-id", name: "test", version: 1, tags: {} }, + globalArgs: {}, + methodArgs: {}, + methodName: "run", + executionStatus: "succeeded", + dataHandles: [], + }; +} + +/** + * Creates a fresh ReportRegistry with a single lazy entry and a type loader + * that promotes it on demand. Returns the registry and a counter for the + * number of times the loader was invoked. + */ +function createRegistryWithLazyReport( + typeName: string, + scope: "method" | "model" | "workflow" = "method", +): { registry: ReportRegistry; loaderCallCount: { value: number } } { + const registry = new ReportRegistry(); + registry.registerLazy({ + type: typeName, + bundlePath: `/tmp/fake-bundles/${typeName}.js`, + sourcePath: `/tmp/fake-sources/${typeName}.ts`, + version: "2026.04.11.1", + }); + + const loaderCallCount = { value: 0 }; + registry.setTypeLoader((type) => { + loaderCallCount.value++; + registry.promoteFromLazy(type, makeReport(scope)); + return Promise.resolve(); + }); + + return { registry, loaderCallCount }; +} + +Deno.test("executeReports: promotes lazy report named in selection.require", async () => { + const { registry, loaderCallCount } = createRegistryWithLazyReport( + "@test/lazy-method", + ); + const { repo, saved } = createInMemoryDataRepo(); + const modelType = ModelType.create("test/model"); + const context = makeMethodContext(repo, modelType); + + const summary = await executeReports( + registry, + context, + modelType, + "test-id", + { require: ["@test/lazy-method"] }, + {}, + undefined, + "run", + undefined, + ); + + assertEquals(loaderCallCount.value, 1); + assertEquals(summary.failures, 0); + assertEquals(summary.results.length, 1); + assertEquals(summary.results[0].name, "@test/lazy-method"); + assertEquals(summary.results[0].success, true); + // Persisted two artifacts (markdown + json) + assertEquals(saved.length, 2); + assertStringIncludes(saved[0].name, "test-lazy-method"); +}); + +Deno.test("executeReports: promotes lazy report from modelTypeReports defaults", async () => { + const { registry, loaderCallCount } = createRegistryWithLazyReport( + "@test/lazy-default", + ); + const { repo } = createInMemoryDataRepo(); + const modelType = ModelType.create("test/model"); + const context = makeMethodContext(repo, modelType); + + const summary = await executeReports( + registry, + context, + modelType, + "test-id", + undefined, + {}, + undefined, + "run", + ["@test/lazy-default"], + ); + + assertEquals(loaderCallCount.value, 1); + assertEquals(summary.failures, 0); + assertEquals(summary.results.length, 1); + assertEquals(summary.results[0].name, "@test/lazy-default"); + assertEquals(summary.results[0].success, true); +}); + +Deno.test("executeReports: handles ReportRef object form in require", async () => { + const { registry, loaderCallCount } = createRegistryWithLazyReport( + "@test/lazy-ref", + ); + const { repo } = createInMemoryDataRepo(); + const modelType = ModelType.create("test/model"); + const context = makeMethodContext(repo, modelType); + + const summary = await executeReports( + registry, + context, + modelType, + "test-id", + { require: [{ name: "@test/lazy-ref", methods: ["run"] }] }, + {}, + undefined, + "run", + undefined, + ); + + assertEquals(loaderCallCount.value, 1); + assertEquals(summary.results.length, 1); + assertEquals(summary.results[0].name, "@test/lazy-ref"); + assertEquals(summary.results[0].success, true); +}); + +Deno.test("executeReports: ensureTypeLoaded failure fails loudly", async () => { + const registry = new ReportRegistry(); + registry.registerLazy({ + type: "@test/lazy-broken", + bundlePath: "/tmp/fake-bundles/broken.js", + sourcePath: "/tmp/fake-sources/broken.ts", + version: "2026.04.11.1", + }); + registry.setTypeLoader(() => { + return Promise.reject(new Error("bundle import failed")); + }); + + const { repo } = createInMemoryDataRepo(); + const modelType = ModelType.create("test/model"); + const context = makeMethodContext(repo, modelType); + + let caught: Error | undefined; + try { + await executeReports( + registry, + context, + modelType, + "test-id", + { require: ["@test/lazy-broken"] }, + {}, + undefined, + "run", + undefined, + ); + } catch (err) { + caught = err as Error; + } + + assertEquals(caught !== undefined, true); + assertStringIncludes(caught!.message, "bundle import failed"); +}); + +Deno.test("executeReports: already-loaded reports are not re-promoted", async () => { + const registry = new ReportRegistry(); + registry.register("@test/already-loaded", makeReport("method")); + + let loaderCalled = false; + registry.setTypeLoader(() => { + loaderCalled = true; + return Promise.reject(new Error("type loader should not be called")); + }); + + const { repo } = createInMemoryDataRepo(); + const modelType = ModelType.create("test/model"); + const context = makeMethodContext(repo, modelType); + + const summary = await executeReports( + registry, + context, + modelType, + "test-id", + { require: ["@test/already-loaded"] }, + {}, + undefined, + "run", + undefined, + ); + + assertEquals(loaderCalled, false); + assertEquals(summary.results.length, 1); + assertEquals(summary.results[0].success, true); +});