fix(data): accept string type in getContent, findByName, findAllForModel (swamp-club#364)

[stack72]

Summary

• Production UnifiedDataRepository.getContent() required a ModelType object, but workflow-scope reports only have step.modelType as a string — calling getContent(string, ...) threw TypeError: type.toDirectoryPath is not a function
• The testing helper (TestDataRepository) accepted strings, so extension tests passed but production failed at runtime
• Added ModelTypeInput type alias (string | ModelType) and coerceModelType() helper, then widened findByName, getContent, and findAllForModel to accept strings at the domain port and coerce in the production implementation

Test Plan

• Added 3 unit tests verifying string-typed calls to getContent, findByName, and findAllForModel return correct results
• Full test suite passes (5956 passed, 0 failed)
• Reproduced the bug in a scratch repo with a workflow-scope report calling getContent(step.modelType, ...) — confirmed type.toDirectoryPath is not a function error before fix, and successful data reads after fix

🤖 Generated with Claude Code

[github-actions]

Code Review

Clean, well-scoped fix for a runtime type mismatch between the domain port and its callers. The approach is sound from a DDD perspective — widening the repository port to accept ModelTypeInput (a union type) while coercing to the ModelType value object at the infrastructure boundary preserves the domain model's invariants. The coercion happens exactly once per entry point, keeping the internal implementation clean.

Blocking Issues

None.

Suggestions

1. Sync method consistency: The sync counterparts (findByNameSync, getContentSync, findAllForModelSync) still require ModelType only. If the same workflow-scope reports could call these paths, they'd hit the same toDirectoryPath is not a function error. Consider widening them in a follow-up if callers need it.

2. Test helper reuse: The three new tests duplicate the tmpDir + CatalogStore + cleanup boilerplate. The file already has a withDataRepo helper (line 759) that encapsulates this pattern — the new tests could use it for consistency and less code. Minor — doesn't block merge.

3. findAllForModel parameter reassignment: In findAllForModel, type = coerceModelType(type) reassigns the parameter. This works because the widened ModelTypeInput type allows it, but using a separate const coerced = coerceModelType(type) (like findByName does by passing to findByNameWithDepth) would be more consistent across the three methods. Stylistic only.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. Sync method variants not widened (src/domain/data/repositories.ts:333-392): The interface widens findByName, getContent, and findAllForModel to accept ModelTypeInput, but the sync counterparts (findByNameSync, getContentSync, findAllForModelSync) still require strict ModelType. If the same workflow-scope report code that triggered the original bug ever calls a sync method with a raw string, it will hit the same type.toDirectoryPath is not a function crash. This is low-risk today — sync callers I found (data_query_service.ts, data_record_mapper.ts, model_resolver.ts) all pass proper ModelType objects — but it creates a latent inconsistency in the API surface. Consider widening the sync methods too for parity, or documenting that they intentionally remain strict.

Low

1. Double coercion on internal calls (src/infrastructure/persistence/unified_data_repository.ts:460-470): In findAllForModel, after type = coerceModelType(type) on line 460, the loop calls this.findByName(type, modelId, dataName) on line 470, which coerces again inside findByNameWithDepth. This is an extra instanceof check per data item — harmless but redundant. Same applies to findAllForModelSince (line 287) calling findByName after already having a ModelType. Not worth fixing, just noting.

2. coerceModelType throws on empty/whitespace-only strings (src/domain/models/model_type.ts:178-181): ModelType.create("") throws "Model type cannot be empty". This is correct behavior and strictly better than the previous type.toDirectoryPath is not a function TypeError — but callers passing user-controlled strings won't get a domain-specific "model type not found" error, they'll get a generic validation error. The current callers pass step.modelType which should always be a valid non-empty string, so this is theoretical.

Verdict

PASS — The fix is correct, targeted, and well-tested. coerceModelType properly handles both branches, TypeScript narrowing after parameter reassignment works correctly, and the tests cover all three widened methods. The original runtime crash (type.toDirectoryPath is not a function) is cleanly resolved.