feat(RHIDP-17326): add pure OCI and npx skill metadata normalizers - #5072
fullsend-ai-coder[bot] wants to merge 4 commits into
Conversation
Implement design D3 native-field mappings and precedence for both OCI and npx sources in the ai-skills-common shared library. The normalizers accept parsed, verified inputs and return schema-valid SkillRecords or explicit per-record diagnostics. OCI precedence: metadata.display-name > metadata.name > frontmatter name for display name; metadata.version > frontmatter metadata.version > frontmatter version for declared version. npx precedence: frontmatter name > discovery entry name; frontmatter metadata.version > frontmatter version. Key behaviors: - Trim scalar strings; normalize author strings to author objects - Omit invalid optional values with diagnostics without failing records - Preserve only allowlisted extensions (oci.namespace, oci.prompt, npx.type) - Retain invalid first-choice version for D5 catalog fallback - Authors never imply catalog ownership; SkillCard namespace stays in extensions - Tags are trimmed, lowercased, deduplicated, and validated against Backstage catalog tag rules Adds OCI SkillCard/Markdown and npx index/frontmatter fixtures with conflicting-version scenarios. Tests validate all records pass the shared snapshot validator. Related to RHIDP-17326 Assisted-by: claude-opus-4-6
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
|
🤖 Finished Review · ✅ Success · Started 10:40 PM UTC · Completed 11:01 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.81 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5072 +/- ##
==========================================
+ Coverage 64.05% 64.10% +0.05%
==========================================
Files 2717 2718 +1
Lines 107661 107857 +196
Branches 30323 30394 +71
==========================================
+ Hits 68959 69147 +188
- Misses 36858 36878 +20
+ Partials 1844 1832 -12
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Risk Assessment: moderate (2/5) DetailsLarge line count (2207 lines) in a brand-new package is the primary risk driver, offset by bot authorship, no security or dependency concerns, no CI changes, no protected paths, and substantial test coverage (860 test lines for 803 implementation lines), yielding a moderate overall risk. Previous runRisk Assessment: moderate (2/5) DetailsScore of 2 (moderate) preserved from prior assessment: large additive PR (2086 lines, large blast radius) composed entirely of new files in a new shared library, bot-authored, with no protected paths, no CI or dependency changes, test ratio of 0.11, clean Tier 2 (all new files = 2), Tier 3 unavailable (external Jira); weights redistributed 62/38 yield composite 1.845, rounding to 2. Previous run (2)Risk Assessment: moderate (2/5) DetailsScore of 2 (moderate) reflects a large additive PR (1862 lines, large blast radius) that is entirely new files in a new shared library, bot-authored, with no protected paths, no CI or dependency changes, clean single-author git history, and strong per-LOC test coverage; Tier 3 unavailable (external Jira), weights redistributed 62/38, composite 1.54 rounds to 2. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix address and resolve review comments |
|
🤖 Finished Fix · ✅ Success · Started 7:10 PM UTC · Completed 7:29 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.86 |
- Mark task 1.2 as complete in tasks.md with narrative note - Add spec.md scenarios for tag validation, author normalization, diagnostics - Add README.md Normalization section documenting new functions and types - Add diagnostic for non-array tag candidates in normalizeTags - Emit diagnostics for non-string candidates at all priority levels - Use OciExtensions type instead of Record<string, string> - Export CATALOG_TAG_PATTERN and MAX_TAG_LENGTH as @public constants - Document unknown field types as deliberate v1 contract stability choice - Add @remarks to NpxDiscoveryEntry.type documenting accepted values - Add tests for OCI tag fallback, non-array tag diagnostic, and non-string fallback diagnostic - Update API report for new public exports Addresses #5072 Assisted-by: claude-opus-4-6
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 11 review findings: 10 fixed (documentation gaps in tasks.md, spec.md, README.md; missing diagnostics for non-array tags and non-string fallback candidates; OciExtensions type usage; exported behavioral constants; JSDoc for unknown type stability and NpxDiscoveryEntry.type accepted values; 3 new tests) and 1 disagreement (fixture @public visibility is intentional). All 119 tests pass. Fixed (10):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:31 PM UTC · Completed 7:54 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.84 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix address review comments |
|
🤖 Finished Fix · ❌ Failure (post-script /home/runner/work/rhdh-plugins/rhdh-plugins/.fullsend/.fullsend-cache/resources/sha256/f0b37e364cfe852baffb406950fba12d481781d1ec6d4ec0d4d22f581fa19dd8/scripts/post-fix.sh failed: exit sta…) · Started 8:00 PM UTC · Completed 8:18 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.01 |
Resolve the task checklist and generated ai-skills-common API report conflicts. Assisted-by: GPT-6
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/redhat-developer/rhdh-plugins/actions/runs/37057585913 Details: |
|
/fs-fix address review comments |
|
🤖 Finished Fix · ✅ Success · Started 8:20 PM UTC · Completed 8:36 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.76 |
- [HIGH] stale-spec-audit: Update audit.md to acknowledge task 1.2 completion with a fresh timestamp - [MEDIUM] missing-diagnostic-consistency: Add diagnoseNonStringCandidates calls for owner and lifecycle fields in both normalizeOciMetadata and normalizeNpxMetadata, with test coverage - [MEDIUM] hardcoded-tag-pattern-divergence: Add JSDoc documenting that CATALOG_TAG_PATTERN is a snapshot of Backstage's rule and not imported from @backstage/catalog-model (not a dependency of this library) - [LOW] api-shape-patterns: Extract shared MarkdownFrontmatter base interface with OciMarkdownFrontmatter and NpxFrontmatter as type aliases - [LOW] data-exposure: Truncate user-controlled values to 80 chars in diagnostic messages (tag values, entry.type) - [LOW] required-field-diagnostic-inconsistency: Document the intentional behavioral distinction in code comments (required name field uses terminal diagnostic vs per-candidate warnings for optional fields) Addresses #5072 Assisted-by: claude-opus-4-6
🔧 Fix agent — iteration 2 (human-triggered)Addressed all 7 review findings: 6 fixed (audit.md task 1.2 acknowledgment, owner/lifecycle diagnostic consistency with 4 new tests, CATALOG_TAG_PATTERN provenance documentation, shared MarkdownFrontmatter base interface, diagnostic value truncation, name field diagnostic documentation) and 1 disagreement (fixture @public visibility is intentional for cross-package test reuse). All 51 tests pass. Fixed (6):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:44 PM UTC · Completed 9:05 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.53 |
| * | ||
| * @public | ||
| */ | ||
| export const CATALOG_TAG_PATTERN = /^[a-z0-9:+#]([a-z0-9:+#._-]*[a-z0-9:+#])?$/; |
There was a problem hiding this comment.
[medium] Inconsistent validation logic
CATALOG_TAG_PATTERN (/^a-z0-9:+#?$/) allows dots and underscores in tags, while CATALOG_TAG_RE in catalog-helpers.ts (/^[a-z0-9:+#]+(-[a-z0-9:+#]+)*$/) does not. Both exist in the same package for tag validation but serve different code paths. The normalizer pattern appears to more faithfully reproduce Backstage actual tag pattern.
Suggested fix: Align the two patterns if both are intended to validate against the same Backstage tag rules. If they intentionally serve different purposes, document why they differ.
| * | ||
| * @public | ||
| */ | ||
| export const CATALOG_TAG_PATTERN = /^[a-z0-9:+#]([a-z0-9:+#._-]*[a-z0-9:+#])?$/; |
There was a problem hiding this comment.
[low] Exported constant stability
CATALOG_TAG_PATTERN is a hand-copied snapshot of Backstage tag validation regex, not imported from @backstage/catalog-model. The existing JSDoc at lines 263-269 already documents provenance and semver implications.
| * | ||
| * @public | ||
| */ | ||
| export const MAX_TAG_LENGTH = 63; |
There was a problem hiding this comment.
[low] Code organization
MAX_TAG_LENGTH = 63 is defined in both catalog-helpers.ts (private) and normalizer.ts (exported). The modules are deliberately uncoupled, but the two definitions could drift independently.
| * validated against Backstage's tag rules, and deduplicated. | ||
| * Invalid or overlength tags are omitted with diagnostics. | ||
| */ | ||
| function normalizeTags( |
There was a problem hiding this comment.
[low] Naming conventions
The private function normalizeTags(candidates, diagnostics) shares its name with the public export normalizeTags(tags) from catalog-helpers.ts. The functions have different signatures and semantics.
| raw, | ||
| )}' omitted — exceeds ${MAX_TAG_LENGTH} characters`, | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
[low] Edge case
When a tag is whitespace-only, the diagnostic reports does not match catalog tag pattern rather than indicating the tag was empty after trimming. Functionally correct but misleading.
Summary
Implements OpenSpec task 1.2: pure OCI and npx metadata normalizers for the
ai-skills-commonshared library.normalizeOciMetadata()— accepts parsed OCI SkillCard metadata and Markdown frontmatter with trusted source identity/integrity fields; returns schema-validOciSkillRecordor diagnostics following D3 precedence (metadata.display-name > metadata.name > frontmatter name; metadata.version > frontmatter metadata.version > frontmatter version)normalizeNpxMetadata()— accepts parsed npx discovery entry and frontmatter with trusted inputs; returns schema-validNpxSkillRecordor diagnostics following D3 precedence (frontmatter name > entry name; frontmatter metadata.version > frontmatter version)[{ name: value }]; accept SkillCard authors with non-empty name and optional email; omit invalid entries with diagnosticsextensions.oci.namespace,extensions.oci.prompt, andextensions.npx.type; SkillCard namespace stays in extensions, never overrides catalog namespace✔️ Checklist
Related to https://redhat.atlassian.net/browse/RHIDP-17326
Post-script verification
agent/RHIDP-17326-skill-metadata-normalizers)dc2f19a4fdb1f491c1e3696dfe9b59112510e99e..HEAD)