Repository navigation
feat(RHIDP-17325): add skills-common shared contract library - #5061
Conversation
Create the skills-common package under workspaces/ai-integrations that defines the shared v1 SkillRecord and SkillSnapshot contract for OCI and npx skill connectors and the common catalog provider. Implements OpenSpec task 1.1 in full and the shared-library portions of cross-cutting task 1.4 (schema validation, ordering, response/ count limits, invariant tests, REST contract documentation). The library exports: - v1 SkillRecord, OciSkillRecord, NpxSkillRecord, SkillSnapshot types with source-discriminated extension validation - Runtime validators enforcing schema version, status invariants (loading/ready/partial/failed), unique keys, disjoint successful/failed key sets, digest format, and count bounds - Deterministic bounded snapshot construction with stable-key ordering and longest-prefix selection within 1,000-record and 5 MiB serialized-size limits - GET /skills/:sourceId REST contract constants and documentation - Reusable OCI and npx valid/invalid contract fixtures The library performs no network, scheduler, database, or Catalog operations. Related to RHIDP-17325 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 4:36 PM UTC · Completed 4:58 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.74 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5061 +/- ##
==========================================
+ Coverage 63.94% 64.01% +0.06%
==========================================
Files 2712 2716 +4
Lines 107212 107558 +346
Branches 30237 30322 +85
==========================================
+ Hits 68560 68855 +295
- Misses 38089 38140 +51
Partials 563 563
*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: elevated (3/5) DetailsA large new shared-library introduction (3034 lines across 19 files, 14 brand-new) with two dependency file changes sustains elevated risk; Tier 1 signals are unchanged from the prior assessment, anchoring preserves the score of 3 despite bot authorship and absence of security-sensitive or CI changes partially offsetting the scale. Previous runRisk Assessment: elevated (3/5) DetailsA large new shared-library introduction (2643 lines across 14 new files) with two dependency file changes drives elevated risk; bot authorship and absence of security-sensitive or CI changes partially offset the scale. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsHigh
Low
Next steps:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
johnmcollier
left a comment
There was a problem hiding this comment.
Are there OpenSpec resources that can be resolved with this change?
|
/fs-fix address review comments, comments from sonar |
|
🤖 Finished Fix · ✅ Success · Started 6:59 PM UTC · Completed 7:21 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.42 |
- Fix type-guard ambiguity: isOciSkillRecord and isNpxSkillRecord now require sourceType parameter to disambiguate records without extensions - Fix applyByteLimit fast-path placeholder to use 'partial' (longest status value) instead of 'ready' to prevent 2-byte overrun at boundary - Add SkillSnapshotV1 type alias for forward-compatibility - Add field-level JSDoc to SkillAuthor, OciExtensions, NpxExtensions, ValidationResult, OciSkillRecord.extensions, NpxSkillRecord.extensions - Document closed union semantics on SkillSourceType, SnapshotStatus, and NpxExtensions.type - Expand fixture JSDoc from compact inline to multi-line block form - Add missing API docs to README: isValidUtcTimestamp, sortRecordsByKey, and Constants section with MAX_SNAPSHOT_RECORDS, MAX_SNAPSHOT_BYTES, MAX_RESPONSE_BYTES, SUPPORTED_SCHEMA_VERSION - Add negative test for validateSnapshotSize exceeding MAX_SNAPSHOT_BYTES - Add negative/mutual-exclusion tests for type guards - Regenerate report.api.md (removes all (undocumented) markers) Addresses #5061 Assisted-by: claude-opus-4-6
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 14 review findings: fixed the HIGH type-guard ambiguity by adding sourceType parameter, fixed the applyByteLimit off-by-2 logic error, added 6 missing tests (1 negative validateSnapshotSize + 5 type guard mutual exclusion), added comprehensive JSDoc documentation (field-level, closed-union, closed-literal), added SkillSnapshotV1 forward-compatibility alias, and expanded README with missing API docs and Constants section. One finding (fixtures @alpha/@internal) was partially addressed with expanded JSDoc but could not change the release tag due to Backstage toolchain constraints. Fixed (13):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
|
/fs-fix confirm if openspec tasks need to be resolved |
|
🤖 Finished Fix · ✅ Success · Started 7:23 PM UTC · Completed 7:33 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.01 |
Check off task 1.1 (SkillRecord/SkillSnapshot schemas and validators) as fully implemented by the skills-common package. Annotate task 1.4 with shared-library completion status (extension validation, ordering, status invariants, version/key/count/size checks, REST contract docs done; discovery budgets and pagination-cycle tests remain for connector scope). Update the footnote and audit timestamp accordingly. Addresses #5061 Assisted-by: claude-opus-4-6
🔧 Fix agent — iteration 2 (human-triggered)Confirmed openspec tasks need resolution. Checked off task 1.1 as fully implemented. Annotated task 1.4 with shared-library completion status. Updated audit.md timestamp per AGENTS.md freshness rules. Tasks 1.2, 1.3, and 2.x-5.x remain for future connector/provider PRs. Fixed (2):
Tests: passed Decision points
Next steps:
|
|
/fs-fix fix address failing sonarcloud checks |
|
🤖 Finished Fix · ✅ Success · Started 2:27 PM UTC · Completed 2:48 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.72 |
Resolve SonarCloud Quality Gate failure (D Reliability Rating) and
associated code smells:
- snapshot.ts: Add localeCompare comparator to failedSkillKeys sort
(S2871 — sort without compare function, the Bug causing D rating)
- validation.ts: Replace isNaN with Number.isNaN (S4325)
- validation.ts: Collapse else { if } into else if (S1066)
- validation.ts: Reduce cognitive complexity of validateBaseRecord
(23 → ~3) by extracting validateRequiredRecordFields,
validateOptionalRecordFields, validateAuthorsField, validateTagsField
- validation.ts: Reduce cognitive complexity of validateOciExtensions
(17 → ~10) by extracting validateOciContainerKeys
- validation.ts: Reduce cognitive complexity of validateNpxExtensions
(16 → ~10) by extracting validateNpxContainerKeys
- validation.ts: Reduce cognitive complexity of validateSnapshot
(49 → ~4) by extracting validateSnapshotShape, validateObservedAt,
validateStatusInvariants, validateSnapshotRecords, validateDisjointKeys
All 71 existing tests pass. No behavior changes — pure structural
refactoring and targeted fixes.
Addresses #5061
Assisted-by: claude-opus-4-6
🔧 Fix agent — iteration 3 (human-triggered)Addressed all 7 SonarCloud findings: fixed the Bug-severity sort issue (S2871) causing the D Reliability Rating quality gate failure, replaced isNaN with Number.isNaN (S4325), collapsed else-if pattern (S1066), and reduced cognitive complexity of 4 functions (validateBaseRecord 23→ Fixed (7):
Tests: passed Decision points
Next steps:
|
|
/fs-fix address my review comment on the plugin name |
|
🤖 Finished Fix · ✅ Success · Started 2:54 PM UTC · Completed 3:09 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.04 |
Rename the plugin package from backstage-plugin-skills-common to backstage-plugin-ai-skills-common per reviewer feedback that the name should be more unique. The ai- prefix follows the workspace naming convention (e.g., ai-experience-common). - Renamed directory plugins/skills-common to plugins/ai-skills-common - Updated package.json name, pluginId, pluginPackage, pluginPackages - Updated all references in changeset, README, report.api.md, openspec docs - Updated yarn.lock Addresses #5061 Assisted-by: claude-opus-4-6
🔧 Fix agent — iteration 4 (human-triggered)Renamed plugin from skills-common to ai-skills-common per reviewer feedback that the name should be more unique. Follows the workspace ai- prefix convention. Fixed (1):
Tests: passed Decision points
Next steps:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 3:17 PM UTC · Completed 3:39 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.79 |
|
/fs-fix address and resolve review comments |
|
🤖 Finished Fix · ✅ Success · Started 3:53 PM UTC · Completed 4:11 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.61 |
- Replace localeCompare() with Unicode code-point comparison for failed key sorting in boundSnapshot(), matching the ordering used by sortRecordsByKey() per design D2 - Add portable utf8ByteLength helper to validateSnapshotSize() using TextEncoder with Buffer.byteLength fallback, making the public API safe for frontend (common-library) consumers - Fix validateAuthorsField() to capture and return validateAuthor() boolean result, matching the pattern used by validateTagsField() - Add package.json description field - Add non-ASCII key test verifying code-point sort ordering Addresses #5061 Assisted-by: claude-opus-4-6
🔧 Fix agent — iteration 5 (human-triggered)Addressed 4 review findings with code fixes (localeCompare ordering, Buffer.byteLength portability, validateAuthorsField return value, missing package.json description) and added a non-ASCII key test. Disagreed with 2 low-priority findings (fixture subpath export is pre-1.0 scope, closed unions are already documented as intentional). Fixed (4):
Disagreed (2):
Tests: passed
Decision points
Next steps:
|
|
Summary
skills-commonshared library package underworkspaces/ai-integrations/plugins/implementing the v1 SkillRecord and SkillSnapshot contract defined in OpenSpec design D2/D6SkillRecord,OciSkillRecord,NpxSkillRecord,SkillSnapshot), runtime validators with source-discriminated extension checking, bounded snapshot construction with deterministic stable-key ordering, and REST contract documentation forGET /skills/:sourceIdOpenSpec scope
Implements task 1.1 fully and the shared-library portions of cross-cutting task 1.4 (validation, ordering, response/count limits, invariant tests, REST contract). Leaves 1.4 open for OCI/npx discovery budgets and pagination-cycle tests (connector scope).
Testing
boundSnapshotproduces valid snapshots pervalidateSnapshotcreateLoadingSnapshotandcreateFailedSnapshothelpersyarn lint,yarn tsc,yarn prettier:check, andyarn build:api-reports:only --cipass✔️ Checklist
Related to https://redhat.atlassian.net/browse/RHIDP-17325
Post-script verification
agent/RHIDP-17325-skills-common-contract)ef7a4d0145f3e89675a8e327cb243dd143522b50..HEAD)