diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index e057f8b1..2c501a2d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -204,6 +204,17 @@ cannot be updated or deleted. A durable supersession row drives post-commit cleanup, and object storage bytes are deleted only after an exact organization/reference query proves that no persisted owner still pins them. +Four exact parent-owned capabilities isolate the Skill artifact lifecycle. +`assetregistry::skill-package` accepts canonical validated uploads; +`assetregistry::skill-delivery` returns authorized release facts and content +without a storage locator; `assetregistry::skill-cleanup` exposes only the +Worker batch trigger and immutable summary; and +`assetregistry::skill-storage` is limited to exact parent persistence, +delivery, cleanup, and MinIO consumers. The parent owns storage writes, +compensation, reference persistence, supersession retry state, cleanup, and +storage opening. Skill package semantics never receive or publish the stored +object key. + The browser reaches that same lifecycle through Scratch authoring, bounded `SKILL.md`/ZIP/folder upload, or GitHub import; each path ends at an ordinary private Draft in the Assets Governance workspace. GitHub preview and eligible diff --git a/apps/api/src/test/java/com/orgmemory/api/SkillCapabilityBoundaryTests.java b/apps/api/src/test/java/com/orgmemory/api/SkillCapabilityBoundaryTests.java new file mode 100644 index 00000000..ca7a6fb2 --- /dev/null +++ b/apps/api/src/test/java/com/orgmemory/api/SkillCapabilityBoundaryTests.java @@ -0,0 +1,33 @@ +package com.orgmemory.api; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.core.importer.ImportOption; +import java.util.Set; +import java.util.TreeSet; +import org.junit.jupiter.api.Test; + +class SkillCapabilityBoundaryTests { + + private static final Set PARENT_CAPABILITY_PACKAGES = Set.of( + "com.orgmemory.core.assetregistry.skillpackage.", + "com.orgmemory.core.assetregistry.skilldelivery.", + "com.orgmemory.core.assetregistry.skillcleanup.", + "com.orgmemory.core.assetregistry.skillstorage."); + + @Test + void apiDoesNotImportParentSkillCapabilities() { + var dependencies = new ClassFileImporter() + .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) + .importPackages("com.orgmemory.api") + .stream() + .flatMap(type -> type.getDirectDependenciesFromSelf().stream()) + .map(dependency -> dependency.getTargetClass().getName()) + .filter(name -> PARENT_CAPABILITY_PACKAGES.stream() + .anyMatch(name::startsWith)) + .collect(TreeSet::new, Set::add, Set::addAll); + + assertEquals(Set.of(), dependencies); + } +} diff --git a/apps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.java b/apps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.java index 7c003731..ab5cd91c 100644 --- a/apps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.java +++ b/apps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.java @@ -40,7 +40,7 @@ import com.orgmemory.core.assetregistry.prompt.PromptEvaluationResult; import com.orgmemory.core.assetregistry.prompt.PromptExecutionService; import com.orgmemory.core.assetregistry.promptcontract.PromptRunResult; -import com.orgmemory.core.assetregistry.SkillPackageStoragePort; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.assetregistry.SkillRegistryService; import com.orgmemory.core.assetregistry.WorkInstructionService; import com.orgmemory.core.assetregistry.WorkInstructionView; diff --git a/apps/worker/src/main/java/com/orgmemory/worker/assetregistry/SkillPackageSupersessionCleanupScheduler.java b/apps/worker/src/main/java/com/orgmemory/worker/assetregistry/SkillPackageSupersessionCleanupScheduler.java index 71db2426..339e7399 100644 --- a/apps/worker/src/main/java/com/orgmemory/worker/assetregistry/SkillPackageSupersessionCleanupScheduler.java +++ b/apps/worker/src/main/java/com/orgmemory/worker/assetregistry/SkillPackageSupersessionCleanupScheduler.java @@ -1,8 +1,7 @@ package com.orgmemory.worker.assetregistry; -import com.orgmemory.core.assetregistry.SkillPackageCleanupOutcome; -import com.orgmemory.core.assetregistry.SkillPackageSupersessionCleanupService; -import java.util.Map; +import com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupOperations; +import com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupSummary; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.scheduling.annotation.Scheduled; @@ -14,10 +13,10 @@ class SkillPackageSupersessionCleanupScheduler { private static final Logger LOGGER = LoggerFactory.getLogger(SkillPackageSupersessionCleanupScheduler.class); - private final SkillPackageSupersessionCleanupService cleanup; + private final SkillPackageCleanupOperations cleanup; SkillPackageSupersessionCleanupScheduler( - SkillPackageSupersessionCleanupService cleanup) { + SkillPackageCleanupOperations cleanup) { this.cleanup = cleanup; } @@ -25,9 +24,9 @@ class SkillPackageSupersessionCleanupScheduler { fixedDelayString = "${orgmemory.asset-registry.skill-package-cleanup-interval:1m}") void cleanup() { - Map outcomes = cleanup.cleanupPending(25); - if (!outcomes.isEmpty()) { - LOGGER.info("Skill package supersession cleanup outcomes={}", outcomes); + SkillPackageCleanupSummary summary = cleanup.cleanupPending(25); + if (!summary.isEmpty()) { + LOGGER.info("Skill package supersession cleanup summary={}", summary); } } } diff --git a/apps/worker/src/test/java/com/orgmemory/worker/SkillCapabilityBoundaryTests.java b/apps/worker/src/test/java/com/orgmemory/worker/SkillCapabilityBoundaryTests.java new file mode 100644 index 00000000..56d59ef9 --- /dev/null +++ b/apps/worker/src/test/java/com/orgmemory/worker/SkillCapabilityBoundaryTests.java @@ -0,0 +1,31 @@ +package com.orgmemory.worker; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.core.importer.ImportOption; +import java.util.Set; +import java.util.TreeSet; +import org.junit.jupiter.api.Test; + +class SkillCapabilityBoundaryTests { + + @Test + void workerImportsOnlyTheCleanupCapability() { + var dependencies = new ClassFileImporter() + .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) + .importPackages("com.orgmemory.worker") + .stream() + .flatMap(type -> type.getDirectDependenciesFromSelf().stream()) + .map(dependency -> dependency.getTargetClass().getName()) + .filter(name -> name.startsWith( + "com.orgmemory.core.assetregistry.skill")) + .collect(TreeSet::new, Set::add, Set::addAll); + + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupOperations", + "com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupSummary"), + dependencies); + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java b/core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java index 4fd870cb..b6dd9d9d 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java @@ -1,5 +1,6 @@ package com.orgmemory.core.assetregistry; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.shared.BaseEntity; import jakarta.persistence.Column; import jakarta.persistence.Entity; diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java b/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java index 1a85adf8..081a0405 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java @@ -1,5 +1,6 @@ package com.orgmemory.core.assetregistry; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.assetregistry.consumption.AssetAvailability; import com.orgmemory.core.assetregistry.consumption.AssetConsumptionRelease; import com.orgmemory.core.assetregistry.consumption.AssetPublicationMode; diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.java b/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.java index 7f28a58c..cd9c6056 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.java @@ -1,5 +1,6 @@ package com.orgmemory.core.assetregistry; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.assetregistry.consumption.AssetAvailability; import com.orgmemory.core.assetregistry.consumption.AssetConsumptionRelease; import com.orgmemory.core.assetregistry.consumption.AssetReleaseUseQuery; diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillDistributionService.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillDistributionService.java index cbfd375f..5e5b2ba3 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/SkillDistributionService.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillDistributionService.java @@ -1,60 +1,41 @@ package com.orgmemory.core.assetregistry; -import com.orgmemory.core.assetregistry.consumption.AssetConsumptionRelease; - -import com.orgmemory.core.assetregistry.api.AssetIdentity; -import com.orgmemory.core.assetregistry.api.AssetIdentityQuery; -import com.orgmemory.core.assetregistry.api.AssetNotFoundException; -import com.orgmemory.core.assetregistry.api.AssetType; import com.orgmemory.core.assetregistry.api.AssetUnavailableException; +import com.orgmemory.core.assetregistry.consumption.AssetConsumptionRelease; +import com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseContent; +import com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseDeliveryQuery; +import com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseDescriptor; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact; import com.orgmemory.core.organization.CurrentActor; -import java.util.Objects; -import java.util.regex.Pattern; import java.util.UUID; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; -/** - * Canonical authenticated Skill distribution boundary. - */ +/** Canonical authenticated Skill distribution boundary. */ @Service public class SkillDistributionService { private static final Logger log = LoggerFactory.getLogger(SkillDistributionService.class); - private static final Pattern COORDINATE = - Pattern.compile("[a-z0-9]+(?:[._-][a-z0-9]+)*"); - private final AssetRegistryService assets; - private final AssetIdentityQuery identities; - private final AssetReleaseRepository releaseRepository; - private final AssetPayloadReferenceRepository references; + private final SkillReleaseDeliveryQuery deliveries; private final SkillPackageSpecReader specs; - private final SkillPackageStoragePort storage; SkillDistributionService( - AssetRegistryService assets, - AssetIdentityQuery identities, - AssetReleaseRepository releaseRepository, - AssetPayloadReferenceRepository references, - SkillPackageSpecReader specs, - SkillPackageStoragePort storage) { - this.assets = assets; - this.identities = identities; - this.releaseRepository = releaseRepository; - this.references = references; + SkillReleaseDeliveryQuery deliveries, + SkillPackageSpecReader specs) { + this.deliveries = deliveries; this.specs = specs; - this.storage = storage; } public SkillInstallManifest manifest( - CurrentActor actor, - UUID assetId, - UUID releaseId) { - ResolvedSkill resolved = resolve(actor, assetId, releaseId); + CurrentActor actor, UUID assetId, UUID releaseId) { + SkillReleaseDescriptor descriptor = + deliveries.describe(actor, assetId, releaseId); + SkillInstallManifest manifest = manifest(descriptor); audit(actor, "get_skill_manifest", assetId, releaseId); - return resolved.manifest(); + return manifest; } public SkillInstallManifest manifest( @@ -62,48 +43,26 @@ public SkillInstallManifest manifest( String namespace, String slug, String version) { - Objects.requireNonNull(actor, "actor"); - AssetIdentity asset = identities - .findByCoordinate( - actor.organizationId(), - normalizeCoordinate(namespace, "namespace"), - normalizeCoordinate(slug, "slug")) - .filter(value -> value.type() == AssetType.SKILL) - .orElseThrow(AssetNotFoundException::new); - String versionLabel; - try { - versionLabel = AssetRelease.validateVersionLabel(version); - } catch (IllegalArgumentException | NullPointerException invalid) { - throw new AssetNotFoundException(invalid); - } - AssetRelease release = releaseRepository - .findByAssetIdAndOrganizationIdAndVersionLabel( - asset.id(), - actor.organizationId(), - versionLabel) - .orElseThrow(AssetNotFoundException::new); - return manifest(actor, asset.id(), release.getId()); + SkillReleaseDescriptor descriptor = + deliveries.describe(actor, namespace, slug, version); + SkillInstallManifest manifest = manifest(descriptor); + audit( + actor, + "get_skill_manifest", + descriptor.release().assetId(), + descriptor.release().releaseId()); + return manifest; } public SkillPackageContent open( - CurrentActor actor, - UUID assetId, - UUID releaseId) { - ResolvedSkill resolved = resolve(actor, assetId, releaseId); - SkillPackageStoragePort.StoredSkillPackageContent content; - try { - content = storage.open(resolved.reference().getReferenceValue()); - } catch (RuntimeException unavailable) { - throw new AssetUnavailableException( - "The Skill package is temporarily unavailable", - unavailable); - } + CurrentActor actor, UUID assetId, UUID releaseId) { + SkillReleaseContent content = deliveries.open(actor, assetId, releaseId); try { - verifyStored(resolved.reference(), content.metadata()); + SkillInstallManifest manifest = manifest(content.descriptor()); audit(actor, "download_skill_package", assetId, releaseId); return new SkillPackageContent( - resolved.manifest(), - fileName(resolved.manifest()), + manifest, + fileName(manifest), content.content()); } catch (RuntimeException invalid) { try { @@ -115,28 +74,17 @@ public SkillPackageContent open( } } - private ResolvedSkill resolve( - CurrentActor actor, - UUID assetId, - UUID releaseId) { - Objects.requireNonNull(actor, "actor"); - AssetConsumptionRelease release = - assets.releaseForUse(actor, assetId, releaseId, AssetType.SKILL); + private SkillInstallManifest manifest(SkillReleaseDescriptor descriptor) { + AssetConsumptionRelease release = descriptor.release(); SkillPackageSpec spec; try { spec = specs.read(release.payload()); } catch (RuntimeException invalid) { throw new AssetUnavailableException( - "The Skill release manifest is unavailable", - invalid); + "The Skill release manifest is unavailable", invalid); } - AssetPayloadReference reference = references - .findByReleaseIdAndOrganizationId( - releaseId, actor.organizationId()) - .orElseThrow(() -> new AssetUnavailableException( - "The Skill release package is unavailable")); - verifyReference(spec, reference); - SkillInstallManifest manifest = new SkillInstallManifest( + verifyReference(spec, descriptor.artifact()); + return new SkillInstallManifest( release.assetId(), release.releaseId(), release.namespace(), @@ -156,58 +104,26 @@ private ResolvedSkill resolve( spec.metadata(), spec.files().stream() .map(file -> new SkillInstallManifest.File( - file.path(), - file.size(), - file.sha256())) + file.path(), file.size(), file.sha256())) .toList()); - return new ResolvedSkill(manifest, reference); } private static void verifyReference( - SkillPackageSpec spec, - AssetPayloadReference reference) { - if (!reference.isBlobReference() - || !spec.artifact().sha256().equals(reference.getDigest()) - || spec.artifact().contentLength() - != reference.getContentLength() - || !spec.artifact().mediaType() - .equals(reference.getMediaType())) { + SkillPackageSpec spec, SkillPackageArtifact artifact) { + if (!spec.artifact().sha256().equals(artifact.sha256()) + || spec.artifact().contentLength() != artifact.contentLength() + || !spec.artifact().mediaType().equals(artifact.mediaType())) { throw new AssetUnavailableException( "The Skill release package metadata is inconsistent"); } } - private static void verifyStored( - AssetPayloadReference reference, - SkillPackageStoragePort.StoredSkillPackage stored) { - if (!reference.getReferenceValue().equals(stored.objectKey()) - || !reference.getDigest().equals(stored.sha256()) - || reference.getContentLength() != stored.contentLength() - || !reference.getMediaType().equals(stored.mediaType())) { - throw new AssetUnavailableException( - "The stored Skill package failed its integrity check"); - } - } - private static String fileName(SkillInstallManifest manifest) { String version = manifest.version() .replaceAll("[^A-Za-z0-9._-]", "-"); return manifest.slug() + "-" + version + ".zip"; } - private static String normalizeCoordinate( - String value, String field) { - String normalized = Objects.requireNonNull(value, field) - .strip() - .toLowerCase(java.util.Locale.ROOT); - if (normalized.isEmpty() - || normalized.length() > 128 - || !COORDINATE.matcher(normalized).matches()) { - throw new AssetNotFoundException(); - } - return normalized; - } - private static void audit( CurrentActor actor, String action, @@ -221,9 +137,4 @@ private static void audit( assetId, releaseId); } - - private record ResolvedSkill( - SkillInstallManifest manifest, - AssetPayloadReference reference) { - } } diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageAssetService.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageAssetService.java new file mode 100644 index 00000000..318639a9 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageAssetService.java @@ -0,0 +1,185 @@ +package com.orgmemory.core.assetregistry; + +import com.orgmemory.core.assetregistry.api.AssetNotFoundException; +import com.orgmemory.core.assetregistry.api.AssetType; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageAssetCommand; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackagePayloadPolicy; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageUpload; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; +import com.orgmemory.core.organization.CurrentActor; +import com.orgmemory.core.permission.KnowledgeClassification; +import java.util.Objects; +import java.util.UUID; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Service; + +@Service +class SkillPackageAssetService implements SkillPackageAssetCommand { + + private static final Logger LOGGER = + LoggerFactory.getLogger(SkillPackageAssetService.class); + + private final SkillPackageStoragePort storage; + private final SkillPackagePayloadPolicy payloadPolicy; + private final AssetRegistryService assets; + private final SkillPackageSupersessionCleanupService cleanup; + + SkillPackageAssetService( + SkillPackageStoragePort storage, + SkillPackagePayloadPolicy payloadPolicy, + AssetRegistryService assets, + SkillPackageSupersessionCleanupService cleanup) { + this.storage = storage; + this.payloadPolicy = payloadPolicy; + this.assets = assets; + this.cleanup = cleanup; + } + + @Override + public void requireCreate(CurrentActor actor, UUID knowledgeSpaceId) { + assets.requireSkillCreate(actor, knowledgeSpaceId); + } + + @Override + public KnowledgeClassification requireEdit( + CurrentActor actor, UUID assetId) { + assets.requireSkillEdit(actor, assetId); + AssetView current = assets.get(actor, assetId); + if (current.type() != AssetType.SKILL || current.draft() == null) { + throw new AssetNotFoundException(); + } + return KnowledgeClassification.valueOf( + current.draft().classification()); + } + + @Override + public UUID importPackage( + CurrentActor actor, + String namespace, + UUID knowledgeSpaceId, + KnowledgeClassification classification, + SkillPackageUpload upload) { + Objects.requireNonNull(actor, "actor"); + Objects.requireNonNull(classification, "classification"); + Objects.requireNonNull(upload, "upload"); + requireCreate(actor, knowledgeSpaceId); + payloadPolicy.validate(upload.payload(), upload.artifact()); + SkillPackageStoragePort.StoredSkillPackage stored = null; + UUID assetId = null; + try { + stored = store(actor, upload); + SkillPackageArtifact artifact = artifact(stored); + requireMatchingArtifact(upload.artifact(), artifact); + assetId = assets.createValidatedSkillIdentity( + actor, + namespace, + upload.slug(), + knowledgeSpaceId, + draft(upload, classification), + stored); + assets.projectCreated(actor, assetId); + return assetId; + } catch (RuntimeException failure) { + if (assetId == null) { + deleteIfStored(stored, failure); + } + throw failure; + } + } + + @Override + public UUID replacePackage( + CurrentActor actor, + UUID assetId, + long expectedLockVersion, + SkillPackageUpload upload) { + Objects.requireNonNull(actor, "actor"); + Objects.requireNonNull(assetId, "assetId"); + Objects.requireNonNull(upload, "upload"); + KnowledgeClassification classification = requireEdit(actor, assetId); + payloadPolicy.validate(upload.payload(), upload.artifact()); + SkillPackageStoragePort.StoredSkillPackage stored = null; + SkillDraftReplacement replacement = null; + try { + stored = store(actor, upload); + SkillPackageArtifact artifact = artifact(stored); + requireMatchingArtifact(upload.artifact(), artifact); + replacement = assets.replaceValidatedSkillDraft( + actor, + assetId, + expectedLockVersion, + draft(upload, classification), + stored); + } catch (RuntimeException failure) { + if (replacement == null) { + deleteIfStored(stored, failure); + } + throw failure; + } + cleanupAfterCommit(replacement.supersessionId()); + return assetId; + } + + private SkillPackageStoragePort.StoredSkillPackage store( + CurrentActor actor, SkillPackageUpload upload) { + return storage.put( + new SkillPackageStoragePort.SkillPackageWriteRequest( + actor.organizationId(), + upload.packageId(), + upload.artifact().contentLength(), + upload.artifact().sha256(), + upload.storageMetadata()), + upload.content()); + } + + private static AssetDraftInput draft( + SkillPackageUpload upload, + KnowledgeClassification classification) { + return new AssetDraftInput( + upload.title(), + upload.summary(), + classification.name(), + upload.schemaVersion(), + upload.payload()); + } + + private static SkillPackageArtifact artifact( + SkillPackageStoragePort.StoredSkillPackage stored) { + return new SkillPackageArtifact( + stored.sha256(), stored.contentLength(), stored.mediaType()); + } + + private static void requireMatchingArtifact( + SkillPackageArtifact expected, SkillPackageArtifact stored) { + if (!expected.equals(stored)) { + throw new IllegalArgumentException( + "Stored Skill package metadata does not match inspected bytes"); + } + } + + private void cleanupAfterCommit(UUID supersessionId) { + try { + cleanup.cleanup(supersessionId); + } catch (RuntimeException failure) { + LOGGER.warn( + "Skill package supersession cleanup remains pending for {} ({})", + supersessionId, + failure.getClass().getSimpleName()); + } + } + + private void deleteIfStored( + SkillPackageStoragePort.StoredSkillPackage stored, + Throwable failure) { + if (stored == null) { + return; + } + try { + storage.delete(stored.objectKey()); + } catch (RuntimeException cleanupFailure) { + failure.addSuppressed(cleanupFailure); + } + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageCleanupOutcome.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageCleanupOutcome.java index 0cce4200..2bb4649e 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageCleanupOutcome.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageCleanupOutcome.java @@ -1,6 +1,6 @@ package com.orgmemory.core.assetregistry; -public enum SkillPackageCleanupOutcome { +enum SkillPackageCleanupOutcome { DELETED, RETAINED_BY_IMMUTABLE_REFERENCE, RETRY_SCHEDULED, diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.java index 27178a36..a71d043c 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.java @@ -1,15 +1,19 @@ package com.orgmemory.core.assetregistry; -import com.orgmemory.core.assetregistry.profile.AssetPayloadProfile; - import com.orgmemory.core.assetregistry.api.AssetType; +import com.orgmemory.core.assetregistry.profile.AssetPayloadProfile; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackagePayloadPolicy; import java.util.Set; import org.springframework.stereotype.Component; import tools.jackson.databind.json.JsonMapper; import tools.jackson.databind.ObjectMapper; @Component -class SkillPackageProfile implements AssetPayloadProfile, SkillPackageSpecReader { +class SkillPackageProfile implements + AssetPayloadProfile, + SkillPackageSpecReader, + SkillPackagePayloadPolicy { static final String SCHEMA_VERSION = "2"; @@ -30,6 +34,18 @@ public void validate(String payload) { read(payload); } + @Override + public void validate( + String canonicalPayload, SkillPackageArtifact artifact) { + SkillPackageSpec spec = read(canonicalPayload); + if (!spec.artifact().sha256().equals(artifact.sha256()) + || spec.artifact().contentLength() != artifact.contentLength() + || !spec.artifact().mediaType().equals(artifact.mediaType())) { + throw new SkillPackageValidationException( + "The Skill package artifact does not match its canonical payload"); + } + } + @Override public SkillPackageSpec read(String payload) { try { diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinator.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinator.java index 0fa781ca..440adc6c 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinator.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinator.java @@ -1,5 +1,6 @@ package com.orgmemory.core.assetregistry; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import java.time.Instant; import java.util.UUID; import org.springframework.stereotype.Service; diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupService.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupService.java index ed49886f..fac30870 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupService.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupService.java @@ -1,5 +1,7 @@ package com.orgmemory.core.assetregistry; +import com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupOperations; +import com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupSummary; import java.time.Instant; import java.util.EnumMap; import java.util.Map; @@ -8,7 +10,8 @@ import org.springframework.stereotype.Service; @Service -public class SkillPackageSupersessionCleanupService { +class SkillPackageSupersessionCleanupService + implements SkillPackageCleanupOperations { private final SkillPackageSupersessionRepository supersessions; private final SkillPackageSupersessionCleanupCoordinator coordinator; @@ -20,11 +23,12 @@ public class SkillPackageSupersessionCleanupService { this.coordinator = coordinator; } - public SkillPackageCleanupOutcome cleanup(UUID supersessionId) { + SkillPackageCleanupOutcome cleanup(UUID supersessionId) { return coordinator.cleanup(supersessionId); } - public Map cleanupPending(int limit) { + @Override + public SkillPackageCleanupSummary cleanupPending(int limit) { int boundedLimit = Math.min(Math.max(limit, 1), 100); Map outcomes = new EnumMap<>(SkillPackageCleanupOutcome.class); @@ -33,6 +37,14 @@ public Map cleanupPending(int limit) { PageRequest.of(0, boundedLimit))) { outcomes.merge(coordinator.cleanup(id), 1, Integer::sum); } - return Map.copyOf(outcomes); + return new SkillPackageCleanupSummary( + outcomes.getOrDefault(SkillPackageCleanupOutcome.DELETED, 0), + outcomes.getOrDefault( + SkillPackageCleanupOutcome.RETAINED_BY_IMMUTABLE_REFERENCE, + 0), + outcomes.getOrDefault( + SkillPackageCleanupOutcome.RETRY_SCHEDULED, 0), + outcomes.getOrDefault( + SkillPackageCleanupOutcome.ALREADY_RESOLVED, 0)); } } diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.java index dec0514d..fc3d3755 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.java @@ -1,41 +1,36 @@ package com.orgmemory.core.assetregistry; -import com.orgmemory.core.assetregistry.api.AssetNotFoundException; -import com.orgmemory.core.assetregistry.api.AssetType; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageAssetCommand; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageUpload; import com.orgmemory.core.organization.CurrentActor; import com.orgmemory.core.permission.KnowledgeClassification; import com.orgmemory.core.shared.error.BusinessUnavailableException; -import java.io.InputStream; import java.io.IOException; +import java.io.InputStream; import java.util.Map; import java.util.Objects; import java.util.UUID; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; -import tools.jackson.databind.json.JsonMapper; +import tools.jackson.core.JacksonException; import tools.jackson.databind.ObjectMapper; +import tools.jackson.databind.json.JsonMapper; @Service public class SkillRegistryService { - private static final Logger LOGGER = LoggerFactory.getLogger(SkillRegistryService.class); - private final SkillPackageInspector inspector; - private final SkillPackageStoragePort storage; + private final SkillPackageAssetCommand packages; private final AssetRegistryService assets; - private final SkillPackageSupersessionCleanupService cleanup; private final ObjectMapper json = JsonMapper.builder().build(); SkillRegistryService( SkillPackageInspector inspector, - SkillPackageStoragePort storage, - AssetRegistryService assets, - SkillPackageSupersessionCleanupService cleanup) { + SkillPackageAssetCommand packages, + AssetRegistryService assets) { this.inspector = inspector; - this.storage = storage; + this.packages = packages; this.assets = assets; - this.cleanup = cleanup; } public SkillPackageInspection inspectPackage( @@ -84,39 +79,23 @@ AssetView importPackage( SkillPackageSpec.Origin origin) { Objects.requireNonNull(actor, "actor"); Objects.requireNonNull(classification, "classification"); - assets.requireSkillCreate(actor, knowledgeSpaceId); - UUID packageId = UUID.randomUUID(); - SkillPackageStoragePort.StoredSkillPackage stored = null; - UUID assetId = null; + packages.requireCreate(actor, knowledgeSpaceId); try (SkillPackageInspector.StagedSkillPackage staged = - inspector.inspect(content, contentLength)) { - try (InputStream packageContent = staged.open()) { - stored = storage.put( - new SkillPackageStoragePort.SkillPackageWriteRequest( - actor.organizationId(), - packageId, - staged.contentLength(), - staged.sha256(), - Map.of("skill-name", staged.metadata().name())), - packageContent); - } - SkillPackageSpec spec = specification(staged, stored, origin); - AssetDraftInput draft = draft(spec, classification); - assetId = assets.createValidatedSkillIdentity( + inspector.inspect(content, contentLength); + InputStream packageContent = staged.open()) { + SkillPackageArtifact artifact = new SkillPackageArtifact( + staged.sha256(), + staged.contentLength(), + SkillPackageArtifact.ZIP_MEDIA_TYPE); + SkillPackageSpec spec = specification(staged, artifact, origin); + UUID assetId = packages.importPackage( actor, namespace, - spec.name(), knowledgeSpaceId, - draft, - stored); - return assets.projectCreated(actor, assetId); - } catch (RuntimeException failure) { - if (assetId == null) { - deleteIfStored(stored, failure); - } - throw failure; + classification, + upload(spec, artifact, packageContent)); + return assets.get(actor, assetId); } catch (IOException failure) { - deleteIfStored(stored, failure); throw new BusinessUnavailableException( "skill.package-staging-unavailable", "The Skill package could not be staged", @@ -132,53 +111,32 @@ public AssetView replacePackage( InputStream content) { Objects.requireNonNull(actor, "actor"); Objects.requireNonNull(assetId, "assetId"); - assets.requireSkillEdit(actor, assetId); - AssetView current = assets.get(actor, assetId); - if (current.type() != AssetType.SKILL || current.draft() == null) { - throw new AssetNotFoundException(); - } - KnowledgeClassification classification = KnowledgeClassification.valueOf( - current.draft().classification()); - SkillPackageStoragePort.StoredSkillPackage stored = null; - SkillDraftReplacement replacement = null; + packages.requireEdit(actor, assetId); try (SkillPackageInspector.StagedSkillPackage staged = - inspector.inspect(content, contentLength)) { - try (InputStream packageContent = staged.open()) { - stored = storage.put( - new SkillPackageStoragePort.SkillPackageWriteRequest( - actor.organizationId(), - UUID.randomUUID(), - staged.contentLength(), - staged.sha256(), - Map.of("skill-name", staged.metadata().name())), - packageContent); - } - SkillPackageSpec spec = specification(staged, stored, null); - replacement = assets.replaceValidatedSkillDraft( + inspector.inspect(content, contentLength); + InputStream packageContent = staged.open()) { + SkillPackageArtifact artifact = new SkillPackageArtifact( + staged.sha256(), + staged.contentLength(), + SkillPackageArtifact.ZIP_MEDIA_TYPE); + SkillPackageSpec spec = specification(staged, artifact, null); + UUID replacedId = packages.replacePackage( actor, assetId, expectedLockVersion, - draft(spec, classification), - stored); - } catch (RuntimeException failure) { - if (replacement == null) { - deleteIfStored(stored, failure); - } - throw failure; + upload(spec, artifact, packageContent)); + return assets.get(actor, replacedId); } catch (IOException failure) { - deleteIfStored(stored, failure); throw new BusinessUnavailableException( "skill.package-staging-unavailable", "The Skill package could not be staged", failure); } - cleanupAfterCommit(replacement.supersessionId()); - return replacement.asset(); } private SkillPackageSpec specification( SkillPackageInspector.StagedSkillPackage staged, - SkillPackageStoragePort.StoredSkillPackage stored, + SkillPackageArtifact artifact, SkillPackageSpec.Origin origin) { return new SkillPackageSpec( staged.metadata().name(), @@ -189,40 +147,34 @@ private SkillPackageSpec specification( staged.metadata().metadata(), origin, new SkillPackageSpec.Artifact( - stored.sha256(), stored.contentLength(), stored.mediaType()), + artifact.sha256(), + artifact.contentLength(), + artifact.mediaType()), staged.files()); } - private AssetDraftInput draft( - SkillPackageSpec spec, KnowledgeClassification classification) { - return new AssetDraftInput( + private SkillPackageUpload upload( + SkillPackageSpec spec, + SkillPackageArtifact artifact, + InputStream content) { + String payload; + try { + payload = json.writeValueAsString(spec); + } catch (JacksonException failure) { + throw new BusinessUnavailableException( + "skill.package-staging-unavailable", + "The Skill package could not be staged", + failure); + } + return new SkillPackageUpload( + UUID.randomUUID(), + spec.name(), spec.name(), spec.description(), - classification.name(), SkillPackageProfile.SCHEMA_VERSION, - json.writeValueAsString(spec)); - } - - private void cleanupAfterCommit(UUID supersessionId) { - try { - cleanup.cleanup(supersessionId); - } catch (RuntimeException failure) { - LOGGER.warn( - "Skill package supersession cleanup remains pending for {} ({})", - supersessionId, - failure.getClass().getSimpleName()); - } - } - - private void deleteIfStored( - SkillPackageStoragePort.StoredSkillPackage stored, Throwable failure) { - if (stored == null) { - return; - } - try { - storage.delete(stored.objectKey()); - } catch (RuntimeException cleanupFailure) { - failure.addSuppressed(cleanupFailure); - } + payload, + artifact, + Map.of("skill-name", spec.name()), + content); } } diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillReleaseDeliveryService.java b/core/src/main/java/com/orgmemory/core/assetregistry/SkillReleaseDeliveryService.java new file mode 100644 index 00000000..1f89c9c2 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/SkillReleaseDeliveryService.java @@ -0,0 +1,155 @@ +package com.orgmemory.core.assetregistry; + +import com.orgmemory.core.assetregistry.api.AssetIdentity; +import com.orgmemory.core.assetregistry.api.AssetIdentityQuery; +import com.orgmemory.core.assetregistry.api.AssetNotFoundException; +import com.orgmemory.core.assetregistry.api.AssetType; +import com.orgmemory.core.assetregistry.api.AssetUnavailableException; +import com.orgmemory.core.assetregistry.consumption.AssetConsumptionRelease; +import com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseContent; +import com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseDeliveryQuery; +import com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseDescriptor; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; +import com.orgmemory.core.organization.CurrentActor; +import java.util.Locale; +import java.util.Objects; +import java.util.UUID; +import java.util.regex.Pattern; +import org.springframework.stereotype.Service; + +@Service +class SkillReleaseDeliveryService implements SkillReleaseDeliveryQuery { + + private static final Pattern COORDINATE = + Pattern.compile("[a-z0-9]+(?:[._-][a-z0-9]+)*"); + + private final AssetRegistryService assets; + private final AssetIdentityQuery identities; + private final AssetReleaseRepository releases; + private final AssetPayloadReferenceRepository references; + private final SkillPackageStoragePort storage; + + SkillReleaseDeliveryService( + AssetRegistryService assets, + AssetIdentityQuery identities, + AssetReleaseRepository releases, + AssetPayloadReferenceRepository references, + SkillPackageStoragePort storage) { + this.assets = assets; + this.identities = identities; + this.releases = releases; + this.references = references; + this.storage = storage; + } + + @Override + public SkillReleaseDescriptor describe( + CurrentActor actor, UUID assetId, UUID releaseId) { + return resolve(actor, assetId, releaseId).descriptor(); + } + + @Override + public SkillReleaseDescriptor describe( + CurrentActor actor, + String namespace, + String slug, + String version) { + Objects.requireNonNull(actor, "actor"); + AssetIdentity asset = identities + .findByCoordinate( + actor.organizationId(), + normalizeCoordinate(namespace, "namespace"), + normalizeCoordinate(slug, "slug")) + .filter(value -> value.type() == AssetType.SKILL) + .orElseThrow(AssetNotFoundException::new); + String versionLabel; + try { + versionLabel = AssetRelease.validateVersionLabel(version); + } catch (IllegalArgumentException | NullPointerException invalid) { + throw new AssetNotFoundException(invalid); + } + AssetRelease release = releases + .findByAssetIdAndOrganizationIdAndVersionLabel( + asset.id(), actor.organizationId(), versionLabel) + .orElseThrow(AssetNotFoundException::new); + return describe(actor, asset.id(), release.getId()); + } + + @Override + public SkillReleaseContent open( + CurrentActor actor, UUID assetId, UUID releaseId) { + ResolvedRelease resolved = resolve(actor, assetId, releaseId); + SkillPackageStoragePort.StoredSkillPackageContent content; + try { + content = storage.open(resolved.reference().getReferenceValue()); + } catch (RuntimeException unavailable) { + throw new AssetUnavailableException( + "The Skill package is temporarily unavailable", unavailable); + } + try { + verifyStored(resolved.reference(), content.metadata()); + return new SkillReleaseContent( + resolved.descriptor(), content.content()); + } catch (RuntimeException invalid) { + try { + content.close(); + } catch (Exception closeFailure) { + invalid.addSuppressed(closeFailure); + } + throw invalid; + } + } + + private ResolvedRelease resolve( + CurrentActor actor, UUID assetId, UUID releaseId) { + Objects.requireNonNull(actor, "actor"); + AssetConsumptionRelease release = + assets.releaseForUse(actor, assetId, releaseId, AssetType.SKILL); + AssetPayloadReference reference = references + .findByReleaseIdAndOrganizationId( + releaseId, actor.organizationId()) + .orElseThrow(() -> new AssetUnavailableException( + "The Skill release package is unavailable")); + if (!reference.isBlobReference()) { + throw new AssetUnavailableException( + "The Skill release package metadata is inconsistent"); + } + SkillReleaseDescriptor descriptor = new SkillReleaseDescriptor( + release, + new SkillPackageArtifact( + reference.getDigest(), + reference.getContentLength(), + reference.getMediaType())); + return new ResolvedRelease(descriptor, reference); + } + + private static void verifyStored( + AssetPayloadReference reference, + SkillPackageStoragePort.StoredSkillPackage stored) { + if (!reference.getReferenceValue().equals(stored.objectKey()) + || !reference.getDigest().equals(stored.sha256()) + || reference.getContentLength() != stored.contentLength() + || !reference.getMediaType().equals(stored.mediaType())) { + throw new AssetUnavailableException( + "The stored Skill package failed its integrity check"); + } + } + + private static String normalizeCoordinate(String value, String field) { + String normalized = Objects.requireNonNull(value, field) + .strip() + .toLowerCase(Locale.ROOT); + if (normalized.isEmpty() + || normalized.length() > 128 + || !COORDINATE.matcher(normalized).matches()) { + throw new AssetNotFoundException(); + } + return normalized; + } + + private record ResolvedRelease( + SkillReleaseDescriptor descriptor, + AssetPayloadReference reference) { + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/SkillPackageCleanupOperations.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/SkillPackageCleanupOperations.java new file mode 100644 index 00000000..e54ab9af --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/SkillPackageCleanupOperations.java @@ -0,0 +1,6 @@ +package com.orgmemory.core.assetregistry.skillcleanup; + +public interface SkillPackageCleanupOperations { + + SkillPackageCleanupSummary cleanupPending(int limit); +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/SkillPackageCleanupSummary.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/SkillPackageCleanupSummary.java new file mode 100644 index 00000000..b05406d3 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/SkillPackageCleanupSummary.java @@ -0,0 +1,25 @@ +package com.orgmemory.core.assetregistry.skillcleanup; + +public record SkillPackageCleanupSummary( + int deleted, + int retainedByImmutableReference, + int retryScheduled, + int alreadyResolved) { + + public SkillPackageCleanupSummary { + if (deleted < 0 + || retainedByImmutableReference < 0 + || retryScheduled < 0 + || alreadyResolved < 0) { + throw new IllegalArgumentException( + "Skill package cleanup counts cannot be negative"); + } + } + + public boolean isEmpty() { + return deleted == 0 + && retainedByImmutableReference == 0 + && retryScheduled == 0 + && alreadyResolved == 0; + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/package-info.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/package-info.java new file mode 100644 index 00000000..b0f01af6 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillcleanup/package-info.java @@ -0,0 +1,2 @@ +@org.springframework.modulith.NamedInterface("skill-cleanup") +package com.orgmemory.core.assetregistry.skillcleanup; diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseContent.java b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseContent.java new file mode 100644 index 00000000..bd6bd9a0 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseContent.java @@ -0,0 +1,20 @@ +package com.orgmemory.core.assetregistry.skilldelivery; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Objects; + +public record SkillReleaseContent( + SkillReleaseDescriptor descriptor, + InputStream content) implements AutoCloseable { + + public SkillReleaseContent { + descriptor = Objects.requireNonNull(descriptor, "descriptor"); + content = Objects.requireNonNull(content, "content"); + } + + @Override + public void close() throws IOException { + content.close(); + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseDeliveryQuery.java b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseDeliveryQuery.java new file mode 100644 index 00000000..37624374 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseDeliveryQuery.java @@ -0,0 +1,19 @@ +package com.orgmemory.core.assetregistry.skilldelivery; + +import com.orgmemory.core.organization.CurrentActor; +import java.util.UUID; + +public interface SkillReleaseDeliveryQuery { + + SkillReleaseDescriptor describe( + CurrentActor actor, UUID assetId, UUID releaseId); + + SkillReleaseDescriptor describe( + CurrentActor actor, + String namespace, + String slug, + String version); + + SkillReleaseContent open( + CurrentActor actor, UUID assetId, UUID releaseId); +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseDescriptor.java b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseDescriptor.java new file mode 100644 index 00000000..6fee1ca6 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/SkillReleaseDescriptor.java @@ -0,0 +1,15 @@ +package com.orgmemory.core.assetregistry.skilldelivery; + +import com.orgmemory.core.assetregistry.consumption.AssetConsumptionRelease; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact; +import java.util.Objects; + +public record SkillReleaseDescriptor( + AssetConsumptionRelease release, + SkillPackageArtifact artifact) { + + public SkillReleaseDescriptor { + release = Objects.requireNonNull(release, "release"); + artifact = Objects.requireNonNull(artifact, "artifact"); + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/package-info.java b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/package-info.java new file mode 100644 index 00000000..c545e14a --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skilldelivery/package-info.java @@ -0,0 +1,2 @@ +@org.springframework.modulith.NamedInterface("skill-delivery") +package com.orgmemory.core.assetregistry.skilldelivery; diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageArtifact.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageArtifact.java new file mode 100644 index 00000000..f0047d6a --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageArtifact.java @@ -0,0 +1,39 @@ +package com.orgmemory.core.assetregistry.skillpackage; + +import java.util.Objects; + +public record SkillPackageArtifact( + String sha256, + long contentLength, + String mediaType) { + + public static final String ZIP_MEDIA_TYPE = "application/zip"; + + public SkillPackageArtifact { + sha256 = requireSha256(sha256); + if (contentLength <= 0) { + throw new IllegalArgumentException( + "Skill package content length must be positive"); + } + mediaType = requireText(mediaType, "mediaType", 128); + } + + private static String requireSha256(String value) { + String normalized = requireText(value, "sha256", 64); + if (!normalized.matches("[0-9a-f]{64}")) { + throw new IllegalArgumentException( + "sha256 must be lowercase hexadecimal"); + } + return normalized; + } + + private static String requireText( + String value, String field, int maximumLength) { + String normalized = Objects.requireNonNull(value, field).trim(); + if (normalized.isEmpty() || normalized.length() > maximumLength) { + throw new IllegalArgumentException( + field + " is blank or exceeds its limit"); + } + return normalized; + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageAssetCommand.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageAssetCommand.java new file mode 100644 index 00000000..72df9988 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageAssetCommand.java @@ -0,0 +1,25 @@ +package com.orgmemory.core.assetregistry.skillpackage; + +import com.orgmemory.core.organization.CurrentActor; +import com.orgmemory.core.permission.KnowledgeClassification; +import java.util.UUID; + +public interface SkillPackageAssetCommand { + + void requireCreate(CurrentActor actor, UUID knowledgeSpaceId); + + KnowledgeClassification requireEdit(CurrentActor actor, UUID assetId); + + UUID importPackage( + CurrentActor actor, + String namespace, + UUID knowledgeSpaceId, + KnowledgeClassification classification, + SkillPackageUpload upload); + + UUID replacePackage( + CurrentActor actor, + UUID assetId, + long expectedLockVersion, + SkillPackageUpload upload); +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackagePayloadPolicy.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackagePayloadPolicy.java new file mode 100644 index 00000000..3b669b31 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackagePayloadPolicy.java @@ -0,0 +1,6 @@ +package com.orgmemory.core.assetregistry.skillpackage; + +public interface SkillPackagePayloadPolicy { + + void validate(String canonicalPayload, SkillPackageArtifact artifact); +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageUpload.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageUpload.java new file mode 100644 index 00000000..a453d16f --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/SkillPackageUpload.java @@ -0,0 +1,32 @@ +package com.orgmemory.core.assetregistry.skillpackage; + +import java.io.InputStream; +import java.util.Map; +import java.util.Objects; +import java.util.UUID; + +public record SkillPackageUpload( + UUID packageId, + String slug, + String title, + String summary, + String schemaVersion, + String payload, + SkillPackageArtifact artifact, + Map storageMetadata, + InputStream content) { + + public SkillPackageUpload { + packageId = Objects.requireNonNull(packageId, "packageId"); + slug = Objects.requireNonNull(slug, "slug"); + title = Objects.requireNonNull(title, "title"); + summary = Objects.requireNonNull(summary, "summary"); + schemaVersion = Objects.requireNonNull(schemaVersion, "schemaVersion"); + payload = Objects.requireNonNull(payload, "payload"); + artifact = Objects.requireNonNull(artifact, "artifact"); + storageMetadata = storageMetadata == null + ? Map.of() + : Map.copyOf(storageMetadata); + content = Objects.requireNonNull(content, "content"); + } +} diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/package-info.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/package-info.java new file mode 100644 index 00000000..395a0e19 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillpackage/package-info.java @@ -0,0 +1,2 @@ +@org.springframework.modulith.NamedInterface("skill-package") +package com.orgmemory.core.assetregistry.skillpackage; diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageStoragePort.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillstorage/SkillPackageStoragePort.java similarity index 78% rename from core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageStoragePort.java rename to core/src/main/java/com/orgmemory/core/assetregistry/skillstorage/SkillPackageStoragePort.java index 96fe814f..fec8ac14 100644 --- a/core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageStoragePort.java +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillstorage/SkillPackageStoragePort.java @@ -1,7 +1,7 @@ -package com.orgmemory.core.assetregistry; +package com.orgmemory.core.assetregistry.skillstorage; -import java.io.InputStream; import java.io.IOException; +import java.io.InputStream; import java.util.Map; import java.util.UUID; @@ -25,7 +25,8 @@ record SkillPackageWriteRequest( organizationId, "organizationId"); packageId = java.util.Objects.requireNonNull(packageId, "packageId"); if (contentLength <= 0) { - throw new IllegalArgumentException("Skill package content length must be positive"); + throw new IllegalArgumentException( + "Skill package content length must be positive"); } expectedSha256 = requireSha256(expectedSha256); metadata = metadata == null ? Map.of() : Map.copyOf(metadata); @@ -41,7 +42,8 @@ record StoredSkillPackage( public StoredSkillPackage { objectKey = requireText(objectKey, "objectKey", 1024); if (contentLength <= 0) { - throw new IllegalArgumentException("Stored Skill package length must be positive"); + throw new IllegalArgumentException( + "Stored Skill package length must be positive"); } mediaType = requireText(mediaType, "mediaType", 128); sha256 = requireSha256(sha256); @@ -67,15 +69,18 @@ public void close() throws IOException { private static String requireSha256(String value) { String normalized = requireText(value, "sha256", 64); if (!normalized.matches("[0-9a-f]{64}")) { - throw new IllegalArgumentException("sha256 must be lowercase hexadecimal"); + throw new IllegalArgumentException( + "sha256 must be lowercase hexadecimal"); } return normalized; } - private static String requireText(String value, String field, int maximumLength) { + private static String requireText( + String value, String field, int maximumLength) { String normalized = java.util.Objects.requireNonNull(value, field).trim(); if (normalized.isEmpty() || normalized.length() > maximumLength) { - throw new IllegalArgumentException(field + " is blank or exceeds its limit"); + throw new IllegalArgumentException( + field + " is blank or exceeds its limit"); } return normalized; } diff --git a/core/src/main/java/com/orgmemory/core/assetregistry/skillstorage/package-info.java b/core/src/main/java/com/orgmemory/core/assetregistry/skillstorage/package-info.java new file mode 100644 index 00000000..b7fb2be1 --- /dev/null +++ b/core/src/main/java/com/orgmemory/core/assetregistry/skillstorage/package-info.java @@ -0,0 +1,2 @@ +@org.springframework.modulith.NamedInterface("skill-storage") +package com.orgmemory.core.assetregistry.skillstorage; diff --git a/core/src/test/java/com/orgmemory/core/ModulithVerificationTests.java b/core/src/test/java/com/orgmemory/core/ModulithVerificationTests.java index a5204985..b7900bfa 100644 --- a/core/src/test/java/com/orgmemory/core/ModulithVerificationTests.java +++ b/core/src/test/java/com/orgmemory/core/ModulithVerificationTests.java @@ -1190,6 +1190,124 @@ void assetRegistryConsumptionIsAnExactExplicitNamedInterface() { exposedTypes); } + @Test + void assetRegistrySkillCapabilitiesAreExactExplicitNamedInterfaces() { + var assetRegistry = modules.getModuleByName("assetregistry").orElseThrow(); + + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact", + "com.orgmemory.core.assetregistry.skillpackage.SkillPackageAssetCommand", + "com.orgmemory.core.assetregistry.skillpackage.SkillPackagePayloadPolicy", + "com.orgmemory.core.assetregistry.skillpackage.SkillPackageUpload"), + assetRegistry.getNamedInterfaces() + .getByName("skill-package") + .orElseThrow() + .asJavaClasses() + .map(type -> type.getName()) + .collect(TreeSet::new, Set::add, Set::addAll)); + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseContent", + "com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseDeliveryQuery", + "com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseDescriptor"), + assetRegistry.getNamedInterfaces() + .getByName("skill-delivery") + .orElseThrow() + .asJavaClasses() + .map(type -> type.getName()) + .collect(TreeSet::new, Set::add, Set::addAll)); + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupOperations", + "com.orgmemory.core.assetregistry.skillcleanup.SkillPackageCleanupSummary"), + assetRegistry.getNamedInterfaces() + .getByName("skill-cleanup") + .orElseThrow() + .asJavaClasses() + .map(type -> type.getName()) + .collect(TreeSet::new, Set::add, Set::addAll)); + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort", + "com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort$SkillPackageWriteRequest", + "com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort$StoredSkillPackage", + "com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort$StoredSkillPackageContent"), + assetRegistry.getNamedInterfaces() + .getByName("skill-storage") + .orElseThrow() + .asJavaClasses() + .map(type -> type.getName()) + .collect(TreeSet::new, Set::add, Set::addAll)); + } + + @Test + void assetRegistrySkillCapabilitiesHaveExactCoreConsumers() { + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.SkillDistributionService", + "com.orgmemory.core.assetregistry.SkillPackageAssetService", + "com.orgmemory.core.assetregistry.SkillPackageProfile", + "com.orgmemory.core.assetregistry.SkillRegistryService", + "com.orgmemory.core.assetregistry.SkillReleaseDeliveryService", + "com.orgmemory.core.assetregistry.skilldelivery.SkillReleaseDescriptor"), + directConsumersOf( + "com.orgmemory.core.assetregistry.skillpackage")); + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.SkillDistributionService", + "com.orgmemory.core.assetregistry.SkillReleaseDeliveryService"), + directConsumersOf( + "com.orgmemory.core.assetregistry.skilldelivery")); + assertEquals( + Set.of("com.orgmemory.core.assetregistry.SkillPackageSupersessionCleanupService"), + directConsumersOf( + "com.orgmemory.core.assetregistry.skillcleanup")); + assertEquals( + Set.of( + "com.orgmemory.core.assetregistry.AssetPayloadReference", + "com.orgmemory.core.assetregistry.AssetRegistryCoordinator", + "com.orgmemory.core.assetregistry.AssetRegistryService", + "com.orgmemory.core.assetregistry.SkillPackageAssetService", + "com.orgmemory.core.assetregistry.SkillPackageSupersessionCleanupCoordinator", + "com.orgmemory.core.assetregistry.SkillReleaseDeliveryService"), + directConsumersOf( + "com.orgmemory.core.assetregistry.skillstorage")); + } + + @Test + void assetRegistrySkillCapabilityImplementationsRemainInternal() { + for (String implementation : Set.of( + "com.orgmemory.core.assetregistry.SkillPackageAssetService", + "com.orgmemory.core.assetregistry.SkillPackageSupersessionCleanupService", + "com.orgmemory.core.assetregistry.SkillReleaseDeliveryService")) { + assertFalse(Modifier.isPublic( + loadClass(implementation).getModifiers())); + } + } + + private static Class loadClass(String name) { + try { + return Class.forName(name); + } catch (ClassNotFoundException missing) { + throw new AssertionError(missing); + } + } + + private static Set directConsumersOf(String targetPackage) { + return new ClassFileImporter() + .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) + .importPackages("com.orgmemory.core") + .stream() + .filter(type -> !type.getPackageName().equals(targetPackage)) + .filter(type -> type.getDirectDependenciesFromSelf().stream() + .anyMatch(dependency -> dependency.getTargetClass() + .getPackageName() + .equals(targetPackage))) + .map(type -> type.getName().replaceFirst("\\$.*$", "")) + .collect(TreeSet::new, Set::add, Set::addAll); + } + @Test void assetRegistryPromptIsAClosedProfileModule() { var prompt = modules.getModuleByName("assetregistry.prompt").orElseThrow(); diff --git a/core/src/test/java/com/orgmemory/core/assetregistry/SkillDistributionServiceTests.java b/core/src/test/java/com/orgmemory/core/assetregistry/SkillDistributionServiceTests.java index eb969468..6529d2b8 100644 --- a/core/src/test/java/com/orgmemory/core/assetregistry/SkillDistributionServiceTests.java +++ b/core/src/test/java/com/orgmemory/core/assetregistry/SkillDistributionServiceTests.java @@ -5,6 +5,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import com.orgmemory.core.assetregistry.api.AssetIdentity; @@ -16,6 +17,7 @@ import com.orgmemory.core.assetregistry.consumption.AssetAvailability; import com.orgmemory.core.assetregistry.consumption.AssetConsumptionRelease; import com.orgmemory.core.assetregistry.consumption.AssetPublicationMode; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.organization.CurrentActor; import java.io.ByteArrayInputStream; import java.util.List; @@ -79,6 +81,60 @@ void closesAndRejectsStoredBytesWhoseMetadataNoLongerMatchesTheRelease() { assertTrue(stream.closed); } + @Test + void closesContentWhenTheCanonicalPayloadDoesNotMatchThePinnedReference() { + Fixture fixture = fixture(); + TrackingInputStream stream = storedContent(fixture); + when(fixture.specs.read("{\"profile\":\"skill\"}")) + .thenReturn(spec("b".repeat(64))); + + assertThrows( + AssetUnavailableException.class, + () -> fixture.service.open(ACTOR, ASSET_ID, RELEASE_ID)); + + assertTrue(stream.closed); + } + + @Test + void closesContentWhenTheCanonicalPayloadCannotBeRead() { + Fixture fixture = fixture(); + TrackingInputStream stream = storedContent(fixture); + when(fixture.specs.read("{\"profile\":\"skill\"}")) + .thenThrow(new IllegalArgumentException("invalid payload")); + + assertThrows( + AssetUnavailableException.class, + () -> fixture.service.open(ACTOR, ASSET_ID, RELEASE_ID)); + + assertTrue(stream.closed); + } + + @Test + void rejectsAReleaseWhosePackageReferenceIsMissing() { + Fixture fixture = fixture(); + when(fixture.references.findByReleaseIdAndOrganizationId( + RELEASE_ID, ORGANIZATION_ID)) + .thenReturn(Optional.empty()); + + assertThrows( + AssetUnavailableException.class, + () -> fixture.service.manifest(ACTOR, ASSET_ID, RELEASE_ID)); + + verifyNoInteractions(fixture.storage); + } + + @Test + void rejectsAReleaseWhosePackageReferenceIsNotABlob() { + Fixture fixture = fixture(); + when(fixture.reference.isBlobReference()).thenReturn(false); + + assertThrows( + AssetUnavailableException.class, + () -> fixture.service.manifest(ACTOR, ASSET_ID, RELEASE_ID)); + + verifyNoInteractions(fixture.storage); + } + @Test void resolvesCoordinateAndVersionBeforeApplyingTheSameLiveUseCheck() { Fixture fixture = fixture(); @@ -167,17 +223,20 @@ private static Fixture fixture() { when(reference.getDigest()).thenReturn(PACKAGE_DIGEST); when(reference.getContentLength()).thenReturn(7L); when(reference.getMediaType()).thenReturn("application/zip"); + SkillReleaseDeliveryService deliveries = new SkillReleaseDeliveryService( + assets, + identities, + releaseRepository, + references, + storage); return new Fixture( - new SkillDistributionService( - assets, - identities, - releaseRepository, - references, - specs, - storage), + new SkillDistributionService(deliveries, specs), assets, identities, releaseRepository, + references, + reference, + specs, storage); } @@ -218,6 +277,10 @@ private static AssetIdentity assetIdentity(AssetType type) { } private static SkillPackageSpec spec() { + return spec(PACKAGE_DIGEST); + } + + private static SkillPackageSpec spec(String packageDigest) { return new SkillPackageSpec( "triage", "Triage customer issues", @@ -227,7 +290,7 @@ private static SkillPackageSpec spec() { Map.of("owner", "support"), null, new SkillPackageSpec.Artifact( - PACKAGE_DIGEST, + packageDigest, 7, "application/zip"), List.of(new SkillPackageSpec.FileEntry( @@ -241,9 +304,25 @@ private record Fixture( AssetRegistryService assets, AssetIdentityQuery identities, AssetReleaseRepository releaseRepository, + AssetPayloadReferenceRepository references, + AssetPayloadReference reference, + SkillPackageSpecReader specs, SkillPackageStoragePort storage) { } + private static TrackingInputStream storedContent(Fixture fixture) { + TrackingInputStream stream = new TrackingInputStream(); + when(fixture.storage.open("private/skill.zip")) + .thenReturn(new SkillPackageStoragePort.StoredSkillPackageContent( + stream, + new SkillPackageStoragePort.StoredSkillPackage( + "private/skill.zip", + 7, + "application/zip", + PACKAGE_DIGEST))); + return stream; + } + private static final class TrackingInputStream extends ByteArrayInputStream { diff --git a/core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageAssetServiceTests.java b/core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageAssetServiceTests.java new file mode 100644 index 00000000..8a7f3e4e --- /dev/null +++ b/core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageAssetServiceTests.java @@ -0,0 +1,141 @@ +package com.orgmemory.core.assetregistry; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import com.orgmemory.core.assetregistry.api.AssetPortfolioState; +import com.orgmemory.core.assetregistry.api.AssetType; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageArtifact; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackagePayloadPolicy; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageUpload; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; +import com.orgmemory.core.organization.CurrentActor; +import com.orgmemory.core.permission.KnowledgeClassification; +import java.io.ByteArrayInputStream; +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import org.junit.jupiter.api.Test; + +class SkillPackageAssetServiceTests { + + private static final UUID ORGANIZATION_ID = + UUID.fromString("86000000-0000-0000-0000-000000000001"); + private static final UUID USER_ID = + UUID.fromString("86000000-0000-0000-0000-000000000002"); + private static final UUID SPACE_ID = + UUID.fromString("86000000-0000-0000-0000-000000000003"); + private static final UUID ASSET_ID = + UUID.fromString("86000000-0000-0000-0000-000000000004"); + private static final CurrentActor ACTOR = new CurrentActor( + USER_ID, + ORGANIZATION_ID, + null, + "Skill editor", + "skill.editor@example.test"); + private static final SkillPackageArtifact ARTIFACT = + new SkillPackageArtifact("a".repeat(64), 3, "application/zip"); + + @Test + void rejectsAnInvalidImportPayloadBeforeWritingStorage() { + Fixture fixture = fixture(); + rejectPayload(fixture.policy); + + assertThrows( + IllegalArgumentException.class, + () -> fixture.service.importPackage( + ACTOR, + "support", + SPACE_ID, + KnowledgeClassification.INTERNAL, + upload())); + + verifyNoInteractions(fixture.storage); + } + + @Test + void rejectsAnInvalidReplacementPayloadBeforeWritingStorage() { + Fixture fixture = fixture(); + when(fixture.assets.get(ACTOR, ASSET_ID)).thenReturn(skillView()); + rejectPayload(fixture.policy); + + assertThrows( + IllegalArgumentException.class, + () -> fixture.service.replacePackage( + ACTOR, + ASSET_ID, + 4, + upload())); + + verifyNoInteractions(fixture.storage); + } + + private static Fixture fixture() { + SkillPackageStoragePort storage = mock(SkillPackageStoragePort.class); + SkillPackagePayloadPolicy policy = mock(SkillPackagePayloadPolicy.class); + AssetRegistryService assets = mock(AssetRegistryService.class); + SkillPackageSupersessionCleanupService cleanup = + mock(SkillPackageSupersessionCleanupService.class); + return new Fixture( + new SkillPackageAssetService(storage, policy, assets, cleanup), + storage, + policy, + assets); + } + + private static void rejectPayload(SkillPackagePayloadPolicy policy) { + doThrow(new IllegalArgumentException("invalid payload")) + .when(policy) + .validate("{}", ARTIFACT); + } + + private static SkillPackageUpload upload() { + return new SkillPackageUpload( + UUID.randomUUID(), + "support-triage", + "Support triage", + "Triage support requests", + "2", + "{}", + ARTIFACT, + Map.of(), + new ByteArrayInputStream(new byte[] {1, 2, 3})); + } + + private static AssetView skillView() { + return new AssetView( + ASSET_ID, + AssetType.SKILL, + "support", + "support-triage", + SPACE_ID, + AssetPortfolioState.DRAFT_ONLY, + true, + new AssetView.Draft( + UUID.randomUUID(), + 4, + "Support triage", + "Triage support requests", + "INTERNAL", + "2", + "{}", + USER_ID, + Instant.parse("2026-08-03T00:00:00Z")), + List.of(), + List.of(), + List.of(), + null, + List.of()); + } + + private record Fixture( + SkillPackageAssetService service, + SkillPackageStoragePort storage, + SkillPackagePayloadPolicy policy, + AssetRegistryService assets) { + } +} diff --git a/core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinatorTests.java b/core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinatorTests.java index fef26935..0decbdb9 100644 --- a/core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinatorTests.java +++ b/core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageSupersessionCleanupCoordinatorTests.java @@ -1,21 +1,61 @@ package com.orgmemory.core.assetregistry; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import java.time.Instant; +import java.util.List; import java.util.Optional; import java.util.UUID; import org.junit.jupiter.api.Test; +import org.springframework.data.domain.PageRequest; class SkillPackageSupersessionCleanupCoordinatorTests { private static final UUID ORGANIZATION_ID = UUID.randomUUID(); private static final UUID ASSET_ID = UUID.randomUUID(); + @Test + void summarizesTheInternalRetryOutcomesWithoutPublishingRetryEntities() { + SkillPackageSupersessionRepository supersessions = + mock(SkillPackageSupersessionRepository.class); + SkillPackageSupersessionCleanupCoordinator coordinator = + mock(SkillPackageSupersessionCleanupCoordinator.class); + UUID deleted = UUID.randomUUID(); + UUID retained = UUID.randomUUID(); + UUID retry = UUID.randomUUID(); + UUID resolved = UUID.randomUUID(); + when(supersessions.findReadyIds( + any(Instant.class), + eq(SkillPackageSupersession.MAX_ATTEMPTS), + eq(PageRequest.of(0, 100)))) + .thenReturn(List.of(deleted, retained, retry, resolved)); + when(coordinator.cleanup(deleted)) + .thenReturn(SkillPackageCleanupOutcome.DELETED); + when(coordinator.cleanup(retained)) + .thenReturn( + SkillPackageCleanupOutcome.RETAINED_BY_IMMUTABLE_REFERENCE); + when(coordinator.cleanup(retry)) + .thenReturn(SkillPackageCleanupOutcome.RETRY_SCHEDULED); + when(coordinator.cleanup(resolved)) + .thenReturn(SkillPackageCleanupOutcome.ALREADY_RESOLVED); + + var summary = new SkillPackageSupersessionCleanupService( + supersessions, coordinator) + .cleanupPending(1_000); + + assertEquals(1, summary.deleted()); + assertEquals(1, summary.retainedByImmutableReference()); + assertEquals(1, summary.retryScheduled()); + assertEquals(1, summary.alreadyResolved()); + } + @Test void deletesAnUnreferencedSupersededObjectThenResolvesTheLedgerRow() { Fixture fixture = new Fixture(); diff --git a/core/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.java b/core/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.java index 6e5f3931..1b2dd9df 100644 --- a/core/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.java +++ b/core/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.java @@ -8,6 +8,7 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -15,6 +16,8 @@ import com.orgmemory.core.assetregistry.api.AssetPortfolioState; import com.orgmemory.core.assetregistry.api.AssetType; import com.orgmemory.core.assetregistry.api.AssetUnavailableException; +import com.orgmemory.core.assetregistry.skillpackage.SkillPackageAssetCommand; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.organization.CurrentActor; import com.orgmemory.core.organization.OrgMemoryAccessDeniedException; import com.orgmemory.core.permission.KnowledgeClassification; @@ -46,12 +49,10 @@ void refusesUnauthorizedImportsBeforeInspectingOrStoringBytes() { doThrow(new OrgMemoryAccessDeniedException("Denied")) .when(assets) .requireSkillCreate(ACTOR, SPACE_ID); - SkillRegistryService service = - new SkillRegistryService( - new SkillPackageInspector(), - storage, - assets, - mock(SkillPackageSupersessionCleanupService.class)); + SkillRegistryService service = service( + storage, + assets, + mock(SkillPackageSupersessionCleanupService.class)); assertThrows( OrgMemoryAccessDeniedException.class, @@ -85,12 +86,10 @@ void deletesStoredBytesWhenAssetIdentityCreationFails() throws Exception { any(), any())) .thenThrow(new AssetConflictException("Duplicate")); - SkillRegistryService service = - new SkillRegistryService( - new SkillPackageInspector(), - storage, - assets, - mock(SkillPackageSupersessionCleanupService.class)); + SkillRegistryService service = service( + storage, + assets, + mock(SkillPackageSupersessionCleanupService.class)); assertThrows( AssetConflictException.class, @@ -116,12 +115,10 @@ void retainsReferencedBytesWhenAuthorizationProjectionNeedsRetry() throws Except .thenReturn(assetId); when(assets.projectCreated(ACTOR, assetId)) .thenThrow(new AssetUnavailableException("Projection pending")); - SkillRegistryService service = - new SkillRegistryService( - new SkillPackageInspector(), - storage, - assets, - mock(SkillPackageSupersessionCleanupService.class)); + SkillRegistryService service = service( + storage, + assets, + mock(SkillPackageSupersessionCleanupService.class)); assertThrows( AssetUnavailableException.class, @@ -133,12 +130,11 @@ void retainsReferencedBytesWhenAuthorizationProjectionNeedsRetry() throws Except @Test void inspectionIsStatelessAndReturnsOnlyValidatedPackageFacts() throws Exception { byte[] archive = archive(); - SkillPackageStoragePort storage = mock(SkillPackageStoragePort.class); + SkillPackageAssetCommand packages = mock(SkillPackageAssetCommand.class); SkillRegistryService service = new SkillRegistryService( new SkillPackageInspector(), - storage, - mock(AssetRegistryService.class), - mock(SkillPackageSupersessionCleanupService.class)); + packages, + mock(AssetRegistryService.class)); SkillPackageInspection inspection = service.inspectPackage( ACTOR, archive.length, new ByteArrayInputStream(archive)); @@ -146,7 +142,7 @@ void inspectionIsStatelessAndReturnsOnlyValidatedPackageFacts() throws Exception assertEquals("support-triage", inspection.name()); assertEquals(1, inspection.files().size()); assertEquals("# Support triage", inspection.instructions()); - verify(storage, never()).put(any(), any()); + verify(packages, never()).importPackage(any(), any(), any(), any(), any()); } @Test @@ -170,8 +166,7 @@ void replacementAuthorizesBeforeStorageAndCleansTheDurableSupersession() when(assets.replaceValidatedSkillDraft( eq(ACTOR), eq(assetId), eq(7L), any(), any())) .thenReturn(new SkillDraftReplacement(current, supersessionId)); - SkillRegistryService service = new SkillRegistryService( - new SkillPackageInspector(), storage, assets, cleanup); + SkillRegistryService service = service(storage, assets, cleanup); AssetView replaced = service.replacePackage( ACTOR, @@ -181,7 +176,7 @@ void replacementAuthorizesBeforeStorageAndCleansTheDurableSupersession() new ByteArrayInputStream(archive)); assertEquals(assetId, replaced.id()); - verify(assets).requireSkillEdit(ACTOR, assetId); + verify(assets, times(2)).requireSkillEdit(ACTOR, assetId); verify(cleanup).cleanup(supersessionId); verify(storage, never()).delete(any()); } @@ -201,8 +196,7 @@ void replacementDeletesTheNewObjectWhenTheDatabaseSwapFails() throws Exception { }); when(assets.replaceValidatedSkillDraft(any(), any(), any(Long.class), any(), any())) .thenThrow(new AssetConflictException("Changed")); - SkillRegistryService service = new SkillRegistryService( - new SkillPackageInspector(), + SkillRegistryService service = service( storage, assets, mock(SkillPackageSupersessionCleanupService.class)); @@ -230,6 +224,16 @@ private static AssetView importArchive( new ByteArrayInputStream(archive)); } + private static SkillRegistryService service( + SkillPackageStoragePort storage, + AssetRegistryService assets, + SkillPackageSupersessionCleanupService cleanup) { + SkillPackageAssetCommand command = new SkillPackageAssetService( + storage, new SkillPackageProfile(), assets, cleanup); + return new SkillRegistryService( + new SkillPackageInspector(), command, assets); + } + private static SkillPackageStoragePort.StoredSkillPackage stored( SkillPackageStoragePort.SkillPackageWriteRequest request) { return new SkillPackageStoragePort.StoredSkillPackage( diff --git a/docs/increments/active/2026-07-31-spring-modulith-package-refactor/assetregistry-skill-challenge-brief.md b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/assetregistry-skill-challenge-brief.md new file mode 100644 index 00000000..50329936 --- /dev/null +++ b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/assetregistry-skill-challenge-brief.md @@ -0,0 +1,143 @@ +# Asset Registry Skill Boundary Challenge Brief + +Date: 2026-08-02 +Baseline: `6b36e1282dab70e4b224c17d4069e8749ad3edb7` + +Reviewer availability: Claude Fable 5 was launched in fresh Orca terminal +`term_d9a08544-7543-40bf-a4b4-bb09c58a0823`. The original file-directed +request and the required plain-Markdown/no-tools recovery both returned blank +with zero tokens. Per the project challenge procedure, the review therefore +continues in a fresh external Codex `gpt-5.6-sol` session with `ultra` +reasoning; the Fable failure is not treated as a verdict. + +## Reviewer Instructions + +Act as an adversarial, read-only architecture reviewer. Attack the proposal; +do not validate it by default. Verify every claim in the repository itself. +Make no edits, mutations, commits, or plan changes. Read `CLAUDE.md`, +`docs/conventions.md`, `docs/specs/domains/asset-registry.md`, +`docs/tests/domains/asset-registry.md`, the active increment design/plan, and +the filenames under `docs/decisions` before judging. + +Return plain Markdown with: + +1. one explicit verdict: accept, accept with mandatory corrections, or reject; +2. the strongest counterargument; +3. a must-fix list with repository evidence for every item; +4. an exact ownership/API/dependency recommendation; +5. whether one code PR below 100 changed files is responsible or which + independently mergeable sequence is required; +6. the rejected alternative and why it loses. + +## Product Promise At Stake + +OrgMemory is a governed organizational memory and reusable-capability layer +for enterprise AI. A Skill is not an executable shortcut: it is an immutable, +authorized, integrity-checked package published through the shared Asset +lifecycle and distributed only from an exact usable release. Refactoring must +reduce the 72-file Asset Registry root package without weakening tenant +isolation, authorization, atomic Draft/reference replacement, immutable +Revision/Release pins, bounded archive validation, cleanup durability, or the +existing REST/MCP/CLI wire contracts. + +## Exact Proposal Under Review + +> Introduce one immediately closed `assetregistry.skill` nested module for +> Skill-specific inspection, profile parsing, import, GitHub acquisition, +> distribution, and storage-cleanup orchestration. Put every contract consumed +> by API, Worker, connector, object-storage, parent Asset lifecycle, or the +> nested implementation into one exact parent-owned +> `assetregistry::skill` named interface. External top-level modules consume +> only that interface. The nested module exposes no public implementation type +> and imports no parent default-package class, entity, or repository. +> +> Keep Draft/revision/release/payload-reference writes and the +> `SkillPackageSupersession` row/repository in the parent Asset Registry because +> replacement creates the supersession row atomically with the locked Draft +> and payload-reference mutation. Expose narrow parent-owned lifecycle/query +> ports to the nested module for authorization, identity creation/replacement, +> exact-release resolution, reference integrity facts, and cleanup state. Keep +> object-storage calls behind `SkillPackageStoragePort`; do not expose storage +> keys in REST/MCP results. Close the module in the same code-bearing PR, keep +> it below 100 changed paths, and preserve every existing behavior and schema. + +Today these rules are not enforced. The relevant implementation is spread +across: + +- `core/src/main/java/com/orgmemory/core/assetregistry/Skill*.java` +- `core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.java` +- `core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java` +- `core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java` +- `core/src/main/java/com/orgmemory/core/assetregistry/AssetRelease.java` +- `apps/api/src/main/java/com/orgmemory/api/assetregistry` +- `apps/worker/src/main/java/com/orgmemory/worker/assetregistry` +- `integrations/connectors/src/main/java/com/orgmemory/connectors/github` +- `integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio` + +The proposed boundary would be enforced in the new Skill `package-info.java`, +the exact parent named-interface `package-info.java`, and +`core/src/test/java/com/orgmemory/core/ModulithVerificationTests.java`. + +## Repository Evidence And Known Tension + +- The Asset Registry root contains 72 production Java files; 18 are named + `Skill*`. +- Twelve Skill types are currently public. API, Worker, GitHub connector, and + MinIO adapter import concrete services, result records, or ports directly. +- `SkillRegistryService`, `SkillGitHubImportService`, and + `SkillDistributionService` call parent `AssetRegistryService` or consume + parent entities/repositories. +- The reverse edge also exists: `AssetRegistryService`, + `AssetRegistryCoordinator`, and `AssetPayloadReference` consume Skill package + specs/storage values, while the coordinator writes + `SkillPackageSupersession` in the same `REQUIRES_NEW` Draft replacement + transaction. +- `SkillPackageSupersessionCleanupCoordinator` currently locks the cleanup row, + checks all Draft/Revision/Release references, calls object storage, and + records retry state inside one `REQUIRES_NEW` transaction. +- A direct file move therefore creates a parent/child module cycle or publishes + JPA internals. The proposed lifecycle ports avoid both, but may become an + oversized artificial API or split one consistency boundary incorrectly. + +The reviewer must specifically decide: + +1. whether one `skill` module is coherent or inspection/import, distribution, + and cleanup should be separate modules; +2. whether the parent named interface is too broad and should be divided into + exact `skill-package`, `skill-source`, `skill-distribution`, and/or internal + lifecycle interfaces; +3. whether supersession persistence and cleanup belong entirely in the parent, + entirely in Skill, or across a port, without weakening atomic replacement; +4. whether package-private Spring implementations behind parent interfaces are + preferable to retaining selected public nested services for API/tests; +5. the smallest safe code PR sequence below 100 changed files. + +## Comparable Source Evidence + +| System and pin | Observed mechanism | File-level evidence | Relevance and limit | +| --- | --- | --- | --- | +| AgentRegistry `d8d3f4ef1ebeed70d58adafd26590ead6198addf` | The public Skill API is a compact typed envelope; a dedicated controller owns source resolution and depends on a narrow store interface. It records an immutable commit pin but deliberately stores no Skill content. | `D:/OrgMemory/tmp/upstream-agentregistry/pkg/api/v1alpha1/skill.go:3-53`; `D:/OrgMemory/tmp/upstream-agentregistry/internal/registry/controller/skill_controller.go:20-79` | Supports separating Skill-specific resolution from the generic registry store. It cannot justify moving OrgMemory's blob/reference transaction because AgentRegistry explicitly does not own stored Skill bytes. | +| Vercel Skills `1164afa5f0e21ebd01e6fc11249759353f494ad1` | Archive validation is isolated behind bounded entry/byte limits and safe-path normalization; download/extraction orchestration is separate; installation state pins a full folder hash in a distinct lock model. | `D:/OrgMemory/tmp/skill-registry-research/vercel-skills/src/archive.ts:21-30,150-161,265-346`; `D:/OrgMemory/tmp/skill-registry-research/vercel-skills/src/download-source.ts:134-157,259-335`; `D:/OrgMemory/tmp/skill-registry-research/vercel-skills/src/skill-lock.ts:14-42,209-226` | Supports keeping validation, acquisition, and consumer state as explicit contracts. It has no governed multi-tenant Asset lifecycle, so its local lock/store split cannot override OrgMemory authorization or database atomicity. | + +## Operational Cost Motivating The Decision + +The root package began this Asset phase with 119 production Java files and is +still at 72 after Kernel, Authorization, and Prompt extraction. Skill alone is +18 root files with consumers across four Gradle subprojects. The current +same-package access hides bidirectional coupling and makes the directory hard +to navigate; moving by filename would either fail Spring Modulith verification +or silently widen internal repositories and storage identifiers. This is the +next material slice, so the boundary must be settled before characterization +tests or production moves are committed. + +## Suspected Contradictions For The Counterattack + +After the first verdict, challenge it against all three: + +1. A single parent `assetregistry::skill` interface may become a dumping ground + that merely relocates twelve public types without reducing coupling. +2. Keeping supersession persistence in the parent while cleanup behavior sits + in Skill may split one failure/retry aggregate across modules. +3. Returning storage-reference facts through a public named interface may make + a secret implementation identifier broadly importable even if ArchUnit pins + today's consumer. diff --git a/docs/increments/active/2026-07-31-spring-modulith-package-refactor/assetregistry-skill-challenge-verdict.md b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/assetregistry-skill-challenge-verdict.md new file mode 100644 index 00000000..8255cf54 --- /dev/null +++ b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/assetregistry-skill-challenge-verdict.md @@ -0,0 +1,199 @@ +# Asset Registry Skill Boundary Challenge Verdict + +Date: 2026-08-02 +Reviewed baseline: `6b36e1282dab70e4b224c17d4069e8749ad3edb7` + +## Review Execution + +Claude Fable 5 was invoked twice through Orca terminal +`term_d9a08544-7543-40bf-a4b4-bb09c58a0823`. Both the original +file-directed request and the required recovery request returned blank, +zero-token responses, so the configured reviewer was unavailable. + +The repository-mandated fallback ran independently and read-only in Orca +terminal `term_c3c1d2b2-52f7-421a-a306-bfdd719d234a` with +`gpt-5.6-sol` at `ultra` reasoning. It inspected the governing documents, +the current implementation and tests, the exact external consumers, and the +pinned comparable sources. It then completed a second counterattack round +against its own first verdict. No repository edit or commit was made by the +reviewer. + +## Final Verdict + +Accept one immediately closed `assetregistry.skill` module, with mandatory +corrections. The first proposal is rejected because a single +`assetregistry::skill` interface would be a capability dumping ground and +because moving only cleanup orchestration would split the supersession retry +aggregate. + +The counterattack further corrected the first-round verdict: + +- remove `SkillPackageReferenceFacts` entirely; +- do not let `assetregistry.skill` consume `skill-storage` or orchestrate + supersession cleanup; +- make the parent Asset Registry own the complete cross-store artifact saga, + including storage write, compensation, Draft replacement, reference pinning, + immediate cleanup, and scheduled retry; +- keep `objectKey` inside the storage capability and parent persistence only. + +## Strongest Counterargument + +Inspection, GitHub acquisition, authoring, delivery, storage, and cleanup have +different consumers and trust surfaces. Moving their current public types into +one child package could improve directory shape while making the real +capability boundary less precise. Conversely, making each concern a nested +module would introduce modules without independent aggregates or transaction +owners. + +The selected answer is one Skill semantics module plus four exact parent-owned +capabilities. Skill decides what a valid package means. The parent decides how +validated bytes become, replace, pin, distribute, and eventually leave a +governed Asset. + +## Binding Ownership + +`assetregistry.skill` owns: + +- bounded archive inspection and canonical Skill payload validation; +- Skill specification, schema parsing, and profile semantics; +- GitHub acquisition and partial-result orchestration; +- API-facing package, GitHub, and distribution operations; +- install-manifest construction and package-integrity interpretation. + +The parent `assetregistry` module owns: + +- authorization and governed Asset identity; +- object-storage writes and database-failure compensation; +- Draft, revision, release, and payload-reference persistence; +- the supersession row, lock, retry state, immediate cleanup, and scheduled + cleanup; +- exact-release resolution and storage opening. + +This keeps the indivisible `REQUIRES_NEW` Draft replacement in +`AssetRegistryCoordinator.replaceSkillDraft`, including the Draft lock, +reference mutation, Draft mutation, and supersession insert. Revision and +Release reference pinning also stay with the parent. The complete cleanup +aggregate remains parent-owned because it locks the supersession row, checks +all live references, deletes or retains the object, and either removes the row +or records bounded retry state. + +## Exact Parent Capabilities + +The four named interfaces have closed type and consumer sets: + +| Named interface | Exact top-level types | Permitted consumers | +| --- | --- | --- | +| `assetregistry::skill-package` | `SkillPackageAssetCommand`, `SkillPackagePayloadPolicy`, `SkillPackageArtifact`, `SkillPackageUpload` | parent Asset implementation and `assetregistry.skill` only | +| `assetregistry::skill-delivery` | `SkillReleaseDeliveryQuery`, `SkillReleaseDescriptor`, `SkillReleaseContent` | `assetregistry.skill` only | +| `assetregistry::skill-cleanup` | `SkillPackageCleanupOperations`, `SkillPackageCleanupSummary` | Worker only | +| `assetregistry::skill-storage` | `SkillPackageStoragePort` and its nested write, stored-package, and content values | exact parent persistence/delivery/cleanup classes and MinIO only | + +No capability may expose a JPA entity, repository, lock handle, transaction +status, supersession ID, retry entity, mutable state, `AssetView`, controller +DTO, or generic fact map. Only `skill-storage` may carry an object key. + +`assetregistry.skill` may depend on `skill-package` and `skill-delivery`, but +not on `skill-storage` or `skill-cleanup`. API imports none of the four parent +capabilities. Worker imports only `skill-cleanup`. MinIO imports only +`skill-storage`. Exact named-interface membership and exact importer sets are +executable build gates. + +## Closed Skill Surface + +The child module's exact public top-level surface is: + +- `SkillPackageOperations` +- `SkillGitHubOperations` +- `SkillDistributionOperations` +- `SkillGitHubSourcePort` +- `SkillPackageInspection` +- `SkillInstallManifest` +- `SkillPackageContent` + +All Spring implementations, `SkillPackageSpec`, inspector, profile, parser, +and validation exception are package-private. GitHub visibility moves from the +internal package specification into `SkillGitHubSourcePort`. + +The child module's exact dependency allowlist is: + +- `assetregistry::api` +- `assetregistry::consumption` +- `assetregistry::profile` +- `assetregistry::skill-package` +- `assetregistry::skill-delivery` +- `organization` +- `permission` +- `shared::error` + +It imports no parent default-package implementation, entity, repository, +Authorization implementation, Kernel implementation, storage contract, or +cleanup contract. + +## Storage-Reference Constraint + +`assetregistry.skill` submits a bounded `SkillPackageUpload` to the parent. The +parent writes storage, receives and persists the object key, and performs +compensation or cleanup. The child never receives the stored object key. + +For delivery, the parent authorizes the exact release, resolves its reference, +opens storage internally, and returns immutable release and payload facts plus +digest, length, media type, and a content stream. It returns no storage +locator. `SkillInstallManifest`, REST, MCP, Assistant, audit values, logs, and +exception messages must never expose the key. + +## Binding Delivery Sequence + +### PR 1 — Parent-owned artifact lifecycle + +Target fewer than 60 changed paths: + +1. Add failing-first exact named-interface and consumer-isolation tests. +2. Add the four parent capabilities and parent adapters. +3. Move storage write, compensation, replacement cleanup, release/reference + lookup, and storage opening behind the parent contracts. +4. Keep current Skill classes in the parent package temporarily and route them + through the new capabilities. +5. Preserve transactions, schema, authorization, wire contracts, partial + GitHub imports, and cleanup behavior. +6. Pass focused Core/API/OpenAPI/Worker/connector/MinIO/integration gates and a + terminating clean repository test. + +This PR is independently mergeable and introduces no nested Skill module. + +### PR 2 — Move and immediately close `assetregistry.skill` + +Target fewer than 70 changed paths: + +1. Add failing-first closed-module, exact-public-surface, + forbidden-parent-import, and external-consumer tests. +2. Move the package-semantic production types and focused tests. +3. Introduce the three public operation interfaces with package-private + implementations. +4. Internalize `SkillPackageSpec` and move GitHub visibility to the source + port. +5. Update API and connector imports while preserving every wire schema. +6. Add the child as `Type.CLOSED` immediately with the exact dependency + allowlist. +7. Prove it has no dependency on the parent default package, repositories, + entities, `skill-storage`, or `skill-cleanup`, then pass all focused and + terminating clean gates. + +No intermediate open Skill module, schema change, storage-reference exposure, +cleanup-protocol redesign, or product behavior change is permitted. + +## Comparable Sources + +- AgentRegistry pin: `d8d3f4ef1ebeed70d58adafd26590ead6198addf` +- Vercel Skills pin: `1164afa5f0e21ebd01e6fc11249759353f494ad1` + +These sources inform package validation and distribution mechanics. They do +not override OrgMemory's governed Asset lifecycle, organization-scoped +authorization, immutable release pins, or cross-store retry requirements. + +## Rejected Alternative + +Rejected: one parent-owned `assetregistry::skill` interface containing every +API, Worker, connector, storage, lifecycle, and implementation contract, with +cleanup behavior moved into Skill. It broadens unrelated capabilities, leaks a +storage locator toward application consumers, weakens exact dependency +enforcement, and splits the supersession retry aggregate. diff --git a/docs/increments/active/2026-07-31-spring-modulith-package-refactor/design.md b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/design.md index dd5721eb..17b28842 100644 --- a/docs/increments/active/2026-07-31-spring-modulith-package-refactor/design.md +++ b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/design.md @@ -464,6 +464,38 @@ See [the Prompt challenge verdict](assetregistry-prompt-challenge-verdict.md) for the unavailable Fable 5 attempts, independent fallback, counterattack, corrected topology, rejected alternative, and executable gates. +## Asset Registry Skill Boundary + +The Skill family is delivered in two code-bearing PRs. The first establishes +four exact parent-owned capabilities without creating a child module. The +second moves package semantics and immediately closes `assetregistry.skill`. +This sequence keeps every PR below 100 changed paths without an intermediate +open module. + +The parent Asset Registry owns the complete artifact persistence saga: +authorization, storage write and compensation, Draft/revision/release and +payload-reference persistence, supersession locking and retry state, immediate +and scheduled cleanup, exact-release resolution, and storage opening. The +child owns bounded package inspection, Skill specification and validation, +GitHub acquisition, API-facing Skill operations, and install-manifest +construction. + +The parent exposes four exact capability interfaces: `skill-package` for a +validated upload entering the Asset lifecycle, `skill-delivery` for authorized +release facts and content without a locator, `skill-cleanup` for the Worker +trigger, and `skill-storage` for exact parent persistence/cleanup classes plus +MinIO. `assetregistry.skill` may consume only the first two. It never receives +an object key and never orchestrates supersession cleanup. + +When introduced, the closed Skill module exposes exactly four operation/source +interfaces and three immutable results. Its implementations, package +specification, inspector, profile, parser, and validation exception remain +package-private. See +[the Skill challenge verdict](assetregistry-skill-challenge-verdict.md) for the +Fable 5 availability failure, independent fallback, counterattack corrections, +exact contracts and consumer sets, storage-reference constraint, and binding +two-PR sequence. + ## Strongest Counterargument Ordinary internal subpackages would reduce directory size immediately and diff --git a/docs/increments/active/2026-07-31-spring-modulith-package-refactor/plan.md b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/plan.md index dd8fe38a..b173aea1 100644 --- a/docs/increments/active/2026-07-31-spring-modulith-package-refactor/plan.md +++ b/docs/increments/active/2026-07-31-spring-modulith-package-refactor/plan.md @@ -1533,3 +1533,74 @@ package/import search, exact-public-surface, `git diff --check`, and clean-test fallback was used. The terminating repository-wide `clean test` passed all 99 tasks in 7m05s. Rename-aware diff accounting is exactly 69 changed paths, and the Asset Registry root package decreased from 90 to 72 Java files. + +PR #274 merged as `6b36e1282dab70e4b224c17d4069e8749ad3edb7` +after every required CI check passed. CodeRabbit's completed full review found +three valid issues; all were fixed and all three threads were resolved. The +reviewed head `3fe3183e` passed the terminating 99-task clean repository test +in 8m01s. Release remains deferred. The Skill family is the next code-bearing +slice and starts with an independent boundary challenge. + +## Asset Registry Skill Sequence + +- [x] Challenge the exact Skill ownership, parent contract, supersession, and + delivery boundary. Fable 5 returned blank zero-token responses twice; the + independent Orca fallback and counterattack selected one Skill semantics + module but rejected both the catch-all parent interface and split cleanup + ownership. The binding result is recorded in + [the verdict](assetregistry-skill-challenge-verdict.md). +- [x] PR 1: add failing-first exact-interface and importer guards, then + establish parent-owned `skill-package`, `skill-delivery`, `skill-cleanup`, + and `skill-storage` capabilities below 60 changed paths. +- [x] PR 1: route current parent-package Skill flows through those capabilities + without a nested module, storage-key exposure, transaction change, schema + change, or wire-contract change. +- [x] PR 1: pass focused Core/API/OpenAPI/Worker/connector/MinIO/integration + gates, docs and release policy, static analysis fallback, and a terminating + clean repository test. +- [ ] PR 1: merge through CI and CodeRabbit without releasing. +- [ ] PR 2: add failing-first closed-module, exact-public-surface, + forbidden-parent-import, and external-consumer guards, then move and + immediately close `assetregistry.skill` below 70 changed paths. +- [ ] PR 2: pass all focused and terminating gates, merge through CI and + CodeRabbit, then continue to the next Asset Registry profile family without + releasing early. + +The failing-first named-interface characterization produced the expected +`NoSuchElementException` before any capability package existed. Commit +`4a158882` then established the four exact parent interfaces and importer +guards, moved storage/compensation/reference lookup/storage opening behind +parent implementations, retained the complete supersession aggregate in the +parent, and reduced Worker cleanup visibility to an immutable summary. The +Skill semantics layer receives no object key and imports neither storage nor +cleanup capability. + +The implementation changes 43 rename-aware paths, below both the reviewed +60-path target and the hard 100-file PR cap. Full suites passed 485 Core, 186 +API, 67 Worker, 120 connector, and 6 MinIO tests with zero failure, error, or +skip in 7m49s. OpenAPI and PostgreSQL Asset Registry integration tests passed +without a contract or schema rewrite. The documentation operating-model check +passed for 531 Markdown files and 8 mirrored domain pairs. Release policy +passed 18 Tegami/product and 23 workflow/policy tests on exact Node 24.15.0. +JetBrains inspection remained unavailable, so Gradle compilation, executable +Modulith and exact-consumer tests, zero-byte/package/migration checks, and +`git diff --check` supplied the documented fallback. The terminating +repository-wide `clean test` passed all 108 tasks in 7m15s. Release remains +deferred. + +PR #282's first CodeRabbit pass completed after every required CI job was +green and raised three inline findings plus two outside-diff test/exception +suggestions. Commit `518e0277` implements the valid subset: payload/artifact +consistency now fails before storage I/O for both import and replacement, +Jackson 3 serialization failure is translated to the stable staging-unavailable +contract, and six focused tests cover pre-storage rejection, missing/non-blob +references, and stream closure when manifest construction fails. The full Core +suite then passed 491 tests with zero failures. Restricting the parent artifact +value to only `application/zip` was rejected because readable legacy Skill +schema intentionally permits `application/octet-stream`; duplicating coordinate +validation in the lifecycle service was rejected because Kernel already +normalizes and validates both coordinates before persistence, with a stricter +Skill slug grammar than delivery resolution. API exception translation was +also confirmed to serialize only the stable top-level business message, never +the storage cause. The PR remains below the 100-file cap and release remains +deferred. diff --git a/docs/specs/domains/asset-registry.md b/docs/specs/domains/asset-registry.md index c4aa7134..e4f81c18 100644 --- a/docs/specs/domains/asset-registry.md +++ b/docs/specs/domains/asset-registry.md @@ -7,11 +7,13 @@ Source: `core/src/main/java/com/orgmemory/core/assetregistry`, `core/src/main/java/com/orgmemory/core/knowledge/retrieval/KnowledgeCatalogService.java`, `apps/api/src/main/java/com/orgmemory/api/assetregistry`, `apps/api/src/main/java/com/orgmemory/api/knowledge`, +`apps/worker/src/main/java/com/orgmemory/worker/assetregistry`, `apps/mcp/src/main/java/com/orgmemory/mcp`, `apps/cli/src`, -`apps/cli/package.json`, `.github/workflows/publish-cli.yml`, and -`apps/web/src/features/assets`. +`apps/cli/package.json`, `.github/workflows/publish-cli.yml`, +`apps/web/src/features/assets`, and +`integrations/object-storage-minio/src/main/java`. -Reconciled: `2026-08-02-spring-modulith-package-refactor (573c1d1f)`. +Reconciled: `2026-08-03-spring-modulith-package-refactor (518e0277)`. ## Current Behavior @@ -117,6 +119,9 @@ Skill metadata, SHA-256, size, media type, and file manifest form a server-generated draft payload. Payload schema 2 may also carry server-derived GitHub origin repository, full 40-character commit SHA, `SKILL.md` path, and public/private visibility; schema 1 remains readable for existing releases. +The parent validates the canonical payload against the inspected artifact +before performing any storage write, then separately verifies the metadata +reported by storage before changing the Asset ledger. The storage object key remains only in the internal payload-reference ledger. The draft reference is created atomically with the Asset. An accountable owner-class actor may publish that Draft @@ -247,6 +252,16 @@ still references it. Otherwise it is retained; transient storage failures stay in the bounded retry queue. A published Revision or Release therefore keeps its exact original bytes when the working Draft changes. +The parent exposes four exact Skill capabilities rather than one broad Skill +interface. Package creation/replacement receives a canonical upload through +`skill-package`; exact release delivery returns immutable release and artifact +facts plus content through `skill-delivery`; Worker sees only the bounded +`skill-cleanup` batch operation and summary; and only exact parent +persistence/delivery/cleanup classes plus MinIO may import `skill-storage`. +The parent owns the entire storage and supersession saga. Package semantics, +API results, manifests, audit values, logs, and exceptions do not receive the +persisted object key. + ### Federated Knowledge Knowledge remains owned by the canonical Knowledge ledger. The read-only diff --git a/docs/tests/domains/asset-registry.md b/docs/tests/domains/asset-registry.md index 78f3288d..939620b1 100644 --- a/docs/tests/domains/asset-registry.md +++ b/docs/tests/domains/asset-registry.md @@ -6,11 +6,14 @@ Source: `core/src/test/java/com/orgmemory/core/assetregistry`, `core/src/test/java/com/orgmemory/core/ModulithVerificationTests.java`, `apps/api/src/test/java/com/orgmemory/api/assetregistry`, `apps/api/src/test/java/com/orgmemory/api/OpenApiContractTests.java`, +`apps/api/src/test/java/com/orgmemory/api/SkillCapabilityBoundaryTests.java`, +`apps/worker/src/test/java/com/orgmemory/worker/SkillCapabilityBoundaryTests.java`, +`integrations/object-storage-minio/src/test/java`, `apps/mcp/src/test/java/com/orgmemory/mcp`, `apps/cli/src/*.test.ts`, `scripts/npm-publish-workflow-policy.test.mjs`, and `apps/web/src/features/assets/**/*.test.ts`. -Reconciled: `2026-08-02-spring-modulith-package-refactor (573c1d1f)`. +Reconciled: `2026-08-03-spring-modulith-package-refactor (518e0277)`. | Behavior | Evidence | Status | | --- | --- | --- | @@ -19,7 +22,7 @@ Reconciled: `2026-08-02-spring-modulith-package-refactor (573c1d1f)`. | Registration writes Asset, OWNER, and three authorization intents atomically; duplicate roles emit no intent; lifecycle transitions stay on the locked canonical Asset; commands join the parent transaction, queue operations own short transactions, and OpenFGA projection rejects ambient transactions | `AssetKernelServiceTests`, `AssetAuthorizationOutboxTests`, `AssetAuthorizationProjectionServiceTests` | covered | | Prompt, Work Instruction, Pack, and Skill schemas reject invalid payloads | `AssetProfileValidationTests` | covered | | Skill ZIP inspection rejects traversal, case collisions, symlinks, invalid frontmatter, invalid UTF-8, and bounded-size violations without extraction | `SkillPackageInspectorTests` | covered | -| Unauthorized Skill import is rejected before object storage and pre-identity failures clean up staged objects | `SkillRegistryServiceTests` | covered | +| Unauthorized or payload-inconsistent Skill import/replacement is rejected before object storage and pre-identity failures clean up staged objects | `SkillRegistryServiceTests`, `SkillPackageAssetServiceTests` | covered | | Stateless Skill inspection returns canonical bounded metadata without storage; Scratch, raw `SKILL.md`, ZIP, and folder packaging converge on the same server validator | `SkillRegistryServiceTests#inspectionIsStatelessAndReturnsOnlyValidatedPackageFacts`, `skill-package-browser.test.ts`, `asset-registry-golden-poc.spec.ts` | covered | | GitHub Skill preview and private-connection discovery require Skill-create permission on the selected Knowledge Space, pin a full commit SHA, discover nearest bounded `SKILL.md` roots, reject unsafe/link/colliding archives, and keep invalid candidates independently visible | `GitHubSkillArchiveReaderTests`, `GitHubSkillSourceAdapterTests`, `SkillGitHubImportServiceTests`, `asset-registry-golden-poc.spec.ts#GitHub Skill import pins preview, supports private access, and reports partial results` | covered | | Private GitHub import requires an administrator opt-in and selected GitHub App repository, fails closed on missing repository identifiers, audits allow/deny credential use, distinguishes rate limits from private repositories, applies HTTP timeouts, disables generic redirects, validates the single codeload redirect, strips Authorization before archive download, and enforces archive-size bounds | `GitHubSkillSourceAdapterTests`, `connector-github.test.ts` | covered | @@ -27,12 +30,13 @@ Reconciled: `2026-08-02-spring-modulith-package-refactor (573c1d1f)`. | Skill Draft replacement requires live edit authorization plus the expected Draft version, compensates fresh storage on transaction failure, and never mutates an immutable package reference | `SkillRegistryServiceTests`, `AssetRegistryIntegrationTests#replacingAReleasedSkillDraftKeepsTheImmutablePackageAndClearsTheCleanupRow`, `AssetRegistryIntegrationTests#replacingAnUnreleasedSkillDraftDeletesItsUnreferencedOldPackage` | covered | | Database mutation guards allow only Draft-reference deletion; payload-reference update and Revision/Release deletion remain rejected | `AssetRegistryIntegrationTests#onlyDraftPayloadReferencesMayBeDeletedWhileAllReferenceUpdatesStayRejected` | covered | | Post-commit supersession cleanup deletes only an exact unreferenced object, retains immutable pins, and durably schedules bounded retries after storage failure | `SkillPackageSupersessionCleanupCoordinatorTests` | covered | +| The four parent-owned Skill capabilities expose exact type sets and exact Core/API/Worker/MinIO consumer sets; storage locators do not enter API or Worker dependencies | `ModulithVerificationTests#assetRegistrySkillCapabilitiesAreExactExplicitNamedInterfaces`, `#assetRegistrySkillCapabilitiesHaveExactCoreConsumers`, `SkillCapabilityBoundaryTests`, `MinioSkillPackageStorageAdapterTests#adapterExposesOnlyTheParentStorageCapability` | covered | | A projection retry retains the already-referenced Skill object rather than deleting it | `SkillRegistryServiceTests#retainsReferencedBytesWhenAuthorizationProjectionNeedsRetry` | covered | | Skill storage uses an organization-scoped object key and verifies the stored SHA-256 | `MinioSkillPackageStorageAdapterTests` | covered | | Direct Skill publication atomically creates one Revision and Release, pins the exact validated blob through Draft, Revision, and Release, records `DIRECT` provenance, and emits the dedicated audit policy | `AssetRegistryIntegrationTests#skillImportPublishesDirectlyAndPinsTheValidatedBlob` | covered | | An active Skill review blocks direct publication rather than becoming an approval bypass | `AssetRegistryIntegrationTests#directSkillPublicationDoesNotBypassAnActiveReview` | covered | | The direct command rejects every non-Skill Asset profile | `AssetRegistryIntegrationTests#directSkillPublicationRejectsEveryOtherAssetProfile` | covered | -| Exact Skill manifests omit storage keys and package streaming rejects payload, release-reference, and stored-object mismatches | `SkillDistributionServiceTests`, `SkillDistributionControllerTests`, `MinioSkillPackageStorageAdapterTests` | covered | +| Exact Skill manifests omit storage keys; package streaming rejects missing/non-blob references plus payload, release-reference, and stored-object mismatches and closes opened content on manifest failure | `SkillDistributionServiceTests`, `SkillDistributionControllerTests`, `MinioSkillPackageStorageAdapterTests` | covered | | Browser Skill detail reads the exact manifest through an OIDC-session-only endpoint without weakening bearer `assets:read` admission | `AssetConsumptionControllerTests`, `asset-registry-golden-poc.spec.ts` | covered | | Method-level authorization denial returns a stable opaque HTTP 403 instead of an internal HTTP 500 | `ApiExceptionHandlerTests#methodAuthorizationDenialUsesTheStableForbiddenContract` | covered | | MCP Skill discovery and binary proxy retain bearer admission and exchanged API authorization | `SkillPackageControllerTests`, `AssetDeliveryControllerSecurityTests` | covered | diff --git a/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.java b/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.java index dba5aab3..7f7ed358 100644 --- a/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.java +++ b/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.java @@ -1,6 +1,6 @@ package com.orgmemory.integrations.storage.minio; -import com.orgmemory.core.assetregistry.SkillPackageStoragePort; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.knowledge.storage.ObjectStoragePort; import io.minio.MinioClient; import org.springframework.boot.autoconfigure.AutoConfiguration; diff --git a/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.java b/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.java index c8c59de8..1c8a65fa 100644 --- a/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.java +++ b/integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.java @@ -1,6 +1,6 @@ package com.orgmemory.integrations.storage.minio; -import com.orgmemory.core.assetregistry.SkillPackageStoragePort; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.knowledge.storage.ObjectKey; import com.orgmemory.core.knowledge.storage.ObjectStoragePort; import com.orgmemory.core.knowledge.storage.ObjectWriteRequest; diff --git a/integrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.java b/integrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.java index 2c18ca04..1d9e474d 100644 --- a/integrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.java +++ b/integrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.java @@ -1,5 +1,6 @@ package com.orgmemory.integrations.storage.minio; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; @@ -7,7 +8,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import com.orgmemory.core.assetregistry.SkillPackageStoragePort; +import com.orgmemory.core.assetregistry.skillstorage.SkillPackageStoragePort; import com.orgmemory.core.knowledge.storage.ObjectKey; import com.orgmemory.core.knowledge.storage.ObjectContent; import com.orgmemory.core.knowledge.storage.ObjectStoragePort; @@ -19,6 +20,13 @@ class MinioSkillPackageStorageAdapterTests { + @Test + void adapterExposesOnlyTheParentStorageCapability() { + assertArrayEquals( + new Class[] {SkillPackageStoragePort.class}, + MinioSkillPackageStorageAdapter.class.getInterfaces()); + } + @Test void writesAnOrganizationScopedKeyAndReturnsTheVerifiedDigest() { ObjectStoragePort objects = mock(ObjectStoragePort.class);