refactor: remove resourceId from ModelInput, use convention-based resource linking#81
Conversation
…ource linking - Remove `resourceId` field from `ModelInput` domain entity - Adopt convention-based linking: resource ID always equals input ID - Use existing `inputIdToResourceId()` helper consistently across all code paths - Update AWS CloudControl delete operations to gracefully handle missing resources ## Changes **Domain Model:** - Remove `resourceId` from `ModelInputSchema`, props, constructor, getter, setter, and serialization - Delete `setResourceId()` method **Execution Paths:** - Remove resourceId updates in `model_method_run.ts` after method execution - Remove resourceId updates in `execution_service.ts` during workflow execution - Remove resourceId from `method_execution_service.ts` follow-up actions **Resource Resolution:** - Update `model_resolver.ts` to use `inputIdToResourceId(input.id)` for resource lookups - Simplify `resolveModel()` to always derive resource ID from input ID **AWS CloudControl:** - Change delete behavior: instead of throwing when no resource exists, return "deleted" result - Look up resource by derived ID and extract AWS identifier from attributes **Display Commands:** - Remove resourceId from model search and edit item mappings ## Test plan - [x] All 1178 tests pass - [x] `deno check` - Type checking passes - [x] `deno lint` - No lint errors - [x] `deno fmt` - Properly formatted - [x] `deno run compile` - Binary compiles 🤖 Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Review Summary
This PR implements a clean refactoring that removes the resourceId field from ModelInput in favor of a convention-based linking approach where resource ID always equals input ID. This is a well-executed simplification of the domain model.
No Blocking Issues Found
What Was Reviewed
Domain Model Changes
ModelInputentity properly removesresourceIdfrom schema, props, constructor, getter, setter, and serialization- The
inputIdToResourceId()helper function inmodel_resource.tsis used correctly throughout for the convention-based linking - Design documentation updated to reflect the new convention
AWS CloudControl Model Changes
executeDelete()now gracefully handles the case where no resource exists (returns success withdeleteResource: true)- Properly extracts AWS resource identifier from resource attributes
- Good error handling when unable to extract AWS resource identifier
Resolution Logic
ModelResolvercorrectly updated to useinputIdToResourceId(input.id)for resource lookups- Both
buildModelData()andresolveModel()use the convention consistently
Test Coverage
- All three AWS model tests (EC2 Instance, Subnet, VPC) updated to reflect new behavior
- Tests properly verify the "delete method without resource returns deleted result" scenario
ModelInputtests updated to removeresourceId-related assertions- Good mock implementations for
ResourceRepositoryin tests
Code Style
- Uses named exports (compliant with CLAUDE.md)
- No
anytypes found in the changes - TypeScript strict mode maintained
Suggestions (Non-Blocking)
-
Comment in ec2_instance_model_test.ts (line 19-26): The comment block at the top of the test file still shows
resourceIdin the example attributes. This is just a documentation comment and doesn't affect functionality, but could be cleaned up for consistency:/* Example instance attributes id: 9ad6033a-e77b-4e3b-8347-c5e10dad073b resourceId: 9ad6033a-e77b-4e3b-8347-c5e10dad073b // <-- could remove this line ... */
-
The PR description mentions all 1178 tests pass, and the implementation is clean and consistent. The convention-based approach simplifies the domain model by eliminating redundant state tracking.
LGTM! 🎉
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.
resourceIdfield fromModelInputdomain entityinputIdToResourceId()helper consistently across all code pathsChanges
Domain Model:
resourceIdfromModelInputSchema, props, constructor, getter, setter, and serializationsetResourceId()methodExecution Paths:
model_method_run.tsafter method executionexecution_service.tsduring workflow executionmethod_execution_service.tsfollow-up actionsResource Resolution:
model_resolver.tsto useinputIdToResourceId(input.id)for resource lookupsresolveModel()to always derive resource ID from input IDAWS CloudControl:
Display Commands:
Test plan
deno check- Type checking passesdeno lint- No lint errorsdeno fmt- Properly formatteddeno run compile- Binary compiles🤖 Generated with Claude Code