fix: promote lazy-loaded reports in executeReports() before filtering#1161
fix: promote lazy-loaded reports in executeReports() before filtering#1161
Conversation
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.
There was a problem hiding this comment.
Code Review
Well-structured bug fix for a real regression. The fix is centralized in executeReports() so all four call sites benefit automatically, the design doc is updated to document the promotion contract, and the test coverage is thorough.
Blocking Issues
None.
Suggestions
-
Test helper
makeMethodContextusesas unknown asandas anycasts (lines 1099-1102): This is consistent with every otherexecuteReportstest in the file, so it's fine here — but if the test infrastructure ever gets a shared test-context builder, these helpers should use it. -
Comment length in the production fix: The 20-line block comment in
executeReports()(lines 335-353) is unusually long for inline code. The design doc already captures the full rationale (the "Promotion contract for iteration" section). The inline comment could be trimmed to ~5 lines pointing at the design doc and the issue number, keeping the code more scannable. Not blocking since thorough comments are preferable to missing ones.
Overall: clean fix, correct DDD placement (domain service owns the promotion responsibility rather than pushing it to callers), good test coverage of the happy path, error propagation, ReportRef object form, and idempotency for already-loaded reports. LGTM.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None found.
Medium
-
Promise.allsurfaces only the first failure when multiple lazy reports fail —src/domain/reports/report_execution_service.ts:363. If a user requires two lazy reports and both bundles are broken,Promise.allrejects with the first error and the second is swallowed. The user sees "bundle X failed" but not "bundle Y also failed." In practice this is unlikely to cause confusion (fix one, re-run, see the next), andPromise.allSettledwould add complexity to the error-reporting path for a marginal gain. Noting for awareness, not blocking. -
ensureTypeLoadedfailure in the failed-method report path is silently swallowed —src/domain/workflows/execution_service.ts:883-897. When a method execution fails, the catch block at line 895 logs the promotion error atdebuglevel and continues. This is intentional (the catch predates this PR and is there to avoid masking the original execution error), but it means a broken lazy report bundle in the failed-method path will be silently dropped. This is pre-existing behavior for report execution errors; the new promotion errors now follow the same path. No action needed from this PR.
Low
- No test for workflow-scope lazy promotion — The five new tests all use
scope: "method"contexts. The promotion logic is scope-agnostic (it promotes all candidates, thenfilterReportshandles scope), so method-scope tests are sufficient to exercise the promotion code path. A workflow-scope test would only add coverage forfilterReports's scope matching, which is already covered by existingfilterReportsunit tests. Not blocking.
Verdict
PASS. Clean, well-targeted fix. The core logic is correct: build the candidate set from selection.require ∪ modelTypeReports, promote each via ensureTypeLoaded (which is a no-op for already-loaded and unknown names), then proceed with getAll() + filterReports(). The Set correctly deduplicates overlapping names. Error propagation is sound — ensureTypeLoaded cleans up typeLoadPromises in both success and failure paths, and the re-throw ensures broken required reports fail loudly on the success execution path. The five new tests cover the key scenarios (require string, require ReportRef object, modelTypeReports defaults, failure propagation, already-loaded no-op). The centralised fix correctly covers all call sites without requiring changes to callers.
Summary
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()callsregistry.getAll(), which returns onlyfully-loaded entries from the
reportsMap. User reports registeredlazily from the bundle catalog live in
lazyTypesand were silentlydropped by
filterReports()before their bundles were ever imported.Builtin reports like
@swamp/workflow-summarykept working because theyare registered eagerly in
builtin/mod.ts.The fix. In
executeReports(), before callingregistry.getAll(),build a deduped set of candidate names from
selection.require(viagetReportRefName()to handle theReportRef | stringunion) andmodelTypeReports, thenawait 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 fourexecuteReports()call sites (workflow scope, method scope, modelscope, and failed-method summary).
Errors from
ensureTypeLoaded()propagate unchanged — a broken bundlefor a required report fails loudly rather than being silently skipped.
Promotion is unconditional for every candidate name (we cannot inspect
report.scopeuntil the bundle is imported), which may waste one importon scope-mismatched reports; this is bounded by the candidate set size
and
typeLoadPromisesdedupes concurrent callers.Also updates
design/reports.md"Report Registry" section to describethe post-#1089 two-state model (fully loaded vs lazy),
ensureTypeLoaded,setLoader/setTypeLoader, and the promotioncontract that domain services must honor before iterating via
getAll().Before / After
Minimal reproduction: a workflow requires a workflow-scope user report
@test/repro-81. Runswamp workflow run test-workflowtwice.Before this fix — run 1 executes the report, run 2 silently drops it:
After this fix — both runs execute the report:
Test Plan
deno check— clean (1021 files)deno lint— clean (1021 files)deno fmt --check— clean (1127 files)deno run test— 4286 passed, 0 failedreport_execution_service_test.tscoveringlazy-report promotion via
selection.require, viamodelTypeReportsdefaults, via the
ReportRefobject form, loud failure onensureTypeLoadedrejection, and no-op for already-loaded reportsthe repro scratch repo: user report executes on both run 1
(catalog bootstrap) and run 2 (catalog populated — previously
broken)