feat(#4220): add annotation spec and migration-readiness CLI - #4498
Conversation
Deliver RHIDP-15346 and RHIDP-15347 for the upstream schema alignment readiness epic (RHDHPLAN-1513). Part A - Annotation specification (RHIDP-15346): Publish formal spec at specifications/annotation-specification.md documenting all rhdh.io/ai-asset-* annotations (category, version, source), entity kind/spec.type mapping table, upstream mapping scenarios with confidence levels, fields requiring transformation, and an explicit future-work section. Cross-references ai-catalog-entity-model/design.md Decision 1 as source of truth. Part B - Migration-readiness CLI (RHIDP-15347): Scaffold backstage-plugin-boost-migration-readiness at plugins/boost-migration-readiness/. The CLI queries the Backstage catalog API, filters entities by rhdh.io/ai-asset-category, and produces a per-entity readiness report with current-to-target kind mapping, confidence levels, required transformations, and warnings for partial annotations or kind/type mismatches. Supports --output-format json|text, --token for auth, and --filter. Strictly read-only - no catalog writes. Imports annotation constants from boost-entity-provider-sdk. Includes 39 unit tests covering all 7 categories, partial/missing annotations, unrecognized categories, kind/type mismatches, both output formatters, and the no-McpServer-rename constraint. Closes #4220
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
|
🤖 Review · Commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4498 +/- ##
==========================================
- Coverage 62.00% 61.68% -0.32%
==========================================
Files 2597 2574 -23
Lines 104302 102690 -1612
Branches 29307 28727 -580
==========================================
- Hits 64671 63347 -1324
+ Misses 39032 37520 -1512
- Partials 599 1823 +1224
*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:
|
|
🤖 Finished Review · ✅ Success · Started 3:55 PM UTC · Completed 4:08 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $7.61 |
Review — ApprovePR: #4498 — feat(#4220): add annotation spec and migration-readiness CLI SummaryThis PR delivers two clearly scoped artifacts for the upstream schema alignment readiness epic (RHDHPLAN-1513): Part A — Annotation Specification (RHIDP-15346): Formal specification at Part B — Migration-Readiness CLI (RHIDP-15347): New Correctness
Security
Style & Conventions
Documentation
Observations (non-blocking)
Labels: PR adds new boost workspace plugin and specification document Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)Review — PR #4498Verdict: Approve | Reviewed at SummaryThis PR delivers two distinct deliverables for the upstream schema alignment readiness epic (RHDHPLAN-1513):
CorrectnessNo critical or high issues found. The code is well-structured with clear separation of concerns (catalog client → analyzer → formatters → CLI). Key observations:
Test coverage is strong: 39 tests covering all 7 categories, partial/missing annotations, unrecognized categories, kind/type mismatches, both output formatters, URL validation, auth headers, response validation, the no-McpServer-rename constraint, and entityRef construction. SecurityNo security issues found. Notable security-positive patterns:
Intent & CoherenceThe PR aligns well with issue #4220 and the linked RHIDP-15346/15347 stories. It:
Style & Conventions
Documentation
Follow-up Items (Low)
This is a first review. No prior review was found (provenance: none). Previous run (4)Review — PR #4498Verdict: Comment — 3 medium-severity findings worth noting; none blocking. SummaryThis PR delivers two authorized deliverables for issue #4220 (RHIDP-15346/15347):
Scope: Well-scoped to authorized work. No McpServer rename. No vector-store/ai-tool categories. No catalog writes. Sibling Strengths:
FindingsMedium1. Cross-document inconsistency — 2. Spec-code output mismatch — 3. Terminal injection via unsanitized warnings — Low4. No test coverage for CLI argument parser — 5. sanitize() misses carriage return — 6. Missing spec.type edge case untested — 7. --token flag missing from spec — 8. --token visible in process listings — Previous run (5)ReviewFindingsMedium
Low
Previous run (6)Review — ApprovePR: #4498 — feat(#4220): add annotation spec and migration-readiness CLI SummaryThis PR delivers two authorized deliverables for the upstream schema alignment readiness epic (RHDHPLAN-1513):
Review dimensions
FindingsLow — docs-gap ( VerdictApprove. The change is well-scoped, correctly implemented, thoroughly tested (39 tests across 4 test files), and follows established workspace patterns. The single low-severity finding is a minor documentation gap that does not block merging — the behavior is documented in Previous run (7)ReviewVerdict: Comment — medium-severity findings worth noting but none blocking. This PR delivers the two items authorized by #4220 (RHIDP-15346 + RHIDP-15347): an annotation specification document and a migration-readiness CLI. The implementation is well-structured, technically accurate against the design document source of truth, and includes 39 unit tests covering the core analysis engine, catalog client, formatters, and mapping rules. No scope creep was detected — the PR stays strictly within the authorized work and correctly excludes vector-store/ai-tool categories, actual migration, and RHIDP-15302 concerns. Verified correct
Medium findings1. CLI entry point has no test coverageFile: The Suggested remediation: Extract 2.
|
| # | Category | File | Description |
|---|---|---|---|
| 1 | edge-case | src/catalogClient.ts:63 |
URL construction via new URL(path, validatedBase.origin) strips any path component from the catalog URL. Path-prefixed reverse proxy setups would silently lose the prefix. |
| 2 | test-coverage-gap | src/analyze.test.ts |
No test covers the alreadyAligned=true + mismatch-warning contradiction (finding #2 above). The model-server test uses kind: 'Resource', never triggering the contradictory path. |
| 3 | edge-case | src/analyze.ts:72 |
Unrecognized category values are assigned via categoryValue as any, creating type unsoundness. Downstream consumers using assessment.category as a key into MAPPING_RULES would get undefined without a type error. |
| 4 | robustness | src/catalogClient.ts:76 |
response.json() result is cast to CatalogEntity[] without runtime validation. A misconfigured catalog could return non-array data, causing runtime crashes in analyzeEntities(). |
| 5 | doc-code-mismatch | annotation-specification.md |
Spec lists "Legacy Resource kind entities to API kind" as an MCP Server transformation, but MAPPING_RULES['mcp-server'].transformations does not include this item. |
| 6 | missing-doc-update | workspaces/boost/README.md |
Workspace-level Plugins table does not list the new boost-migration-readiness package (note: table is already incomplete for other packages). |
What Looks Good
- Scope alignment: Both issue deliverables (Part A annotation spec, Part B CLI) are present and match the authorized work. No scope creep.
- Constraint enforcement: The no-
McpServer-rename constraint is enforced by dedicated tests in bothmappings.test.tsandanalyze.test.ts. - SDK integration: Annotation constants are correctly imported from
boost-entity-provider-sdkrather than duplicated. - Design doc consistency: Mapping tables match Decision 1 in
ai-catalog-entity-model/design.md. - Security: URL protocol validation, token scoped to Authorization header, no secrets in code. SSRF is not a concern since this is a user-invoked CLI tool.
- Cross-repo contracts: No existing interfaces modified.
pluginPackagesarray updates follow established patterns with correct alphabetical ordering. - Read-only guarantee: No catalog writes; footer explicitly states assessment-only.
This is a first review. No prior review exists for this PR.
Previous run (10)
Review — Approve
PR: feat(#4220): add annotation spec and migration-readiness CLI
This PR delivers RHIDP-15346 (annotation specification) and RHIDP-15347 (migration-readiness CLI) for the upstream schema alignment readiness epic. The change is well-structured, correctly scoped, and matches the requirements in issue #4220.
What was reviewed
-
Part A — Annotation specification (
specifications/annotation-specification.md): 325-line specification documenting all sevenrhdh.io/ai-asset-*annotations, entity kind/spec.type mappings, upstream mapping scenarios with confidence levels (high → low), transformation requirements, and explicit future-work framing. Cross-referencesai-catalog-entity-model/design.mdDecision 1 as source of truth. -
Part B — Migration-readiness CLI (
plugins/boost-migration-readiness/): Newnode-librarypackage that queries the Backstage catalog API, filters byrhdh.io/ai-asset-category, and produces per-entity readiness assessments. Supports--output-format json|text,--token, and--filter. Strictly read-only. Imports annotation constants fromboost-entity-provider-sdk.
Strengths
- Clean architecture: Clear separation — types, mappings, analysis, catalog client, formatters, CLI, and public API index. Each module has a single responsibility.
- Correct use of SDK constants: Imports
AI_ASSET_CATEGORY_ANNOTATION,AI_ASSET_VERSION_ANNOTATION,AI_ASSET_SOURCE_ANNOTATION, andAIAssetCategorySchemafromboost-entity-provider-sdkrather than duplicating values. - Thorough core tests: 39 tests covering all 7 categories, partial/missing annotations, unrecognized categories, kind/type mismatches, both formatters, entityRef construction, and the no-
McpServer-rename constraint. - Explicit case-sensitivity in
alreadyAligned: TheAIResource→AiResourcecasing difference is correctly treated as a real migration step, not trivial equivalence. - URL validation:
catalogClient.tsvalidates protocol (http/https only) before making requests. - Package scaffolding:
pluginPackagesarrays synced across all sibling packages,yarn.lockupdated, API report file generated.
Findings
All findings are low severity — none block merging.
1. No changeset file (low, style/conventions)
The repo convention (per .fullsend/AGENTS.md) is to include a changeset with minor bump for new features. No .changeset/ entry is present for the new boost-migration-readiness package. This can be added before or after merge depending on the team's workflow.
2. No tests for catalogClient.ts (low, correctness)
The catalog client module — URL validation, error handling, token header attachment, filter query param — has no unit tests. The core analysis and formatter logic is well-tested, but the I/O boundary (fetchEntities) is not. A test with a mocked fetch would cover: valid/invalid URLs, non-http protocols, 401/500 responses, token presence/absence, and filter parameter propagation.
3. No tests for cli.ts (low, correctness)
The CLI argument parser (parseArgs) handles --catalog-url (required), --output-format validation, --token, --filter, --help, and unknown argument rejection. None of these paths are tested. Since the parser calls process.exit, testing would need a wrapper or spawn approach.
4. fetchEntities response cast without runtime validation (low, correctness)
The response is cast via (await response.json()) as CatalogEntity[] without validating the actual shape. If the Backstage API returns an unexpected format (e.g., wrapped in { items: [...] } or an error body that passes response.ok), this would silently produce incorrect data. For v0.1.0 this is acceptable, but a Zod schema for the response would harden the integration.
Verdict
The implementation is correct, well-tested where it matters most (analysis logic and formatters), properly scoped to read-only assessment, and aligned with the issue requirements. The annotation specification is comprehensive and accurately reflects the current upstream landscape. Low-severity findings are noted for follow-up but do not block.
Approve ✅
Labels: PR adds annotation specification doc and new boost migration-readiness CLI plugin package
Previous run (11)
Review — PR #4498
Verdict: Approve ✅
Summary
This PR delivers RHIDP-15346 (annotation specification) and RHIDP-15347 (migration-readiness CLI) for the upstream schema alignment readiness epic, closing #4220. Both parts are well-implemented, properly scoped, and match the issue requirements.
Part A — Annotation Specification: Published at workspaces/boost/specifications/annotation-specification.md. Covers all seven AI asset categories, three rhdh.io/ai-asset-* annotations, entity kind/spec.type mapping table, six upstream mapping scenarios with confidence levels, field transformations, and an explicit future-work section. Correctly cross-references ai-catalog-entity-model/design.md Decision 1 as source of truth. The mapping table matches Decision 1 exactly.
Part B — Migration-Readiness CLI: New package at plugins/boost-migration-readiness/ with role node-library. Implements catalog entity fetching, per-entity readiness analysis, and JSON/text output formatting. Imports annotation constants from boost-entity-provider-sdk. 39 unit tests cover all 7 categories, partial/missing annotations, unrecognized categories, kind/type mismatches, both output formatters, entity ref construction, and the no-McpServer-rename constraint. Strictly read-only — no catalog writes.
Correctness
- Mapping rules match
ai-catalog-entity-model/design.mdDecision 1 exactly — seven categories with correct kind, spec.type, confidence levels, and upstream RFC references. alreadyAlignedlogic correctly uses case-sensitive kind comparison (AIResource ≠ AiResource), which is intentional since casing alignment is a real migration step.- Kind/type mismatch detection correctly uses case-insensitive comparison for detecting unexpected entity kinds.
- Entity ref construction properly defaults namespace to
'default'when unspecified. - Zod validation reuses
AIAssetCategorySchemafrom the SDK, ensuring the category enum stays in sync. - Test coverage is comprehensive across all analysis dimensions.
Security
- Read-only operation confirmed — no catalog writes, mutations, or config changes.
- Bearer token passed via
Authorizationheader (standard pattern). - URL construction uses the
URLconstructor, avoiding injection in query parameters. - No sensitive data logged or exposed in outputs.
Intent & Coherence
- PR scope matches issue Upstream Schema Alignment — Annotation Spec & Migration-Readiness CLI (RHIDP-15346/15347; split from #4042) #4220 requirements precisely — annotation spec + CLI tool, no more.
- Correctly avoids
kind: McpServerrename per issue constraints and gate decisions. - MCP server mapped as
APIkind withMcpServerApiEntitymodel — aligned with upstream backstage#34016 (Option 3). - Assessment-only framing is consistent throughout (README footer, text formatter footer, annotation spec §6).
- The
workspace:^dependency onboost-entity-provider-sdkfollows the monorepo convention.
Style & Conventions
- Apache 2.0 license headers on all source files.
- Package structure mirrors sibling plugins (
.eslintrc.js,CHANGELOG.md,README.md,report.api.md,package.json). pluginPackagesarrays updated in all 8 siblingpackage.jsonfiles — alphabetically ordered and consistent.yarn.lockupdated with new package resolution.- JSDoc documentation on all public exports.
@public/@internalTSDoc tags used appropriately.
Documentation
- README covers CLI usage, options table, output interpretation (confidence levels, entity categories), programmatic API, upstream tracking links, and future work.
- Annotation specification is thorough and well-structured with clear section organization.
report.api.mdprovides the public API surface contract.
Findings
1. No changeset for new package (low)
The PR adds a new package at version 0.1.0 but does not include a .changeset/*.md file. Per workspace conventions, a changeset with minor bump should typically accompany new features. However, initial package creation at 0.1.0 may not require one — verify with the project's changeset policy.
File: workspaces/boost/.changeset/ (missing entry)
2. fetchEntities does not paginate (low)
The catalog client fetches entities with a single GET /api/catalog/entities call without pagination parameters. While the Backstage catalog API returns all entities by default, very large catalogs may benefit from paginated fetching. Consider adding pagination support as a follow-up.
File: workspaces/boost/plugins/boost-migration-readiness/src/catalogClient.ts, line 42
3. No unit tests for catalogClient.ts (low)
The fetchEntities HTTP client function has no direct unit tests. Error handling (non-200 responses), auth header injection, URL construction with filters, and the response type assertion are untested. The core analysis logic is well-tested, but the I/O boundary is not.
File: workspaces/boost/plugins/boost-migration-readiness/src/catalogClient.ts
Labels: PR adds new plugin package and specification document in the boost workspace
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
workspace/boost
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
🤖 Finished Review · ✅ Success · Started 6:51 PM UTC · Completed 6:58 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.52 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
/fs-fix The SonarCloud quality gate is failing (new_security_rating: C, needs better than C). Pasting the findings here since the fix agent can't reach sonarcloud.io directly. Blocking the quality gate:
Other new-code issues flagged (not gate-blocking, but clean up if you touch these files anyway):
Priority: fix the SSRF vulnerability so the quality gate passes. The code smells are secondary. |
|
🤖 Finished Fix · ✅ Success · Started 7:00 PM UTC · Completed 7:13 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.82 |
Add URL validation in fetchEntities() to reject non-http(s) protocols, addressing the SSRF vulnerability flagged by SonarCloud (tssecurity:S8703). Also refactors parseArgs() and formatText() to reduce cognitive complexity below the SonarCloud threshold (S3776), combines multiple Array#push() calls (S7778), and converts repetitive formatText tests to parameterized it.each() assertions (S5976). Addresses review feedback on #4498
|
🤖 Review · ❌ Terminated · Started 7:15 PM UTC · Ended 7:22 PM UTC Commit: |
|
/fs-fix Address all comments |
|
🤖 Fix · ❌ Terminated · Started 7:18 PM UTC · Ended 7:34 PM UTC Commit: |
|
🤖 Finished Review · ✅ Success · Started 5:53 PM UTC · Completed 6:13 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $9.46 |
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.
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-fix Address all comments in #4498 (comment) grounded in facts using TDD methodology. Tests must provide behavioral assurance against business acceptance criteria and verify business-level behavior. |
|
🤖 Finished Fix · ✅ Success · Started 6:17 PM UTC · Completed 6:35 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $3.87 |
- Fix source-of-truth contradiction in annotation-specification.md: replace "supersedes" with "documents...which have diverged" language to align with Decision 1 being the authoritative source of truth - Fix stale footer quote in design.md: update "pending upstream stabilization" to "pending upstream RFC finalization" to match formatters.ts and spec.md - Fix edge case in analyze.ts: emit Kind/type mismatch warning when spec.type is missing/empty but the mapping expects a value, by removing the falsy guard on currentSpecType - Fix DEL (0x7f) gap in sanitize() regex in formatters.ts - Fix stale argument list in tasks.md: add --token to CLI args Tests added (TDD): 3 new tests verifying spec.type edge case and DEL character sanitization behavior (65 total, all passing). Addresses review feedback on #4498
|
🤖 Review · ❌ Terminated · Started 6:37 PM UTC · Ended 7:20 PM UTC Commit: |
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.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 6:37 PM UTC · Completed 7:19 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
|
/fs-fix Address comments from #4498 (comment) grounded in facts using TDD methodology. Tests must provide behavioral assurance against business acceptance criteria and verify business-level behavior. |
|
🤖 Finished Fix · ✅ Success · Started 7:23 PM UTC · Completed 7:40 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $3.88 |
…ests - Extract parseArgs into separately importable module with 22 behavioral tests covering required flags, format validation, BACKSTAGE_TOKEN env var precedence, --flag=value syntax, error handling, and --help - Update annotation-specification/spec.md to match actual connector-emitted kinds: agent uses AiResource/agent, model-server uses AiModelServerAPI - Update migration-readiness-tooling/spec.md confidence level list to include all four levels (high/medium-high/medium-low/low) - Document formatJson terminal safety: JSON preserves raw entity data including DEL (U+007F) by design, with behavioral test documenting the intentional choice - Add README security note recommending BACKSTAGE_TOKEN env var over --token flag for production use Addresses review feedback on #4498
|
🤖 Review · ❌ Terminated · Started 7:41 PM UTC · Ended 7:49 PM UTC Commit: |
|
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 7:41 PM UTC · Completed 7:48 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.88 |
|
🤖 Finished Retro · ✅ Success · Started 8:15 PM UTC · Completed 8:36 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.80 |
Retro: PR #4498 — Annotation spec & migration-readiness CLITimelineIssue #4220 was created Aug 10 with detailed requirements for an annotation specification document and a migration-readiness CLI (RHIDP-15346/15347). After 4 successful triage runs ($0.74–$0.87 each), the code agent was dispatched on Aug 27 but failed 4 times due to The PR (3,164 additions, 36 files) then entered an extended fix-review cycle: 8 What went well
What could improve
Evidence for existing issues
Proposals filed
Proposals skipped (target repo not allowed)File manually or update
|
3 participants
Deliver RHIDP-15346 and RHIDP-15347 for the upstream schema alignment readiness epic (RHDHPLAN-1513).
Part A - Annotation specification (RHIDP-15346): Publish formal spec at specifications/annotation-specification.md documenting all rhdh.io/ai-asset-* annotations (category, version, source), entity kind/spec.type mapping table, upstream mapping scenarios with confidence levels, fields requiring transformation, and an explicit future-work section. Cross-references ai-catalog-entity-model/design.md Decision 1 as source of truth.
Part B - Migration-readiness CLI (RHIDP-15347): Scaffold backstage-plugin-boost-migration-readiness at plugins/boost-migration-readiness/. The CLI queries the Backstage catalog API, filters entities by rhdh.io/ai-asset-category, and produces a per-entity readiness report with current-to-target kind mapping, confidence levels, required transformations, and warnings for partial annotations or kind/type mismatches. Supports
--output-format json|text, --token for auth, and --filter.
Strictly read-only - no catalog writes.
Imports annotation constants from boost-entity-provider-sdk. Includes 39 unit tests covering all 7 categories, partial/missing annotations, unrecognized categories, kind/type mismatches, both output formatters, and the no-McpServer-rename constraint.
Closes #4220
Post-script verification
agent/4220-annotation-spec-migration-cli)1bb88d72e5db4b79809daf45b7b60f28077bb1da..HEAD)