fix: scope CRUD lifecycle deleted-resource check to declared resource data (#645)

[stack72]

Summary

Fixes #645. PR #640 introduced a deleted-resource lifecycle check (#637) that calls findAllForModel() and throws if any data entry has lifecycle: "deleted". This is too broad — models accumulate multiple data entries over time (old data names, VPC IDs as data names, etc.), and old deleted entries cause false positives that block read/update operations on active resources.

Why This Is the Right Fix

The root cause is that the lifecycle check operates on all data entries for a model, but it should only consider data that belongs to declared resource specs. The writeResource() function already auto-injects a specName tag on every resource write (line 493 in data_writer.ts), so the infrastructure to distinguish declared resource data from historical/untagged data already exists — it just wasn't being used.

This fix:

1. Adds filterDeclaredResourceData() helper — filters data entries to only those with tags.type === "resource" AND tags.specName matching a key in the model's declared resources map.

2. Scopes the read/update pre-check — instead of "fail if ANY data is deleted", the semantic is now "fail if ALL declared-resource data is deleted." Old historical entries without specName tags are excluded from the check entirely.

3. Scopes deletion marker writing — the delete method now only writes deletion markers for declared resource data, not for untagged historical entries.

Why "all deleted" instead of "any deleted"

A model may have multiple declared resource specs. If one is deleted but another is active, blocking the entire model is too aggressive — the active resource should still be readable. The check now only blocks when every single declared resource data entry is deleted, which means the model genuinely has no active resources.

Backwards compatibility

Data created before PR #640 won't have specName tags. This is fine — those entries are excluded from the filter, so they can't trigger false positives. The lifecycle check only activates for data written by the current writeResource() which always injects specName.

User Impact

• Bug fixed: Models that accumulate data entries over multiple workflow runs no longer get falsely blocked on read/update with "Resource was deleted" errors
• No breaking changes: The Track resource deletion in local data model #637 protection is preserved — genuinely deleted resources (all declared resource data marked deleted) still block read/update and require a create to re-create
• No action required: Existing repos work without migration — pre-existing data without specName tags is simply excluded from the check

Files Changed

File	Change
src/domain/models/method_execution_service.ts	Add filterDeclaredResourceData() helper; scope read/update pre-check and deletion marker writing to declared resource data
src/domain/models/method_execution_service_test.ts	Add specName tags to 5 existing test data entries; add 3 new tests for the scoped behavior

Testing

• All 2772 tests pass (32 in method_execution_service_test.ts, including 3 new)
• New tests cover:
  • "read succeeds when old data is deleted but current resource data is active" (the data.latest() in workflow step inputs resolved at workflow start, not at step execution time #645 bug scenario)
  • "deletion markers only written for declared resource data, not untagged data"
  • "read blocked when all declared resource data is deleted" (preserves Track resource deletion in local data model #637)
• deno check, deno lint, deno fmt all pass
• deno run compile succeeds

🤖 Generated with Claude Code

[github-actions]

Review Summary

Verdict: APPROVED ✅

This PR correctly fixes #645 by scoping the deleted-resource lifecycle check to only consider data entries that belong to declared resource specs (using the specName tag that writeResource() auto-injects).

Code Quality

• ✅ TypeScript strict mode compliant — no any types
• ✅ Named exports used consistently
• ✅ Proper type imports (import type { Data })
• ✅ CI passes: lint, tests, and format checks all green

Domain-Driven Design

• ✅ filterDeclaredResourceData() is a well-encapsulated pure function
• ✅ Operates on domain data entities without side effects
• ✅ Logic is appropriately placed as a module-level helper in the execution service

Test Coverage

• ✅ 3 new tests added covering the critical scenarios:
  • Read succeeds when old data is deleted but current resource data is active
  • Deletion markers only written for declared resource data
  • Read blocked when all declared resource data is deleted
• ✅ 5 existing tests updated to include specName tags
• ✅ Tests follow project conventions (_test.ts suffix, @std/assert)

Security

• ✅ No security vulnerabilities identified
• ✅ JSON parsing wrapped in try/catch

Backwards Compatibility

• ✅ Data without specName tags (pre-PR #640) is excluded from filtering
• ✅ Existing workflows continue to work without migration

Minor Suggestions (Non-blocking)

1. Variable shadowing at line 410: The resources variable is redeclared inside the if (methodKind === "delete") block. This is valid (different scope) and keeps the code self-contained, but could be avoided by reusing the outer declaration at line 330.

2. Type specificity: declaredResources: Record<string, unknown> works since only keys are used, but could be more specific if there's a canonical type for the resources map.

Both are minor style preferences that don't warrant changes.

🤖 Generated with Claude Code

[github-actions]

Adversarial Review

I traced through every code path in this PR looking for logic errors, edge cases, security issues, and failure modes. The implementation is solid.

Critical / High

None found.

Medium

1. Tag override order could allow specName spoofing (method_execution_service.ts, data_writer.ts:500-525)

   The specName tag auto-injected at line 493 can be overridden by tagOverrides, runtimeTags, or dataOutputOverrides applied later. If a workflow or user sets --tag specName=nonexistent, newly written data would have a mismatched specName that excludes it from lifecycle checks.

   Breaking example: User runs swamp model run my-model delete --tag specName=fake. The deletion markers written would have specName=fake, which won't match declared resources. Future reads on this model wouldn't see those data entries as deleted.

   Impact: This requires deliberate misconfiguration and only affects new data writes (not the lifecycle check on existing data). The filter operates on stored tags, not runtime tags.

   Suggested fix: Consider making specName immune to overrides, or document this as expected behavior. Low urgency since it requires deliberate action.

Low

1. Error message shows only first deleted resource (method_execution_service.ts:302)

   When all declared resources are deleted, the error reports resourceData[0].name. If a model has multiple resource specs (e.g., vpc, subnet) and both are deleted, users only see one resource name in the error.

   Impact: Minor UX issue. Users might not realize all resources are deleted.

2. Broad catch block swallows unexpected errors (method_execution_service.ts:318-320)

   The catch block around JSON.parse and getContent silently catches all errors. If getContent fails for unexpected reasons (permission error, disk corruption), the error is swallowed.

   Impact: Minimal — only affects the deletedAt timestamp display. Correct behavior is to show "unknown".

3. TypeScript Record<string, string> doesn't model missing keys (filterDeclaredResourceData)

   d.tags["specName"] is typed as string but can be undefined at runtime. The code correctly guards with !== undefined before calling specNames.has(), so this is handled correctly despite TypeScript's typing quirk.

Observations (Not Issues)

• The semantic change from "any deleted blocks read/update" to "all declared resources deleted blocks read/update" is intentional and well-documented in the PR description
• Backwards compatibility is preserved: pre-existing data without specName tags is simply excluded from the filter (won't trigger false positives)
• The withDeletionMarker method correctly preserves original tags, so deletion markers inherit the correct specName
• Short-circuit evaluation in the filter prevents specNames.has(undefined) from being called
• All three new tests cover the key scenarios: mixed deleted/active data, scoped deletion markers, and blocked reads

Verdict

PASS — The logic change is correct, edge cases are properly handled, and tests are comprehensive. The medium finding about tag override order is documented behavior and requires deliberate misconfiguration to trigger. No blocking issues.