feat(asset-registry): add skill package foundation - #77
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds Skill as a governed asset type with bounded ZIP validation, MinIO storage, API import support, lifecycle payload references, database constraints, integration tests, and web metadata. ChangesSkill Package Registry
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.java`:
- Around line 133-156: Update the IOException handling in
AssetRegistryController.importSkill to pass failure as the cause when
constructing ApiRequestException, preserving the original stack trace. Use the
existing (String, Throwable) constructor if available; otherwise add that
constructor to ApiRequestException.
In `@core/build.gradle.kts`:
- Line 15: Declare SnakeYAML as an explicit dependency for the core library in
its Gradle configuration, preferably by adding or reusing a version-catalog
alias and referencing it alongside the existing dependencies. Ensure
SkillPackageInspector’s org.yaml.snakeyaml usage no longer relies on Spring
Boot’s transitive dependency.
In
`@core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java`:
- Around line 50-57: Update the column mappings for digest, mediaType, and
contentLength in AssetPayloadReference to enforce nullable = false, including
the corresponding schema ALTER. Ensure submit/publish conflict checks handle
future INLINE ReferenceKind metadata as nullable before invoking methods such as
isBlindReference, preventing null-row NPEs.
In
`@core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java`:
- Around line 821-829: Update the asset-profile contract used by skill payload
handling to expose parsing as a typed operation, then have skillSpec(String)
invoke that contract directly through AssetPayloadProfile (or the established
SkillPackageSpecReader collaborator). Remove the SkillPackageProfile instanceof
check, downcast, and wiring-time IllegalStateException so the coordinator no
longer depends on profile internals.
In
`@core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.java`:
- Around line 66-83: Update forkRelease to explicitly reject Skill releases
before routing into create or createValidatedSkillIdentity, using the message
“Skill releases cannot be forked yet.” Preserve the existing authorization and
source-release loading flow, and keep non-Skill release forking unchanged.
In
`@core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageInspector.java`:
- Around line 297-312: Update the metadata validation in the source.forEach
block to accept only String, Number, or Boolean values, rejecting dates and all
other scalar types with the existing IllegalArgumentException path. Preserve
string conversion for the allowed types, length validation, and result.put
behavior so persisted metadata is deterministic.
In
`@core/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.java`:
- Around line 93-115: Consolidate the duplicate deleteIfStored overloads into
one method accepting Throwable (or Exception), since addSuppressed supports that
type. Preserve the existing null check, storage.delete call, and suppressed
cleanup-failure behavior for both RuntimeException and IOException callers.
In `@core/src/main/resources/db/migration/V7__add_skill_asset_type.sql`:
- Around line 9-11: Update the asset_payload_references migration to add a CHECK
constraint tying content_length to reference_kind: BLOB references must have a
non-NULL content_length, while non-BLOB references retain existing behavior.
Apply the same constraint to both relevant table definitions or migration
sections, alongside asset_payload_reference_length_check.
In
`@core/src/test/java/com/orgmemory/core/assetregistry/AssetProfileValidationTests.java`:
- Around line 15-29: Extend everyEnabledTypeHasAnExplicitSchemaValidator with a
negative assertion for the Skill profile using skills.validate(...). Verify that
an invalid skill payload is rejected, covering either a non-hex digest or a
manifest missing SKILL.md, while preserving the existing valid skill validation.
In
`@core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageInspectorTests.java`:
- Around line 137-149: Extend SkillPackageInspectorTests with focused tests for
the advertised bounded-input checks: create an archive exceeding MAX_FILES and
assert SkillPackageValidationException, create an archive containing no files
and assert rejection, and create an entry whose data cannot be read (such as
encrypted or unsupported compression) and assert rejection. Reuse the existing
archive builders and inspector.inspect invocation patterns.
- Around line 199-201: Update the ZipArchiveEntry setup in
SkillPackageInspectorTests to replace the raw octal mode 0120777 with the
equivalent self-describing UnixStat constants, adding the UnixStat import and
preserving the symbolic executable-link mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a579068c-dbc9-47aa-a172-85bc0c4bd792
⛔ Files ignored due to path filters (5)
contracts/openapi.jsonis excluded by!contracts/openapi.jsondocs/increments/active/2026-07-27-skill-registry-package-foundation/design.mdis excluded by!docs/**docs/increments/active/2026-07-27-skill-registry-package-foundation/plan.mdis excluded by!docs/**docs/specs/domains/asset-registry.mdis excluded by!docs/**docs/tests/domains/asset-registry.mdis excluded by!docs/**
📒 Files selected for processing (24)
ARCHITECTURE.mdapps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.javaapps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.javacore/build.gradle.ktscore/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReferenceRepository.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetType.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageInspector.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSpec.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageStoragePort.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageValidationException.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.javacore/src/main/resources/db/migration/V7__add_skill_asset_type.sqlcore/src/test/java/com/orgmemory/core/assetregistry/AssetProfileValidationTests.javacore/src/test/java/com/orgmemory/core/assetregistry/SkillPackageInspectorTests.javacore/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.javagradle/libs.versions.tomlintegrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.javaintegrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.javaintegrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.javaweb/src/features/assets/asset-format.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Backend · Java 25
- GitHub Check: Web · Node 24
- GitHub Check: Deployment contracts
- GitHub Check: PostgreSQL GraphRAG
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Record current behavior in architecture/specification documentation only after it exists in code; keep intended behavior in vision, roadmap, or an active increment, and do not duplicate state.
Before using unfamiliar Spring Boot 4, Spring Modulith 2, Spring AI 2, Gradle, React, Vite, Tailwind, or TypeScript APIs, consult current official documentation via Context7 and the projectorgmemory-*verification skills.
Before retrieval, AI, MCP, permission, upload, graph, or export work, readdocs/guidelines/agent-safety.md.
Never commit.envfiles, provider keys, tokens, or customer data.
Run the relevant verification gates fromdocs/guidelines/testing-harness.md; use a terminating clean test as the context gate, and do not treatbootRunas verification.
Files:
core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageValidationException.javacore/build.gradle.ktscore/src/test/java/com/orgmemory/core/assetregistry/SkillPackageInspectorTests.javaARCHITECTURE.mdcore/src/main/java/com/orgmemory/core/assetregistry/AssetType.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.javacore/src/main/resources/db/migration/V7__add_skill_asset_type.sqlintegrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.javagradle/libs.versions.tomlcore/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReferenceRepository.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageInspector.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageStoragePort.javacore/src/test/java/com/orgmemory/core/assetregistry/AssetProfileValidationTests.javaweb/src/features/assets/asset-format.tscore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.javaintegrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSpec.javacore/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.javaintegrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.javaapps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.javaapps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.java
**/*.{java,kt}
📄 CodeRabbit inference engine (CLAUDE.md)
JetBrains IDE inspection is a verification gate for the Java backend.
Files:
core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageValidationException.javacore/src/test/java/com/orgmemory/core/assetregistry/SkillPackageInspectorTests.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetType.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.javaintegrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReferenceRepository.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageInspector.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageStoragePort.javacore/src/test/java/com/orgmemory/core/assetregistry/AssetProfileValidationTests.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.javaintegrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.javacore/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSpec.javacore/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.javaintegrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.javaapps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.javacore/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.javaapps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.java
core/src/main/resources/db/migration/*.sql
⚙️ CodeRabbit configuration file
core/src/main/resources/db/migration/*.sql: The repository is pre-release: V1 is the intentionally resettable clean
baseline and development data carries no migration cost. Once a release
baseline is frozen, later Flyway migrations are immutable. Check tenant
isolation, foreign keys, uniqueness, indexes, append-only evidence
semantics, safe defaults, and PostgreSQL 18 plus pgvector compatibility.
Files:
core/src/main/resources/db/migration/V7__add_skill_asset_type.sql
**/*.{ts,tsx,js,jsx,css,scss,html}
📄 CodeRabbit inference engine (CLAUDE.md)
For frontend files, run Oxlint, TypeScript typecheck, the production build, and browser tests when the UI flow matters; do not run JetBrains IDE inspection on TypeScript, TSX, or web configuration.
Files:
web/src/features/assets/asset-format.ts
web/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
web/src/**/*.{ts,tsx}: OAuth access and refresh tokens must never enter browser JavaScript or
browser storage. Use the HttpOnly BFF session, CSRF-protected mutations,
generated Hey API data clients, accessible states, and both light and
dark themes. Handwritten transport is reserved for documented protocol
flows such as navigation redirects and streaming.
Files:
web/src/features/assets/asset-format.ts
apps/api/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
apps/api/src/main/java/**/*.java: Enforce the browser-BFF and resource-server boundaries. Authentication
must resolve an active internal actor through the explicit issuer and
subject binding. Reject identity, tenant, roles, or permissions supplied
by request payloads, JWT email, or untrusted JWT role claims.
Files:
apps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.java
🧠 Learnings (2)
📚 Learning: 2026-07-23T23:30:44.585Z
Learnt from: kl3inIT
Repo: kl3inIT/OrgMemory PR: 30
File: core/src/main/resources/db/migration/V32__evidence_scoped_graph_semantics.sql:0-0
Timestamp: 2026-07-23T23:30:44.585Z
Learning: For OrgMemory PostgreSQL Flyway migrations under core/src/main/resources/db/migration, do not recommend using `CREATE INDEX CONCURRENTLY` or `DROP INDEX CONCURRENTLY` inside application-owned Flyway migration SQL. Flyway’s schema-history connection may hold a transaction that can cause concurrent index operations to wait indefinitely (e.g., on a `virtualxid`), and docs/conventions.md forbids this pattern. If you need large production-table index replacement, pre-stage online index operations via the deployment pipeline (outside Flyway) rather than inside the migration; “ordinary” index replacement is acceptable for unreleased projections before production traffic.
Applied to files:
core/src/main/resources/db/migration/V7__add_skill_asset_type.sql
📚 Learning: 2026-07-26T05:46:47.443Z
Learnt from: kl3inIT
Repo: kl3inIT/OrgMemory PR: 61
File: apps/mcp/src/main/java/com/orgmemory/mcp/McpSecurityConfiguration.java:50-52
Timestamp: 2026-07-26T05:46:47.443Z
Learning: In OrgMemory, treat the `apps/mcp` and `apps/api` as independent protocol adapter modules. When adjusting OAuth/wire-level scopes, do not introduce a shared Java constant or create a code dependency from `apps/mcp` to `apps/api` solely to deduplicate scope values. Instead, keep OAuth/scope constants adapter-local (e.g., in the relevant adapter/security configuration classes) and ensure cross-adapter consistency via automated realm/OAuth/authorization tests, rather than via shared wiring-level constants or cross-module references.
Applied to files:
apps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.java
🪛 ast-grep (0.44.1)
core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageInspector.java
[warning] 50-50: Temporary file not deleted
Context: Files.createTempFile("orgmemory-skill-", ".zip")
Note: [CWE-377] Insecure Temporary File. Security best practice.
(tempfile-delete)
apps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.java
[warning] 216-216: Avoid LDAP injections
Context: knowledgeSearch.search(any(), any(), any(), any())
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
🪛 PMD (7.26.0)
core/src/test/java/com/orgmemory/core/assetregistry/SkillPackageInspectorTests.java
[Medium] 200-200: AvoidUsingOctalValues (Error Prone): Avoid integer literals that start with zero (interpreted as octal), remove the leading 0 to get a decimal literal (or use explicit 0x, 0b prefixes)
(AvoidUsingOctalValues (Error Prone))
apps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.java
[Medium] 154-154: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'failure' on all code paths
(PreserveStackTrace (Best Practices))
🪛 Squawk (2.59.0)
core/src/main/resources/db/migration/V7__add_skill_asset_type.sql
[warning] 5-7: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 14-15: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 14-15: Adding a UNIQUE constraint requires an ACCESS EXCLUSIVE lock which blocks reads and writes to the table while the index is built. Create an index CONCURRENTLY and create the constraint using the index.
(disallowed-unique-constraint)
[warning] 21-36: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 37-39: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 40-42: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 40-42: Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to each table. Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.
(adding-foreign-key-constraint)
[warning] 44-46: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
[warning] 48-50: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
[warning] 52-54: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🔇 Additional comments (31)
gradle/libs.versions.toml (1)
15-15: LGTM!Also applies to: 34-34
ARCHITECTURE.md (1)
82-86: LGTM!core/src/main/java/com/orgmemory/core/assetregistry/AssetType.java (1)
6-7: LGTM!core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageInspector.java (7)
43-80: LGTM!
82-155: LGTM!
157-191: LGTM!
193-232: LGTM!
358-401: LGTM!
403-452: LGTM!
246-253: 🔒 Security & PrivacyNo change needed.
new Yaml(new SafeConstructor(options))applies theLoaderOptionsconfigured on theBaseConstructor, so the explicit overload is unnecessary.core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageProfile.java (1)
1-38: LGTM!core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageSpec.java (1)
24-91: LGTM!core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageValidationException.java (1)
6-22: LGTM!core/src/test/java/com/orgmemory/core/assetregistry/AssetProfileValidationTests.java (1)
146-167: LGTM!core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java (1)
91-132: LGTM!core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryService.java (1)
59-64: LGTM!Also applies to: 85-94
core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java (2)
33-33: LGTM!Also applies to: 48-61, 74-94, 133-142, 335-338, 525-535
403-413: 🗄️ Data Integrity & IntegrationNo schema change needed. The migration adds unique partial indexes on
(organization_id, draft_id),(organization_id, revision_id), and(organization_id, release_id), enforcing one payload reference per owner.core/src/main/resources/db/migration/V7__add_skill_asset_type.sql (2)
1-54: 🗄️ Data Integrity & IntegrationSquawk lock warnings not actionable for this migration.
Static analysis flags missing
NOT VALID/CONCURRENTLYon the new constraints and indexes. Per prior guidance for this repo,CONCURRENTLYoperations should not be used inside Flyway migrations here, and this repository is pre-release (V1 is a resettable baseline, dev data has no migration cost), so table-scan locking is not a real concern yet.Based on learnings, "do not recommend using
CREATE INDEX CONCURRENTLYorDROP INDEX CONCURRENTLYinside application-owned Flyway migration SQL" and, as per path instructions, "The repository is pre-release: V1 is the intentionally resettable clean baseline and development data carries no migration cost."Sources: Path instructions, Learnings, Linters/SAST tools
40-42: 🗄️ Data Integrity & IntegrationNo change needed. The existing tenant-scoped composite unique constraints on
asset_revisions(id, organization_id)andasset_releases(id, organization_id)support the cross-table FKs;asset_payload_referencesis the only reference table that needed this pattern, and it now uses it for all three payload reference targets.core/src/main/java/com/orgmemory/core/assetregistry/SkillPackageStoragePort.java (1)
1-63: LGTM!core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReferenceRepository.java (1)
1-19: LGTM!integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioObjectStorageAutoConfiguration.java (1)
3-3: LGTM!Also applies to: 35-40
integrations/object-storage-minio/src/main/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapter.java (1)
1-58: LGTM!integrations/object-storage-minio/src/test/java/com/orgmemory/integrations/storage/minio/MinioSkillPackageStorageAdapterTests.java (1)
1-80: LGTM!apps/api/src/main/java/com/orgmemory/api/assetregistry/AssetRegistryController.java (1)
12-19: LGTM!Also applies to: 28-56, 74-74, 121-131
core/src/main/java/com/orgmemory/core/assetregistry/SkillRegistryService.java (2)
31-81: LGTM!
82-91: 🗄️ Data Integrity & IntegrationNo dangling object risk here.
StagedSkillPackage.close()does not perform an I/O close at the staged file level, so it does not represent the checked-close exception window described; the existingRuntimeExceptionguard is appropriate.> Likely an incorrect or invalid review comment.core/src/test/java/com/orgmemory/core/assetregistry/SkillRegistryServiceTests.java (1)
1-153: LGTM!web/src/features/assets/asset-format.ts (1)
1-7: LGTM!Also applies to: 17-17, 42-47
apps/api/src/test/java/com/orgmemory/api/assetregistry/AssetRegistryIntegrationTests.java (1)
28-29: LGTM!Also applies to: 59-63, 143-145, 188-190, 204-216, 892-947, 1333-1352
| @Column(length = 64, updatable = false) | ||
| private String digest; | ||
|
|
||
| @Column(name = "media_type", length = 128, updatable = false) | ||
| private String mediaType; | ||
|
|
||
| @Column(name = "content_length", updatable = false) | ||
| private Long contentLength; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the migration actually enforces NOT NULL / constraints on asset_payload_references
fd -t f 'V7__add_skill_asset_type.sql' --exec cat -n {}
rg -n 'asset_payload_references' -A 30 core/src/main/resources/db/migrationRepository: kl3inIT/OrgMemory
Length of output: 15162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -t f 'AssetPayloadReference.java|.*Asset.*|Asset.*Coordinator|Asset.*Service|Reference|Coordinator' core/src/main/java | sed -n '1,120p'
echo
echo "== AssetPayloadReference.java =="
cat -n core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java
echo
echo "== references to digest/metatype/contentLength/getters =="
rg -n 'getDigest|getMediaType|getContentLength|mediaType|contentLength|ReferenceKind|INLINE|BLOB' core/src/main/java/com/orgmemory/core/assetregistry -A 3 -B 3
echo
echo "== migrations covering BLOB/INLINE constraint changes =="
rg -n 'asset_payload_reference|INLINE|BLOB|digest IS NULL|content_length IS NULL' core/src/main/resources/db/migration -C 3Repository: kl3inIT/OrgMemory
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Programmatically inspect entity constructor guarantees and accessor return types without running repo code.
python3 - <<'PY'
from pathlib import Path
import re
p = Path('core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java')
src = p.read_text()
m = re.search(r'public\s+AssetPayloadReference\s*\((.*?)\)\s*throws', src, re.S)
if not m:
print("constructor not found")
else:
body = re.search(r'public\s+AssetPayloadReference\s*\([^)]*\)\s*.*?\{\s*\n((?:(?!\n\s*\}).)*?)\n\s*\}', m.group(0), re.S)
print("constructor body:\n" + body.group(1).strip() if body else "none")
for name, field_type in [
("digest", re.search(r'private\s+(String)\s+digest\s*;', src)),
("mediaType",re.search(r'private\s+(String)\s+mediaType\s*;', src)),
("contentLength",re.search(r'private\s+(Long)\s+contentLength\s*;', src)),
]:
if field_type:
print(f"{name}: field type = {field_type.group(1)}")
for name, method in [
("digest", re.search(r'public\s+String\s+getDigest\s*\(\)\s*\{([^}]*)\}', src, re.S)),
("media_type", re.search(r'public\s+String\s+getMediaType\s*\(\)\s*\{([^}]*)\}', src, re.S)),
("content_length", re.search(r'public\s+Long\s+getContentLength\s*\(\)\s*\{([^}]*)\}', src, re.S)),
]:
print(f"{name} accessor:\n" + (method.group(1).strip() if method else "not found"))
if "content_length IS NULL OR content_length > 0" in Path('core/src/main/resources/db/migration/V2__asset_registry_foundation.sql').read_text():
print("migration allows content_length NULL")
if "content_length bigint;" in Path('core/src/main/resources/db/migration/V7__add_skill_asset_type.sql').read_text():
print("V7 adds content_length with no explicit NOT NULL in migration excerpt")
PYRepository: kl3inIT/OrgMemory
Length of output: 432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migration references to asset_payload_references reference_kind and constraints =="
rg -n "asset_payload_references|reference_kind|BLOB|INLINE|content_length IS NULL|content_length bigint|content_length" core/src/main/resources/db/migration -C 2 | sed -n '1,220p'
echo
echo "== AssetRegistryCoordinator requireMatchingPackage and payload reference comparisons =="
sed -n '828,852p' core/src/main/java/com/orgmemory/core/assetregistry/AssetRegistryCoordinator.java | cat -nRepository: kl3inIT/OrgMemory
Length of output: 10998
Mirror the BLOB metadata constraints in the entity.
AssetPayloadReference requires digest, mediaType, and contentLength in the constructor, but the columns remain nullable and digest.isBlindReference() always returns false. Add the corresponding nullable = false mapping/ALTER, and handle any future INLINE ReferenceKind metadata as nullable so null rows don’t cause NPEs during submit/publish conflict checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/orgmemory/core/assetregistry/AssetPayloadReference.java`
around lines 50 - 57, Update the column mappings for digest, mediaType, and
contentLength in AssetPayloadReference to enforce nullable = false, including
the corresponding schema ALTER. Ensure submit/publish conflict checks handle
future INLINE ReferenceKind metadata as nullable before invoking methods such as
isBlindReference, preventing null-row NPEs.
1 participant
Outcome
Adds the governed package foundation for company-scoped Skill assets without creating a second registry or marketplace.
What changed
SKILLas a code-owned Asset type while keeping generic JSON creation closed to SkillsPOST /api/assets/skillsVerification
gradlew clean testpassed (97 tasks)Deferred
Summary by CodeRabbit
New Features
Bug Fixes
Documentation