feat(#4795): implement MCP registry annotation projection - #4859
Conversation
Add annotation projection for the MCP registry to Backstage catalog mapping. This is the second half of the server.json transform (the first was direct field mapping in #4813). The projection walks every scalar leaf in server.json that is not consumed by direct mapping and emits it as a modelcontextprotocol.io/<dot.path> annotation with: - D3 key sanitization: per-segment lowercase/illegal-char replacement, boundary normalization, 63-char truncation with FNV-1a hash suffix, and collision disambiguation - D9 secret redaction: prunes default/value/choices from isSecret inputs - D11 URL gating: refuses non-http/https scheme URLs from projection - D12 null/empty omission: null omit, empty arrays/objects omit, but false/0/empty-string project as strings Exports projectAnnotations from the common library package. Includes 65 tests covering all design decisions. 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 5:38 PM UTC · Completed 5:59 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.26 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4859 +/- ##
==========================================
+ Coverage 63.59% 63.69% +0.09%
==========================================
Files 2685 2687 +2
Lines 107076 107495 +419
Branches 29974 30083 +109
==========================================
+ Hits 68099 68468 +369
- Misses 37166 37216 +50
Partials 1811 1811
*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) DetailsTier 1 signals are unchanged from the prior assessment (line count grew from 3155 to 4329 but remains in the ≥2000 bucket, all other signals identical); Tier 2 confirms zero regression history on brand-new files; Tier 3 confirms detailed acceptance criteria and safe rollback posture — composite of ~1.55 rounds to 2 and re-review anchoring preserves the prior moderate score. Previous runRisk Assessment: moderate (2/5) DetailsLarge volume of new code (3155 lines) with complex annotation-projection logic in a shared library warrants standard review, but bot authorship, zero regression history, no CI or dependency changes, well-specified acceptance criteria, and no security-sensitive surface keep the risk at moderate. Previous run (2)Risk Assessment: moderate (2/5) DetailsLarge volume of new code (2574 lines) with complex domain logic in a shared library warrants standard review, but bot authorship, zero regression history, no CI or dependency changes, and adequate test coverage keep the risk at moderate. Previous run (3)Risk Assessment: moderate (2/5) DetailsTier 1 signals are unchanged from the prior assessment; large raw volume (2411 lines, large blast radius) is the sole upward driver but the bulk is new test code for a purely additive, well-scoped feature with no security, dependency, CI, or protected-path exposure, preserving the prior moderate score of 2. Previous run (4)Risk Assessment: moderate (2/5) DetailsAlthough raw change volume is large (2411 lines, large blast radius), the bulk is comprehensive new tests (1847 lines) for a well-scoped additive feature with no security, dependency, CI, or protected-path signals, yielding a moderate composite score. Previous run (5)Risk Assessment: low (1/5) DetailsPrior score of 1 is preserved: signals are unchanged - bot-authored, additive-only feature on newly created files, zero security or CI concerns, comprehensive test coverage (1,585 test lines vs 351 implementation lines), and fully triaged acceptance criteria; the large blast-radius and high line-count are artifacts of the test-heavy addition, not of modifications to existing shared code. Previous run (6)Risk Assessment: low (1/5) DetailsBot-authored PR adding a well-scoped new feature (annotation projection for MCP registry mapping, part 2 of 2) with comprehensive test coverage (1,875 test lines for a 47-line implementation), no security or CI concerns, clean git history on newly created files, and explicitly defined acceptance criteria in the linked issue; the large blast-radius metric is an artifact of the test-heavy change, not of modifications to existing shared code. |
ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewReason: stale-head The review agent reviewed commit Previous run (5)ReviewFindingsMedium
Low
Next steps:
Previous run (6)ReviewFindingsMedium
Low
Next steps:
|
![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.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 6:02 PM UTC · Completed 6:19 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.64 |
- Deduplicate fnv1a32 and normalizeBoundaries from annotationProjection.ts by exporting them as @internal from identity.ts and importing - Fix stale spec scenario: camelCase mimeType to lowercase mimetype - Add export-for-testing-only comments on @internal helpers - Extend consumedPaths/reservedAnnotationKeys JSDoc with format spec - Add hash-collision guard in disambiguation to prevent silent data loss Addresses #4859 Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 6 review findings: deduplicated FNV-1a hash and normalizeBoundaries into identity.ts (medium), fixed stale camelCase mimeType in spec scenario (medium), added export-for-testing intent comments on @internal helpers (low, two related findings), extended consumedPaths JSDoc with format specification (low), and added hash-collision guard in disambiguation to prevent silent data loss (low) Fixed (6):
Tests: passed Decision points
Next steps:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:28 PM UTC · Completed 6:50 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.14 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 6:53 PM UTC · Completed 7:11 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.62 |
- Export MAX_NAME_LENGTH from identity.ts and import in annotationProjection.ts to eliminate duplication - Fix @internal JSDoc tags to use standalone format matching identity.ts - Fix logic gap: disambiguation now applies regardless of truncation hash state, preventing potential reserved-key conflicts - Add @remarks documenting lowercase normalization of annotation keys - Add sync-dependency comment on SECRET_REDACTED_FIELDS - Remove horizontal-dash section dividers not used in other files - Update audit.md timestamp to reflect spec.md changes - Mark tasks 3.1-3.6, 4.3, 4.4, 4.6, 4.7 as completed in tasks.md Addresses #4859 Assisted-by: claude-code
🔧 Fix agent — iteration 2 (human-triggered)Addressed all 9 review findings: fixed 8 items (stale audit timestamp, JSDoc format, disambiguation logic gap, secret-redaction sync comment, MAX_NAME_LENGTH deduplication, section divider style, task completion status, and lowercase normalization documentation) and recorded 1 reasoned disagreement (task 4.5 docs are out of scope for this PR). All 205 tests pass, TypeScript compilation clean, ESLint and Prettier pass. Fixed (8):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
Callers can pass placeholderRemoteUrl to mapRemotes and mapServerToEntity defaults; it is D11-checked like websiteUrl. Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 1:24 AM UTC · Completed 1:49 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.30 |
Signed-off-by: Michael Valdron <mvaldron@redhat.com>
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.
Replace the open index signature with schema-aligned ServerDetail types and expand assertServerJsonSchema to reject unknown or mistyped fields. Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…atures Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…anch state Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
… top Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Split assertServerJsonSchema to reduce cognitive complexity and cover the extracted helpers with unit tests. Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:43 AM UTC · Completed 3:12 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $11.77 |
|
/fs-fix Address the following: Conditionally include placeholderRemoteUrl in the error message only when the parameter was supplied (#4859 (comment)); Remove the 'Exported for unit testing only.' sentence from the JSDoc, keep |
|
🤖 Finished Fix · ✅ Success · Started 3:24 AM UTC · Completed 3:41 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.78 |
Conditionally include placeholderRemoteUrl in mapRemotes error message only when the parameter was supplied. Remove misleading 'Exported for unit testing only.' from parseAbsoluteUrl JSDoc since it is used in production code by annotationProjection.ts. Addresses #4859 Assisted-by: claude-code/claude-sonnet-4-20250514
🔧 Fix agent — iteration 4 (human-triggered)Addressed both review findings: (1) made the mapRemotes error message conditionally reference placeholderRemoteUrl only when the parameter was supplied, and (2) removed the misleading 'Exported for unit testing only.' sentence from parseAbsoluteUrl JSDoc in urlPolicy.ts since the function is used in production code by annotationProjection.ts. Fixed (2):
Tests: passedNext steps:
|
michael-valdron
left a comment
michael-valdron
left a comment
There was a problem hiding this comment.
Current changes lgtm.
@gabemontero @johnmcollier PTAL when you can.
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
I like these refs to the spec via the @ see @ link usage .... on small suggestion - maybe the README.md or a new markdown could somehow contain a cross reference table to all these different locations (i.e. links to the line of code where you have the use of see / link) so we have a centralized view of all the aspects of the mcp registry spec
otherwise seems straight forward; pending any feedback from @johnmcollier go ahead and get this merged @michael-valdron and move onto the next step
Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Point package docs, JSDoc, and tests at the tagged draft schema instead of main so the mapping contract tracks a stable registry revision. Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add field-level type tables for the MCP Registry v1.8.1 server.json entities and link them from the package README. Assisted-by: grok-4.6 Signed-off-by: Michael Valdron <mvaldron@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@gabemontero Addressed your feedback, I added a separate document highlighting the breakdowns of the server.json types with links to the source locations with an inclusion reference to it in the README (forgot to push that commit) ce3fd2d. Just need @johnmcollier's complete review, any feedback addressed there and we're good to go I think. |
3 participants
Summary
Implement annotation projection for the MCP registry → Backstage catalog mapping (part 2 of 2; part 1 was direct field mapping in #4813).
The projection walks every scalar leaf in
server.jsonthat is not consumed by direct mapping and emits it as amodelcontextprotocol.io/<dot.path>annotation with:default/value/choicesfromisSecretinputsnull→omit, empty[]/{}→omit, butfalse/0/""→project as stringsExports
projectAnnotationsfrom the common library package.Test plan
Closes #4795
Post-script verification
agent/4795-annotation-projection)e760451223d9ae9997cc1cab890e710194954e1e..HEAD)