fix: make CatalogStore required on FileSystemUnifiedDataRepository

[adamhjk]

Summary

• Make CatalogStore a required constructor parameter on FileSystemUnifiedDataRepository, turning missing-catalog bugs into compile errors
• Fix the root cause of swamp-club#39: executeModelMethod in WorkflowExecutionService created a data repo without catalog write-through, so data.latest() returned null for data written by workflow steps
• Add createCatalogStore() factory helper to centralize catalog construction
• Make queryData required on ModelMethodRunDeps and catalogStore required on WorkflowRunDeps
• Remove redundant ?. optional chaining on catalogStore across CLI/serve code
• Update design/data-query.md to reflect required semantics

Test Plan

• deno check passes (type system enforces catalog presence at all 120+ call sites)
• deno lint clean
• deno fmt clean
• All 4280 tests pass
• Reproduced bug scenario in scratch repo: workflow with write step → data.latest() read step now succeeds (previously failed with No such key: attributes)

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — This PR is a pure internal refactoring. All CLI command changes are limited to removing ?. optional chaining on catalogStore (now a required field) and simplifying DataQueryService construction in model_method_run.ts. No user-visible output, error messages, flags, help text, or JSON schema are affected.

[github-actions]

Code Review

This is a well-executed bug fix that makes CatalogStore a required dependency at the type level, turning the runtime bug (swamp-club#39) into a compile-time error. The approach is sound: enforce the invariant in the type system rather than relying on callers to remember the optional parameter.

Blocking Issues

None.

Suggestions

1. Temp dir leak in run_test.ts: createTestDeps() in src/libswamp/workflows/run_test.ts (lines 174-175) creates temp directories with Deno.makeTempDirSync() but never cleans them up. This is a pre-existing pattern so not a regression, but worth noting for a future cleanup pass.

2. DDD alignment: Good use of the factory pattern with createCatalogStore() to centralize catalog construction. Making CatalogStore required on FileSystemUnifiedDataRepository correctly enforces the aggregate invariant that every data mutation must maintain catalog consistency — this is the right place to enforce it.

Verification

• ✅ No libswamp import boundary violations — CLI/presentation code imports only from src/libswamp/mod.ts
• ✅ All ?. optional chaining on catalogStore replaced with . (required access)
• ✅ WorkflowRunDeps.catalogStore, ModelMethodRunDeps.queryData, and StepExecutionContext.catalogStore all made required
• ✅ Design doc (design/data-query.md) updated to reflect required semantics
• ✅ All 120+ test call sites updated with CatalogStore instances
• ✅ createCatalogStore() factory properly resolves datastore paths
• ✅ No any types introduced, no security concerns, license headers intact

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. src/libswamp/workflows/run_test.ts:174,441 — Temp directory leak with SQLite handle

   createTestDeps() and createTestDepsWithCapture() both call Deno.makeTempDirSync() and open a CatalogStore (SQLite DB) but never clean up the directory or close the database. Each test invocation leaves a temp dir with an open SQLite file behind.

   Breaking example: Not a production issue, but if the test suite grows, accumulated temp dirs and unclosed SQLite handles could exhaust file descriptors or disk in CI environments.

   Suggested fix: Wrap in a cleanup pattern, or use the existing withTempDir helper that other test files use, and call catalogStore.close() in a finally block.

Low

1. src/libswamp/ createXxxDeps functions — Multiple CatalogStore instances per repo*

   Each createXxxDeps() factory (gc, get, list, rename, versions, delete, evaluate, output_data, output_logs, validate, reports/search) calls createCatalogStore() which opens a fresh SQLite connection. If multiple operations run concurrently against the same repo, there will be multiple open connections to the same _catalog.db. This is pre-existing behavior that WAL mode + busy_timeout=5000 handles, and the PR doesn't change the lifecycle pattern — it just makes the parameter required. Not a new risk.

2. Tests using hardcoded /tmp paths for CatalogStore (e.g., unified_data_repository_test.ts:50, execution_service_test.ts:1323)

   A few tests construct CatalogStore(join("/tmp/test-repo", "_catalog.db")) or similar. The CatalogStore constructor calls ensureDirSync which will create these directories as a side effect. Not harmful in practice since /tmp is ephemeral, but leaves debris between test runs.

Verdict

PASS — This is a clean, well-structured bug fix. The core change (making CatalogStore required on FileSystemUnifiedDataRepository) is sound: it turns a runtime "missing catalog" silent failure into a compile-time error. The root cause fix in DefaultStepExecutor.executeModelMethod correctly threads the catalogStore through StepExecutionContext. The createCatalogStore() factory helper correctly replicates the previous inline catalog construction pattern. All 41 changed files are consistent with the new contract, and the type system (deno check passing) enforces correctness at all 120+ call sites. No logic errors, security issues, or data integrity concerns.