From 0be85a6bad98f3ca0fe7cc28fc6c12369d557bb1 Mon Sep 17 00:00:00 2001 From: stack72 Date: Sun, 8 Mar 2026 00:04:16 +0000 Subject: [PATCH] fix: scope CRUD lifecycle deleted-resource check to declared resource data (#645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 operations on active resources. Scope the check to only declared resource data using the specName tag that writeResource() automatically injects on every resource write. Change the semantic from 'fail if ANY data is deleted' to 'fail if ALL declared-resource data is deleted'. Co-Authored-By: Claude Opus 4.6 --- src/domain/models/method_execution_service.ts | 77 ++++-- .../models/method_execution_service_test.ts | 231 +++++++++++++++++- 2 files changed, 279 insertions(+), 29 deletions(-) diff --git a/src/domain/models/method_execution_service.ts b/src/domain/models/method_execution_service.ts index 4e5455cd..43b5f4eb 100644 --- a/src/domain/models/method_execution_service.ts +++ b/src/domain/models/method_execution_service.ts @@ -39,6 +39,7 @@ import { } from "./data_writer.ts"; import { coerceMethodArgs } from "./zod_type_coercion.ts"; import { containsExpression } from "../expressions/expression_parser.ts"; +import type { Data } from "../data/data.ts"; /** * Maximum depth for recursive follow-up action processing. @@ -46,6 +47,22 @@ import { containsExpression } from "../expressions/expression_parser.ts"; */ const DEFAULT_MAX_FOLLOW_UP_DEPTH = 100; +/** + * Filters data entries to only those belonging to declared resource specs. + * Uses the `specName` tag that `writeResource()` auto-injects on every write. + */ +function filterDeclaredResourceData( + allData: Data[], + declaredResources: Record, +): Data[] { + const specNames = new Set(Object.keys(declaredResources)); + return allData.filter((d) => + d.tags["type"] === "resource" && + d.tags["specName"] !== undefined && + specNames.has(d.tags["specName"]) + ); +} + /** * Formats a Zod error into a human-readable string. */ @@ -264,7 +281,10 @@ export class DefaultMethodExecutionService implements MethodExecutionService { // Infer method kind for lifecycle checks const methodKind = inferMethodKind(methodName, method); - // Fast-fail: reject read/update on deleted resources + // Fast-fail: reject read/update on deleted resources. + // Only check declared resource data (tagged with specName matching a + // declared resource spec). Block only when ALL declared resource data + // is deleted — old historical data entries should not cause false positives. if (methodKind === "read" || methodKind === "update") { const resources = modelDef.resources ?? {}; if (Object.keys(resources).length > 0) { @@ -272,31 +292,35 @@ export class DefaultMethodExecutionService implements MethodExecutionService { context.modelType, context.modelId, ); - for (const data of existingData) { - if (data.isDeleted) { - // Read the deletion marker content for the timestamp - let deletedAt = "unknown"; - try { - const content = await context.dataRepository.getContent( - context.modelType, - context.modelId, - data.name, + const resourceData = filterDeclaredResourceData( + existingData, + resources, + ); + if ( + resourceData.length > 0 && resourceData.every((d) => d.isDeleted) + ) { + const deleted = resourceData[0]; + let deletedAt = "unknown"; + try { + const content = await context.dataRepository.getContent( + context.modelType, + context.modelId, + deleted.name, + ); + if (content) { + const marker = JSON.parse( + new TextDecoder().decode(content), ); - if (content) { - const marker = JSON.parse( - new TextDecoder().decode(content), - ); - if (marker.deletedAt) { - deletedAt = marker.deletedAt; - } + if (marker.deletedAt) { + deletedAt = marker.deletedAt; } - } catch { - // Use default "unknown" timestamp } - throw new UserError( - `Resource '${data.name}' was deleted at ${deletedAt} — run a 'create' method to re-create it first`, - ); + } catch { + // Use default "unknown" timestamp } + throw new UserError( + `Resource '${deleted.name}' was deleted at ${deletedAt} — run a 'create' method to re-create it first`, + ); } } } @@ -380,7 +404,8 @@ export class DefaultMethodExecutionService implements MethodExecutionService { await context.outputRepository.save(modelDef.type, methodName, output); } - // Write deletion markers after a successful delete method + // Write deletion markers after a successful delete method. + // Only mark declared resource data — untagged or non-resource data is left alone. if (methodKind === "delete") { const resources = modelDef.resources ?? {}; if (Object.keys(resources).length > 0) { @@ -388,7 +413,11 @@ export class DefaultMethodExecutionService implements MethodExecutionService { context.modelType, context.modelId, ); - for (const data of existingData) { + const resourceData = filterDeclaredResourceData( + existingData, + resources, + ); + for (const data of resourceData) { if (!data.isDeleted) { const marker = data.withDeletionMarker({ version: data.version + 1, diff --git a/src/domain/models/method_execution_service_test.ts b/src/domain/models/method_execution_service_test.ts index de850ef1..b48cfec1 100644 --- a/src/domain/models/method_execution_service_test.ts +++ b/src/domain/models/method_execution_service_test.ts @@ -1168,7 +1168,7 @@ Deno.test("executeWorkflow - delete method writes deletion markers for existing contentType: "application/json", lifetime: "infinite", garbageCollection: 10, - tags: { type: "resource" }, + tags: { type: "resource", specName: "my-resource" }, ownerDefinition: { ownerType: "model-method", ownerRef: "test/model:create", @@ -1237,7 +1237,7 @@ Deno.test("executeWorkflow - read after delete throws UserError", async () => { contentType: "application/json", lifetime: "infinite", garbageCollection: 10, - tags: { type: "resource" }, + tags: { type: "resource", specName: "my-resource" }, ownerDefinition: { ownerType: "model-method", ownerRef: "test/model:create", @@ -1302,7 +1302,7 @@ Deno.test("executeWorkflow - update after delete throws UserError", async () => contentType: "application/json", lifetime: "infinite", garbageCollection: 10, - tags: { type: "resource" }, + tags: { type: "resource", specName: "my-resource" }, ownerDefinition: { ownerType: "model-method", ownerRef: "test/model:create", @@ -1418,7 +1418,7 @@ Deno.test("executeWorkflow - delete skips already-deleted resources", async () = contentType: "application/json", lifetime: "infinite", garbageCollection: 10, - tags: { type: "resource" }, + tags: { type: "resource", specName: "my-resource" }, ownerDefinition: { ownerType: "model-method", ownerRef: "test/model:create", @@ -1479,7 +1479,7 @@ Deno.test("executeWorkflow - explicit kind overrides name inference for deletion contentType: "application/json", lifetime: "infinite", garbageCollection: 10, - tags: { type: "resource" }, + tags: { type: "resource", specName: "my-resource" }, ownerDefinition: { ownerType: "model-method", ownerRef: "test/model:create", @@ -1531,6 +1531,227 @@ Deno.test("executeWorkflow - explicit kind overrides name inference for deletion assertEquals(mockRepo.savedData.length, 0); }); +Deno.test("executeWorkflow - read succeeds when old data is deleted but current resource data is active", async () => { + const service = new DefaultMethodExecutionService(); + + // Old historical data entry (no specName tag) that was deleted — should NOT block + const oldDeletedData = Data.create({ + name: "old-vpc-id-123", + contentType: "application/json", + lifetime: "infinite", + garbageCollection: 10, + tags: { type: "resource" }, + ownerDefinition: { + ownerType: "model-method", + ownerRef: "test/model:create", + }, + lifecycle: "deleted", + }); + + // Current active resource data with specName tag + const activeData = Data.create({ + name: "my-resource", + contentType: "application/json", + lifetime: "infinite", + garbageCollection: 10, + tags: { type: "resource", specName: "my-resource" }, + ownerDefinition: { + ownerType: "model-method", + ownerRef: "test/model:create", + }, + }); + + const mockRepo = { + ...createMockDataRepo(), + findAllForModel: () => Promise.resolve([oldDeletedData, activeData]), + getContent: () => Promise.resolve(null), + }; + + const model: ModelDefinition = { + type: ModelType.create("test/read-mixed-data"), + version: "1", + resources: { + "my-resource": { + description: "A resource", + schema: z.object({}), + lifetime: "infinite", + garbageCollection: 10, + }, + }, + methods: { + get: { + description: "Get the resource", + arguments: z.object({}), + execute: () => Promise.resolve({}), + }, + }, + }; + + const definition = Definition.create({ + name: "test-definition", + globalArguments: {}, + }); + + const { context } = createTestContext({ modelType: model.type }); + const contextWithRepo: MethodContext = { + ...context, + dataRepository: mockRepo, + }; + + // Should succeed — old deleted data without specName should not block read + const result = await service.executeWorkflow( + definition, + model, + "get", + contextWithRepo, + ); + assertEquals(result !== undefined, true); +}); + +Deno.test("executeWorkflow - deletion markers only written for declared resource data, not untagged data", async () => { + const service = new DefaultMethodExecutionService(); + + // Declared resource data with specName tag — should get a deletion marker + const resourceData = Data.create({ + name: "my-resource", + contentType: "application/json", + lifetime: "infinite", + garbageCollection: 10, + tags: { type: "resource", specName: "my-resource" }, + ownerDefinition: { + ownerType: "model-method", + ownerRef: "test/model:create", + }, + version: 3, + }); + + // Untagged data (no specName) — should NOT get a deletion marker + const untaggedData = Data.create({ + name: "some-old-data", + contentType: "application/json", + lifetime: "infinite", + garbageCollection: 10, + tags: { type: "resource" }, + ownerDefinition: { + ownerType: "model-method", + ownerRef: "test/model:create", + }, + version: 2, + }); + + const mockRepo = createMockDataRepoWithData([resourceData, untaggedData]); + + const model: ModelDefinition = { + type: ModelType.create("test/delete-scoped-markers"), + version: "1", + resources: { + "my-resource": { + description: "A resource", + schema: z.object({}), + lifetime: "infinite", + garbageCollection: 10, + }, + }, + methods: { + delete: { + description: "Delete the resource", + arguments: z.object({}), + execute: () => Promise.resolve({}), + }, + }, + }; + + const definition = Definition.create({ + name: "test-definition", + globalArguments: {}, + }); + + const { context } = createTestContext({ modelType: model.type }); + const contextWithRepo: MethodContext = { + ...context, + dataRepository: mockRepo, + }; + + await service.executeWorkflow( + definition, + model, + "delete", + contextWithRepo, + ); + + // Only the declared resource data should get a deletion marker + assertEquals(mockRepo.savedData.length, 1); + assertEquals(mockRepo.savedData[0].data.name, "my-resource"); + assertEquals(mockRepo.savedData[0].data.lifecycle, "deleted"); +}); + +Deno.test("executeWorkflow - read blocked when all declared resource data is deleted", async () => { + const service = new DefaultMethodExecutionService(); + + // All declared resource data is deleted + const deletedResource = Data.create({ + name: "my-resource", + contentType: "application/json", + lifetime: "infinite", + garbageCollection: 10, + tags: { type: "resource", specName: "my-resource" }, + ownerDefinition: { + ownerType: "model-method", + ownerRef: "test/model:create", + }, + lifecycle: "deleted", + }); + + const markerContent = new TextEncoder().encode(JSON.stringify({ + deletedAt: "2026-03-07T10:00:00.000Z", + deletedByMethod: "delete", + })); + + const mockRepo = { + ...createMockDataRepo(), + findAllForModel: () => Promise.resolve([deletedResource]), + getContent: () => Promise.resolve(markerContent), + }; + + const model: ModelDefinition = { + type: ModelType.create("test/read-all-deleted"), + version: "1", + resources: { + "my-resource": { + description: "A resource", + schema: z.object({}), + lifetime: "infinite", + garbageCollection: 10, + }, + }, + methods: { + get: { + description: "Get the resource", + arguments: z.object({}), + execute: () => Promise.resolve({}), + }, + }, + }; + + const definition = Definition.create({ + name: "test-definition", + globalArguments: {}, + }); + + const { context } = createTestContext({ modelType: model.type }); + const contextWithRepo: MethodContext = { + ...context, + dataRepository: mockRepo, + }; + + // Should throw — all declared resource data is deleted + await assertRejects( + () => service.executeWorkflow(definition, model, "get", contextWithRepo), + UserError, + "was deleted at 2026-03-07T10:00:00.000Z", + ); +}); + // ---------- Unresolved Expression Tests ---------- Deno.test("execute - Proxy throws for any unresolved expression in globalArgs", async () => {