feat(#4813): add MCP registry server mapping common library plugin - mcp-registry-server-mapping - #4823
feat(#4813): add MCP registry server mapping common library plugin - mcp-registry-server-mapping#4823fullsend-ai-coder[bot] wants to merge 10 commits into
mcp-registry-server-mapping#4823Conversation
Implement the direct field mapping transform from MCP Registry server.json to Backstage mcp-server API entity (Groups 1-2 from tasks.md). This is the first half of the mcp-registry-server-mapping capability; annotation projection (Group 3) lands in #4795. New package: @red-hat-developer-hub/backstage-plugin-mcp-registry- server-mapping (common-library role). What changed: - mapServerToEntity(): pure, deterministic transform producing a valid API entity with spec.type: mcp-server, spec.remotes[], no spec.definition - Identity derivation (D4): metadata.name from sanitized prefix__name__version with stable hash suffix when sanitization mutates or length exceeds 63 chars - D11 URL scheme policy: allowlist absolute http/https only on all emitted URL fields; no host classification or DNS resolution - D8 placeholder remote: type "undefined" with websiteUrl when no valid remotes; actionable error when websiteUrl also unavailable - SCM-aware repository URL combination (D10): github/gitlab/ bitbucket/azure-devops subfolder templates with HEAD ref - Caller defaults: prefix (mcp.registry), owner (unknown), lifecycle (production) with override support - Hand-off contract: consumed paths and reserved annotation keys supplied for the annotation projection sibling - mapping-reference.md: field-mapping table, annotation key rules, caller defaults, D11 policy documentation 89 tests covering entity shape, identity, remotes, repository URL combination, D11 URL policy, determinism, validation, and hand-off. Closes #4813 Assisted-by: claude-opus-4-6 Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add the generated report.api.md for the new mcp-registry-server-mapping package and fix missing @public release tags on DEFAULT_PREFIX and RepositoryUrlResult. Escape @ in TSDoc comment to resolve api-extractor warnings. Closes #4813 Assisted-by: claude-opus-4-6 Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4823 +/- ##
==========================================
+ Coverage 63.28% 63.36% +0.07%
==========================================
Files 2675 2679 +4
Lines 106507 106738 +231
Branches 29819 29893 +74
==========================================
+ Hits 67401 67631 +230
+ Misses 37326 37318 -8
- Partials 1780 1789 +9
*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:25 PM UTC · Completed 3:40 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $6.26 |
ReviewFindingsMedium
Low
Labels: PR implements MCP registry server mapping in the ai-integrations workspace. Previous runReview — request-changesPR: #4823 — feat(#4813): add MCP registry server mapping common library plugin
SummaryThis PR adds a well-structured common library plugin ( The code quality is high, the test suite is thorough (identity, URL policy, mapping, remotes, repository combination), and the design aligns well with the authorized scope from issue #4813. However, one blocking issue and several medium-severity concerns need to be addressed before merge. Blocking1.
|
| # | Finding | File |
|---|---|---|
| 6 | pluginId mismatch: set to mcp-registry-provider but directory is mcp-registry-server-mapping-common |
package.json:21 |
| 7 | Spec says "while" for boundary normalization but code uses single "if" (functionally equivalent) | identity.ts:84 |
| 8 | normalizeBase doesn't re-strip trailing / after .git removal (double-slash edge case) |
repository.ts:53 |
| 9 | Subfolder path segments not URL-encoded for non-Azure SCMs (limited impact) | repository.ts:126 |
| 10 | Missing co-located repository.test.ts; sibling plugins use one-module-one-test pattern |
repository.ts |
| 11 | LinksResult and McpRegistryRemote properties lack JSDoc (undocumented in report.api.md) |
types.ts:33 |
| 12 | No test for empty title: '' edge case (consumed but not emitted, D12 relevance) |
mapServerToEntity.test.ts |
Previous run (2)
Review — request-changes
Summary
This PR implements a new @red-hat-developer-hub/backstage-plugin-mcp-registry-server-mapping common library plugin that deterministically transforms MCP Registry server.json documents into Backstage mcp-server API entities. The implementation is well-structured with comprehensive tests (identity derivation, URL scheme policy, main mapping, repository URL combination) and a clear separation of concerns.
One blocking issue: the PR includes an out-of-scope change to the root package.json that removes the repository URL, which must be reverted. Several medium-severity findings about runtime validation and API contract documentation also require attention.
Findings
🔴 High
1. Root package.json scope creep — repository URL removed · package.json
The root package.json repository field was changed from a URL string to an object that omits the required url property:
- "repository": "[email protected]:redhat-developer/rhdh-plugins.git",
+ "repository": {
+ "type": "git",
+ "directory": "."
+ },Per npm's spec, the object form requires a url field. The new plugin's own package.json correctly includes url, making this inconsistent. This change is unrelated to issue #4813 and affects the entire monorepo. Tools that read repository.url (npm repo, GitHub dependency graph, Renovate/Dependabot) will lose the URL.
Remediation: Revert this change entirely, or add the url field to the object.
🟡 Medium
2. Missing runtime validation for remote.type · mapServerToEntity.ts
McpServerRemote declares type as a required string, but input comes from untrusted JSON where type could be undefined or null at runtime. The code checks remote.url before use but uses remote.type directly without validation. A malformed remote entry missing type would produce type: undefined in the entity, violating the McpServerEntityRemote contract.
Remediation: Add a runtime check: typeof remote.type === 'string' && remote.type.length > 0.
3. Consumed-path tracking asymmetry · mapServerToEntity.ts
websiteUrl is added to consumedPaths regardless of D11 outcome (preventing the projection sibling from re-projecting refused URLs), but remotes[].url is only consumed when D11 passes. This creates an implicit contract: the projection sibling must independently enforce D11 for remote URLs but not for websiteUrl. If the sibling relies solely on consumed-path tracking, refused remote URLs would leak into annotations.
Remediation: Either consume refused remote URLs symmetrically, or document the asymmetry in McpServerMappingResult.consumedPaths JSDoc.
4. McpServerApiEntity omits spec.definition — undocumented deviation · types.ts
The standard Backstage ApiEntity requires spec.definition: string. This entity type intentionally omits it (tests assert not.toHaveProperty('definition')), but the deviation is undocumented. Downstream code casting to the standard ApiEntity type, or catalog validators enforcing spec.definition, will reject these entities.
Remediation: Document why spec.definition is omitted (MCP servers define APIs through remotes, not an inline definition string). Consider adding definition?: string or a sentinel value for standard ApiEntity compatibility.
🔵 Low
5. Subfolder path traversal in display URLs · repository.ts
The subfolder value is normalized (leading/trailing slashes stripped) but not validated for .. segments. A subfolder like ../../etc produces misleading browse URLs. For Azure DevOps, & in subfolder breaks the query parameter format. Risk is limited to misleading display links (no server-side path traversal).
6. Missing test for consumed paths with refused remote URLs · mapServerToEntity.test.ts
No test verifies consumed-path behavior when a remote URL is refused by D11. A test with mixed valid/refused remotes would lock in the asymmetric contract and prevent regressions.
7. Missing test for sanitizeSegment('') · identity.test.ts
No test covers direct invocation of the exported sanitizeSegment function with an empty string input. The empty-string behavior should be documented by a test.
What looks good
- URL scheme policy (D11): Correct use of WHATWG
URLconstructor with no base URL; proper scheme comparison againsthttp:andhttps:with trailing colon. - Identity derivation:
deriveMetadataNametruncation logic is verified correct — result never exceeds 63 characters, always starts/ends with alphanumeric. - Determinism: Annotation keys are sorted, consumed paths are sorted, tests verify byte-identical output for identical inputs.
- Test coverage: Comprehensive test suite covering entity shape, identity, caller defaults, required field validation, remotes (D8), descriptive metadata, repository URL combination (GitHub/GitLab/Bitbucket/Azure DevOps/fallback), determinism, and hand-off contract.
- Package structure: Consistent with sibling common-library packages — license headers, eslint config, export patterns, naming conventions,
backstage.role,sideEffects: falseall match established patterns. - Security posture: Pure function with no I/O; D11 URL allowlist is sound; no secrets, no auth, no network access.
Labels: PR implements a new feature plugin for MCP registry server mapping in the ai-integrations 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.
michael-valdron
left a comment
michael-valdron
left a comment
There was a problem hiding this comment.
/fs-fix Change PR title to have correct feat(#4813) prefix and adjust title to reflect more on the feature changes and less on the first fix commit.
michael-valdron
changed the title
mcp-registry-server-mapping
|
/fs-fix Revert the root package.json change entirely, or add the url field to the repository object. |
|
🤖 Finished Fix · ✅ Success · Started 4:07 PM UTC · Completed 4:11 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $0.57 |
Restore the repository URL that was dropped when converting the repository field from a string to an object in the root package.json. Addresses review feedback on #4823
|
/fs-fix address review comments |
|
🤖 Finished Fix · ✅ Success · Started 4:39 PM UTC · Completed 4:54 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $2.54 |
- Revert root package.json scope-creep (unrelated repository field change) - Add runtime validation for remote.type (non-empty string check) - Consume refused remote URLs symmetrically in consumedPaths - Apply encodeURIComponent per segment for Azure DevOps subfolder paths - Add tests for all three code changes Addresses review feedback on #4823 Assisted-by: claude-opus-4-6
|
🤖 Finished Review · ❌ Failure (agent timed out after 20m0s without completing (timeout: 20m0s)) · Started 9:06 PM UTC · Completed 9:28 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
|
/fs-review |
|
🤖 Finished Review · ❌ Failure (agent timed out after 20m0s without completing (timeout: 20m0s)) · Started 9:46 PM UTC · Completed 10:08 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
|
/fs-fix address review comments |
|
🤖 Finished Fix · ✅ Success · Started 10:10 PM UTC · Completed 10:32 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $6.70 |
- Extract annotation key sort to separate statement (sonarcloud S2871) - Update McpServerDocument and McpServerRepository JSDoc to match draft server.json schema descriptions with @see references - Update mapping-reference.md: add entity type section, consumed path tracking section, and runtime validation notes for remote.type - Handle potentially undefined entity.metadata.annotations in tests - Export LinksResult type and fix ae-forgotten-export API warning - Fix ae-unresolved-link in McpRegistryRemote JSDoc - Regenerate report.api.md Addresses review feedback on #4823 Assisted-by: claude-opus-4-6
|
/fs-fix Resolve the following comments:
|
|
🤖 Finished Fix · ❌ Failure (agent timed out after 25m0s without completing (timeout: 25m0s)) · Started 10:47 PM UTC · Completed 11:22 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
|
/fs-fix rather than importing |
|
🤖 Finished Fix · ✅ Success · Started 11:27 PM UTC · Completed 11:39 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.24 |
…odule Remove re-exports of McpServerApiEntity and McpServerRemote from types.ts. Each module now imports these types directly from @backstage/catalog-model/alpha. Regenerated report.api.md to reflect the updated import structure. Addresses review feedback on #4823 Assisted-by: claude-opus-4-6
|
/fs-fix Resolve the following comments:
|
|
🤖 Finished Fix · ✅ Success · Started 11:50 PM UTC · Completed 12:07 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $3.41 |
- Add openspec/changes/mcp-registry-server-mapping/design.md § DXX links to all DXX design code references in source comments across mapServerToEntity.ts, identity.ts, urlPolicy.ts, and repository.ts - Remove "Step X:" prefixes from all inline comments in mapServerToEntity.ts per code style feedback - report.api.md unchanged (comment-only edits, no API surface change) Addresses review feedback on #4823 Assisted-by: claude-opus-4-6
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 12:17 AM UTC · Completed 12:33 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $9.61 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix address review comments |
|
🤖 Fix · ❌ Terminated · Started 12:40 AM UTC · Ended 1:02 AM UTC Commit: |
- Change backstage.role from common-library to node-library to resolve
node:crypto import incompatibility (blocking finding)
- Trim URLs before storing in mapRemotes and buildLinks to prevent
untrimmed whitespace in emitted entity values
- Add actionable error message when remotes are filtered due to invalid
type fields, with a test for the type-invalid-no-websiteUrl path
- Remove re-exports of McpServerApiEntity and McpServerRemote from
index.ts; consumers should import directly from
@backstage/catalog-model/alpha
- Re-strip trailing / after .git removal in normalizeBase to prevent
double-slash edge case
- Add JSDoc to LinksResult and McpRegistryRemote properties
- Add co-located repository.test.ts with comprehensive coverage
- Add tests for sanitizeSegment('') and empty title edge cases
- Update report.api.md to reflect API surface changes
Addresses review feedback on #4823
Assisted-by: claude-opus-4-6
@michael-valdron the fs-fix prompt needs to be on a single line |
|
🤖 Finished Fix · ✅ Success · Started 12:40 AM UTC · Completed 1:02 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $6.13 |
|
|
/fs-review |
|
🤖 Review · ❌ Terminated · Started 1:11 AM UTC · Ended 1:30 AM UTC Commit: |
@gabemontero I was beginning to notice this as I was iterating over the fixes, espially when I broke up the one that timed out, thanks for clarify! I'll make sure my upcoming |
| return undefined; | ||
| } | ||
|
|
||
| const originalUrl = repository.url; |
There was a problem hiding this comment.
[medium] logic-error
computeRepositoryUrl does not trim whitespace from repository.url before using it, unlike every other URL emit path in the codebase (remote.url.trim() in mapRemotes, doc.websiteUrl.trim() in buildLinks). If repository.url has surrounding whitespace, it would pass isAllowedUrl() (which trims internally) but produce untrimmed URLs in combinedUrl, originalUrl, the Source Code link, backstage.io/source-location, and the repository.url annotation. The D11 spec states 'Trim leading/trailing whitespace' as step 1.
Suggested fix: Add .trim() to the assignment of originalUrl in computeRepositoryUrl: const originalUrl = repository.url.trim();
| */ | ||
| export function computeRepositoryUrl( | ||
| repository: McpServerRepository, | ||
| ): RepositoryUrlResult | undefined { |
There was a problem hiding this comment.
[medium] path-traversal
normalizeSubfolder strips leading/trailing slashes and whitespace but does not reject '..' path traversal segments. A subfolder value containing '..' segments is concatenated into the combined URL for all SCM types, written into metadata.links[].url and backstage.io/source-location. The traversal is limited to the same host but could redirect the Source Code link to an unintended repository path. types.ts states 'Must be a clean relative path' but this invariant is not enforced.
Suggested fix: Add validation to reject or strip '..' path segments (e.g., if (segments.some(s => s === '..')) return undefined;)
|
|
||
| return base; | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] edge-case
normalizeBase operates on the raw URL string rather than parsed URL components. The .git suffix stripping and trailing-slash removal would not work correctly if the repository URL contained query parameters or fragment identifiers. While uncommon for repository.url, the string-based approach could produce incorrect combined URLs in such edge cases.
|
🤖 Finished Review · ✅ Success · Started 1:11 AM UTC · Completed 1:30 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $5.57 |
2 participants
Add the generated report.api.md for the new mcp-registry-server-mapping package and fix missing @public release tags on DEFAULT_PREFIX and RepositoryUrlResult. Escape @ in TSDoc comment to resolve api-extractor warnings.
Assisted-by: claude-opus-4-6
Co-Authored-By: Claude Opus 4.6 [email protected]
Closes #4813
Post-script verification
agent/4813-mcp-registry-server-mapping)5d966c40591070c960f164a16412193061b48104..HEAD)