Skip to content

Conversation

jkiddo
Copy link
Collaborator

@jkiddo jkiddo commented Aug 10, 2025

No description provided.

@jkiddo
Copy link
Collaborator Author

jkiddo commented Aug 23, 2025

Currently awaiting next release of https://github.com/spring-projects/spring-ai

@robogary
Copy link

Formatting check succeeded!

@jkiddo jkiddo requested a review from Copilot August 24, 2025 15:57
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements MCP (Model Context Protocol) integration to provide a bridge between AI models and the FHIR server. The feature adds an MCP server that exposes FHIR operations and CDS Hooks as tools that can be called by AI systems.

Key changes:

  • Added MCP server configuration and bridge implementation
  • Created tool specifications for FHIR CRUD operations and CDS Hooks calls
  • Enabled CR (Clinical Reasoning) and CDS Hooks modules to support MCP functionality

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
pom.xml Added Spring AI MCP and Model Context Protocol SDK dependencies
src/main/resources/application.yaml Added MCP server configuration and enabled CR/CDS Hooks modules
src/main/java/ca/uhn/fhir/jpa/starter/AppProperties.java Fixed annotation ordering for proper configuration
src/main/java/ca/uhn/fhir/rest/server/MCPBridge.java Main bridge class connecting MCP tools to FHIR operations
src/main/java/ca/uhn/fhir/jpa/starter/mcp/*.java MCP-specific classes for tool definitions, request building, and result handling
src/test/resources/mcp/*.xml, *.json Test resources for MCP functionality
src/test/java/ca/uhn/fhir/jpa/starter/McpTests.java Test class for MCP client integration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 170 to 172
// Print the keys of contextMap
for (String key : contextMap.keySet()) {
System.out.println("Context map key: " + key);
Copy link
Preview

Copilot AI Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug code with System.out.println statements (lines 170-173) should be removed from production code. Use proper logging instead.

Suggested change
// Print the keys of contextMap
for (String key : contextMap.keySet()) {
System.out.println("Context map key: " + key);
logger.debug("Context map key: {}", key);

Copilot uses AI. Check for mistakes.

Comment on lines +89 to +91
Object patchBody = config.get("patch");
if (patchBody == null) {
throw new IllegalArgumentException("Missing 'patch' for patch interaction");
Copy link
Preview

Copilot AI Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RequestBuilder looks for a 'patch' parameter but the ToolFactory schemas define the parameter as 'resource' for patch operations. This will cause patch operations to fail.

Suggested change
Object patchBody = config.get("patch");
if (patchBody == null) {
throw new IllegalArgumentException("Missing 'patch' for patch interaction");
Object patchBody = config.get("resource");
if (patchBody == null) {
throw new IllegalArgumentException("Missing 'resource' for patch interaction");

Copilot uses AI. Check for mistakes.

Copilot

This comment was marked as outdated.

@jkiddo jkiddo requested a review from Copilot August 24, 2025 18:34
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements Model Context Protocol (MCP) integration to enable AI agents to interact with FHIR resources and CDS Hooks through standardized tooling. The implementation provides a bridge between MCP clients and the HAPI FHIR server, exposing FHIR operations as MCP tools.

  • Adds MCP server configuration and dependency management for Spring AI MCP integration
  • Implements FHIR resource manipulation tools (CRUD operations, search, transactions) accessible via MCP
  • Enables CDS Hooks invocation through MCP tooling with proper request/response handling

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pom.xml Adds Spring AI MCP and MCP SDK dependencies for protocol support
src/main/resources/application.yaml Configures MCP server settings and enables CR/CDS Hooks modules
src/main/java/ca/uhn/fhir/rest/server/MCPBridge.java Core bridge component that translates MCP tool calls to FHIR operations
src/main/java/ca/uhn/fhir/jpa/starter/mcp/ MCP-specific classes for tool definitions, request building, and result handling
src/test/ Test resources and integration tests for MCP functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 170 to 172
// Print the keys of contextMap
for (String key : contextMap.keySet()) {
System.out.println("Context map key: " + key);
Copy link
Preview

Copilot AI Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug print statements should be removed from production code. Use proper logging instead of System.out.println for debugging purposes.

Suggested change
// Print the keys of contextMap
for (String key : contextMap.keySet()) {
System.out.println("Context map key: " + key);
logger.debug("Context map key: {}", key);

Copilot uses AI. Check for mistakes.

Comment on lines 162 to 164
// TODO Return MCP Error
logger.error(e.getMessage(), e);
e.printStackTrace();
Copy link
Preview

Copilot AI Aug 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the printStackTrace() call as the exception is already being logged via the logger. The printStackTrace() is redundant and can clutter output.

Suggested change
// TODO Return MCP Error
logger.error(e.getMessage(), e);
e.printStackTrace();

Copilot uses AI. Check for mistakes.

Copilot

This comment was marked as outdated.

@jkiddo jkiddo requested a review from Copilot August 24, 2025 19:23
@robogary
Copy link

Formatting check succeeded!

Copilot

This comment was marked as outdated.

@robogary
Copy link

Formatting check succeeded!

@jkiddo
Copy link
Collaborator Author

jkiddo commented Aug 25, 2025

@robogary
Copy link

Formatting check succeeded!

@robogary
Copy link

Formatting check succeeded!

@robogary
Copy link

Formatting check succeeded!

@jkiddo jkiddo requested a review from Copilot August 26, 2025 21:32
Copilot

This comment was marked as outdated.

@jkiddo jkiddo requested a review from Copilot August 26, 2025 22:28
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements Model Context Protocol (MCP) server functionality to the HAPI FHIR starter project, enabling LLM clients to interact with FHIR resources through standardized MCP tools.

  • Adds MCP server configuration and bridge components for FHIR operations and CDS Hooks
  • Implements comprehensive FHIR resource tools (CRUD operations, search, transactions)
  • Enables CDS Hooks integration through MCP protocol

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
pom.xml Adds Spring AI MCP and MCP SDK dependencies
application.yaml Configures MCP server settings and enables CR/CDS Hooks modules
McpServerConfig.java Spring configuration for MCP server with servlet registration
McpFhirBridge.java Bridge component connecting MCP tools to FHIR operations
McpCdsBridge.java Bridge component for CDS Hooks integration via MCP
ToolFactory.java Factory creating MCP tool definitions for FHIR and CDS operations
RequestBuilder.java Builds HTTP requests for different FHIR interactions
Interaction.java Enum mapping MCP interactions to HTTP request types
CallToolResultFactory.java Factory for creating MCP tool call responses
McpBridge.java Interface for MCP bridge implementations
AppProperties.java Moves annotation to fix configuration loading order
.dockerignore Removes obsolete duplicate-finder-result.xml entry
Test resources JSON/XML test files for MCP CDS Hooks integration testing
McpTests.java Disabled integration test for MCP client functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

case SEARCH -> {
method = "GET";
req = new MockHttpServletRequest(method, basePath);
if (config.get("searchParams") instanceof Map<?, ?> sp) {
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code checks for 'searchParams' but the SEARCH_FHIR_RESOURCES_SCHEMA defines the parameter as 'query'. This mismatch will cause search parameters to never be applied.

Suggested change
if (config.get("searchParams") instanceof Map<?, ?> sp) {
if (config.get("query") instanceof Map<?, ?> sp) {

Copilot uses AI. Check for mistakes.

Comment on lines +89 to +91
Object patchBody = config.get("patch");
if (patchBody == null) {
throw new IllegalArgumentException("Missing 'patch' for patch interaction");
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks for 'patch' key but the PATCH_FHIR_RESOURCE_SCHEMA and CONDITIONAL_PATCH_FHIR_RESOURCE_SCHEMA define the parameter as 'resource'. This will cause patch operations to fail.

Suggested change
Object patchBody = config.get("patch");
if (patchBody == null) {
throw new IllegalArgumentException("Missing 'patch' for patch interaction");
Object patchBody = config.get("resource");
if (patchBody == null) {
throw new IllegalArgumentException("Missing 'resource' for patch interaction");

Copilot uses AI. Check for mistakes.

"description": "Additional data to prefetch for the CDS service call."
}
},
"required": ["service", "hook", "hookInstance", "context"]
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The required field lists 'context' but the property is actually named 'hookContext' in the schema above. This will cause validation failures for CDS Hook calls.

Suggested change
"required": ["service", "hook", "hookInstance", "context"]
"required": ["service", "hook", "hookInstance", "hookContext"]

Copilot uses AI. Check for mistakes.

Comment on lines +100 to +109
var hookContext = (Map<String, String>) contextMap.get("hookContext");
if (hookContext.containsKey("userId")) {
context.put("userId", hookContext.get("userId").toString());
}
if (hookContext.containsKey("patientId")) {
context.put("patientId", hookContext.get("patientId").toString());
}
if (hookContext.containsKey("encounterId")) {
context.put("encounterId", hookContext.get("encounterId").toString());
}
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsafe cast to Map<String, String> without type checking. If hookContext contains non-String values, this will throw a ClassCastException at runtime.

Suggested change
var hookContext = (Map<String, String>) contextMap.get("hookContext");
if (hookContext.containsKey("userId")) {
context.put("userId", hookContext.get("userId").toString());
}
if (hookContext.containsKey("patientId")) {
context.put("patientId", hookContext.get("patientId").toString());
}
if (hookContext.containsKey("encounterId")) {
context.put("encounterId", hookContext.get("encounterId").toString());
}
var hookContextObj = contextMap.get("hookContext");
if (hookContextObj instanceof Map<?, ?> hookContext) {
if (hookContext.containsKey("userId")) {
var userId = hookContext.get("userId");
if (userId != null) {
context.put("userId", userId.toString());
}
}
if (hookContext.containsKey("patientId")) {
var patientId = hookContext.get("patientId");
if (patientId != null) {
context.put("patientId", patientId.toString());
}
}
if (hookContext.containsKey("encounterId")) {
var encounterId = hookContext.get("encounterId");
if (encounterId != null) {
context.put("encounterId", encounterId.toString());
}
}
}

Copilot uses AI. Check for mistakes.

Comment on lines 83 to 85
// TODO Return MCP Error
logger.error(e.getMessage(), e);
e.printStackTrace();
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The printStackTrace() call is redundant since the exception is already being logged. Remove this line to avoid duplicate stack traces in logs.

Suggested change
// TODO Return MCP Error
logger.error(e.getMessage(), e);
e.printStackTrace();

Copilot uses AI. Check for mistakes.

Comment on lines +43 to +44
if (config.get("searchParams") instanceof Map<?, ?> sp) {
sp.forEach((k, v) -> req.addParameter(k.toString(), v.toString()));
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code should handle the case where the 'query' parameter (as defined in the schema) needs to be parsed into individual search parameters. Currently it expects a Map but the schema defines it as a comma-separated string.

Suggested change
if (config.get("searchParams") instanceof Map<?, ?> sp) {
sp.forEach((k, v) -> req.addParameter(k.toString(), v.toString()));
Object searchParamsObj = config.get("searchParams");
if (searchParamsObj instanceof Map<?, ?> sp) {
sp.forEach((k, v) -> req.addParameter(k.toString(), v.toString()));
} else if (searchParamsObj instanceof String spStr && !spStr.isBlank()) {
// Parse comma-separated string, e.g. "name=John,age=30"
String[] pairs = spStr.split(",");
for (String pair : pairs) {
String[] kv = pair.split("=", 2);
if (kv.length == 2) {
req.addParameter(kv[0].trim(), kv[1].trim());
}
}

Copilot uses AI. Check for mistakes.

@robogary
Copy link

Formatting check succeeded!

@robogary
Copy link

Formatting check succeeded!

@robogary
Copy link

Formatting check succeeded!

@robogary
Copy link

Formatting check succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants