fix: delete methods return last known state data

[stack72]

Summary

Closes #636

When a model's delete method succeeds, the deletion marker (tombstone) now includes the last known active state attributes merged with the deletion metadata. This fixes two problems reported in the issue:

• Delete responses now include dataArtifacts — Previously the JSON response had no data field, making it impossible to confirm what was deleted without a separate query. Now the response includes the full resource attributes alongside deletedAt/deletedByMethod.

• data.latest() resolves original attributes after deletion — Previously, tombstones only contained {deletedAt, deletedByMethod}, which broke CEL expressions like data.latest("x", "y").attributes.RouteTableId on workflow re-runs. Now those expressions resolve correctly, enabling idempotent re-runs of complex delete workflows.

Design decisions

Enrich the tombstone, not data.latest() — We considered making data.latest() lifecycle-aware (skip tombstones, return last active version) but rejected it because:

1. It would be a breaking change for anyone checking data.latest() to detect deletion
2. Users would need to update existing CEL expressions to use a new function
3. The enriched tombstone approach requires zero changes to existing workflows on both sides — delete detection (deletedAt is still there) and re-runs (original attributes are still there)

Flat merge, accepted collision risk — Deletion metadata (deletedAt, deletedByMethod) is merged into the same JSON object as resource attributes. We considered namespacing (e.g. _deleted.at) but accepted the theoretical collision risk since cloud APIs don't use these field names, and the added complexity wasn't justified.

Deletion markers as data handles — Rather than adding special-case logic in run.ts, deletion markers are appended to currentHandles in method_execution_service.ts so they flow through the existing data artifact pipeline. This means run.ts, the JSON renderer, and all presentation code required zero changes.

Before

{
  "modelId": "ca9547ff-...",
  "modelName": "test-volume",
  "type": "@stack72/digitalocean/volume",
  "methodName": "delete",
  "dataArtifacts": []
}

After

{
  "modelId": "ca9547ff-...",
  "modelName": "test-volume",
  "type": "@stack72/digitalocean/volume",
  "methodName": "delete",
  "dataArtifacts": [
    {
      "name": "main",
      "attributes": {
        "id": "8290fa55-...",
        "name": "swamp-crud-test-vol",
        "region": "us-east-1",
        "status": "active",
        "deletedAt": "2026-03-20T00:02:15.524Z",
        "deletedByMethod": "delete"
      }
    }
  ]
}

Changes

File	Change
src/domain/models/method_execution_service.ts	Read active content before writing tombstone; merge into marker; append as data handle
src/domain/models/method_execution_service_test.ts	Updated tests to verify enriched markers + new test for graceful degradation when no content exists

Test plan

• All 3402 tests pass
• E2E verified: create → delete returns enriched tombstone with full attributes
• E2E verified: re-create after delete still works
• Tombstone on disk contains merged state for data.latest() resolution
• Graceful degradation when active content is missing (marker-only metadata)
• CI passes

🤖 Generated with Claude Code

[github-actions]

Review: Approve ✅

Clean, well-tested change. No blocking issues.

What I checked

• DDD: Tombstone enrichment logic correctly lives in DefaultMethodExecutionService (domain service). Uses Data.withDeletionMarker properly and delegates persistence to the repository.
• Import boundary: No violations — both files are internal to src/domain/models/.
• Code style: No any types, license headers present, test naming follows conventions, named exports used.
• Test coverage: Three relevant test paths covered — enriched tombstone with last known state, graceful degradation when content is missing, and scoped markers for declared resources only. Mock helper cleanly extended with contentByName parameter.
• Security: JSON.parse is wrapped in try/catch, spread of parsed content is safe against prototype pollution in modern JS, and deletion metadata keys (deletedAt, deletedByMethod) overwrite any colliding resource attributes (acknowledged design tradeoff in the PR description).

Suggestion (non-blocking)

The deletion marker handles are pushed to currentHandles after ModelOutput is saved (line 652), so the persisted output won't include the deletion data artifacts in its artifacts.dataArtifacts. The returned in-memory result is correct. This matches the existing pattern for follow-up actions, but worth noting if output replay fidelity matters in the future.

🤖 Generated with Claude Code

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. Field collision silently overwrites resource attributes — method_execution_service.ts:694-698: The flat spread { ...lastKnownState, deletedAt: ..., deletedByMethod: ... } will silently overwrite any resource attribute named deletedAt or deletedByMethod. Some cloud APIs do return deletedAt for soft-deleted resources (e.g., GCP resources, some AWS services). If a resource had deletedAt: "2026-03-19T00:00:00Z" from the cloud provider, the tombstone would replace it with the local deletion timestamp, and data.latest() would return the wrong value. The PR description acknowledges this tradeoff — just flagging that the collision is more realistic than "theoretical" since deletedAt is a common API field name. Consider namespacing under a _tombstone key if this causes issues in practice.

2. TOCTOU between isDeleted check and getContent — method_execution_service.ts:671-681: data.isDeleted is checked on a snapshot from findAllForModel, then getContent reads the latest version. If a concurrent delete operation writes a tombstone between these two calls, getContent would return the tombstone content (containing deletedAt/deletedByMethod). The spread would then merge old deletion metadata into the new tombstone. The end result is still correct (new deletedAt/deletedByMethod override old ones), so this is a benign race, but worth noting.

Low

1. Broad catch swallows I/O errors — method_execution_service.ts:686: The catch block handles both JSON parse errors and any I/O errors (disk failures, permission errors) identically — silent fallback to empty state. A transient disk error during a real delete would produce a tombstone missing all resource attributes, with no log entry to debug it. Consider at minimum logging a warning.

2. Non-object JSON content produces nonsensical spread — method_execution_service.ts:694: If resource content were a JSON primitive or array (e.g., "hello" or [1,2,3]), the spread ...lastKnownState would produce unexpected keys ("0": 1, "1": 2, ... for arrays, nothing for primitives). In practice this can't happen because resources are defined with Zod object schemas, so this is theoretical only.

Verdict

PASS — The change is well-scoped, correctly guarded by the existing !data.isDeleted check, and thoroughly tested (including the graceful degradation path). The field collision risk is acknowledged and acceptable for now. The deletion marker as data handle approach is clean and avoids changes to downstream rendering code.