Skip to content

Conversation

@ChrisEdwards
Copy link
Collaborator

@ChrisEdwards ChrisEdwards commented Nov 12, 2025

Rebased onto main after PR #25 (AIML-224) merged.

Summary

This PR consolidates duplicate MCP tool variants that accept either application name or application ID, reducing tool bloat and optimizing AI agent context window usage. Currently, the MCP server exposes two versions of each application-level operation—one accepting app_name and one accepting app_id—which unnecessarily consumes valuable context space.

Problem Statement

Why This Change Is Needed

The MCP server currently exposes duplicate tools for every application-scoped operation:

Example duplicates:

  • get_vulnerability(app_name, vulnID) AND get_vulnerability_by_id(appID, vulnID)
  • list_vulnerabilities(app_name) AND list_vulnerabilities_with_id(appID)
  • get_ADR_Protect_Rules(app_name) AND get_ADR_Protect_Rules_by_app_id(appID)
  • list_application_libraries(app_name) AND list_application_libraries_by_app_id(appID)

Issues with this approach:

  1. Context window waste: Each tool definition consumes precious AI context space
  2. Tool list bloat: Claude sees 8 tools when only 4 are functionally necessary
  3. Cognitive overhead: More tools means more decisions for the AI to make
  4. Maintenance burden: Any changes require updating two nearly-identical tools

Why app_id-only is sufficient:

  • AI agents are smart enough to call list_applications_with_name first to resolve names to IDs
  • This creates a consistent, predictable workflow pattern
  • Reduces ambiguity (no need to decide which variant to use)

Solution Design

Approach: Keep app_id variants, remove app_name variants

Rationale for choosing app_id over app_name:

  1. Precision: App IDs are unique identifiers; names can be ambiguous or duplicated
  2. Consistency: Aligns with Contrast API patterns which prefer IDs
  3. Existing discovery tool: We already have list_applications_with_name for name→ID resolution
  4. Natural workflow: Discover → Select → Operate is a clear pattern

Changes Made

1. Service Layer Consolidation

Removed duplicate methods:

  • SCAService.getApplicationLibraries(String app_name)
  • ADRService.getProtectData(String applicationName)
  • AssessService.getVulnerability(String vulnID, String app_name)
  • AssessService.listVulnsInAppByName(String app_name)

Renamed remaining methods (MCP tool names only, kept method names for compatibility):

  • list_application_libraries_by_app_idlist_application_libraries
  • get_ADR_Protect_Rules_by_app_idget_ADR_Protect_Rules
  • get_vulnerability_by_idget_vulnerability
  • list_vulnerabilities_with_idlist_vulnerabilities

Enhanced tool descriptions:
All consolidated tools now include guidance: "Use list_applications_with_name first to get the application ID from a name"

2. Code Quality Improvements

Added input validation:

// SCAService.getApplicationLibrariesByID()
if (appID == null || appID.isEmpty()) {
    throw new IllegalArgumentException("Application ID cannot be null or empty");
}

Why this matters: Previously, invalid input would fail deep in the SDK with unclear errors. Now we fail fast with clear, actionable error messages.

3. Comprehensive Test Coverage

New test files (1,200+ lines of tests):

  • SCAServiceTest.java - Unit tests for library operations
  • SCAServiceIntegrationTest.java - Live API tests with test data discovery
  • ADRServiceIntegrationTest.java - Live API tests for Protect rules

Test improvements:

  • Added @MockitoSettings(strictness = Strictness.LENIENT) to handle complex mock scenarios
  • Implemented test data discovery pattern (finds suitable test data automatically)
  • Fixed mock construction issues in CVE testing
  • Fixed integration test expectations for error cases

Test results:

  • ✅ All 250 tests passing (unit + integration)
  • mvn verify successful

4. Documentation Updates

Updated test plan files:
Added AIML-189 consolidation notes to 4 remaining test plans explaining:

  • Which duplicate was removed
  • Why the tool was renamed
  • How to use the consolidated version

Deleted obsolete documentation:
Removed 4 test plan files for deleted app_name variants:

  • test-plan-get_ADR_Protect_Rules.md
  • test-plan-get_vulnerability.md
  • test-plan-list_application_libraries.md
  • test-plan-list_vulnerabilities.md

Impact Analysis

For AI Agents

Before:

🤖 Claude sees 8 tools for app-level operations
→ Must choose between app_name vs app_id variants
→ More context consumed, more cognitive load

After:

🤖 Claude sees 4 tools for app-level operations
→ Clear workflow: list_applications_with_name → get app_id → use tool
→ Less context consumed, simpler decision tree

For Users

No breaking changes for well-designed agents:

  • Agents already calling list_applications_with_name will work unchanged
  • Tool behavior is identical, just accepting app_id instead of app_name

Workflow example:

1. Agent calls: list_applications_with_name("cargo-cats")
2. Gets back: [{ id: "abc-123", name: "cargo-cats-frontgate", ... }]
3. Agent calls: get_vulnerability(appID="abc-123", vulnID="xyz-789")

Backward Compatibility

This should be fully backward compatible. Agents dynamically discover the tools to call and will automatically adapt to getting the app id and calling the app id tools.

Testing Strategy

Unit Tests

  • Mock SDK responses for all success/error paths
  • Validate input parameter checking
  • Verify correct SDK method calls

Integration Tests

  • Live API calls against real Contrast instance
  • Automatic test data discovery (finds apps with data to test)
  • Validates end-to-end workflows
  • Error handling for invalid inputs

Manual Testing

Run these commands to verify:

mvn test          # All unit tests
mvn verify        # Unit + integration tests

Future Improvements

This consolidation establishes a pattern for future tools:

  • ✅ Always prefer IDs over names for entity operations
  • ✅ Provide discovery tools (like list_applications_with_name)
  • ✅ Keep tool descriptions explicit about the workflow
  • ✅ Maintain comprehensive test coverage

Related Issues

  • AIML-189: Remove duplicate tools in MCP Server (app name vs app id)

Testing checklist:

  • ✅ All unit tests pass (250/250)
  • ✅ All integration tests pass
  • ✅ Manual verification of consolidation
  • ✅ Documentation updated

Review focus areas:

  • Verify removal of duplicate tools doesn't break any internal use cases
  • Confirm test coverage is sufficient
  • Check if tool descriptions are clear enough for AI agents

@ChrisEdwards ChrisEdwards changed the base branch from main to AIML-224-consolidate-route-coverage November 12, 2025 05:06
@ChrisEdwards ChrisEdwards force-pushed the AIML-189-remove-duplicate-tools branch from f6acd0f to 28729a4 Compare November 12, 2025 05:08
@ChrisEdwards ChrisEdwards force-pushed the AIML-224-consolidate-route-coverage branch from 22264b6 to d9149ea Compare November 12, 2025 21:54
@ChrisEdwards ChrisEdwards requested a review from Copilot November 13, 2025 05:50
Copy link
Contributor

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 consolidates duplicate MCP tool variants to optimize AI context usage. The core change removes tools that accept application names in favor of those accepting application IDs, establishing a consistent workflow where agents first resolve names to IDs via list_applications_with_name, then use ID-based tools for operations.

Key changes:

  • Removes 4 duplicate app_name tool variants (SCA, ADR, Assess services)
  • Renames remaining app_id tools to simplified names (removes _by_app_id suffix)
  • Adds comprehensive test coverage (1,200+ lines across unit and integration tests)

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
SCAService.java Removes getApplicationLibraries(app_name), renames tool to list_application_libraries, adds input validation
AssessService.java Removes getVulnerability(app_name) and listVulnsInAppByName(app_name), renames remaining tools
ADRService.java Removes getProtectData(app_name), renames tool to get_ADR_Protect_Rules, adds input validation
RouteCoverageService.java Consolidates 6 route coverage methods into single parameterized method
SCAServiceTest.java New comprehensive unit tests for library operations
SCAServiceIntegrationTest.java New integration tests with automatic test data discovery
ADRServiceTest.java Adds unit tests for getProtectDataByAppID
ADRServiceIntegrationTest.java New integration tests for Protect/ADR rules
RouteCoverageServiceTest.java New comprehensive unit tests for consolidated route coverage method
RouteCoverageServiceIntegrationTest.java New integration tests for route coverage with filter validation
CLAUDE.md Adds bead status management guidelines

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Changes Made

### Tool Consolidation
- Removed 4 duplicate app_name variant tools:
  - `SCAService.list_application_libraries` (app_name)
  - `ADRService.get_ADR_Protect_Rules` (app_name)
  - `AssessService.get_vulnerability` (app_name)
  - `AssessService.list_vulnerabilities` (app_name)

### Tool Renaming
- Renamed remaining app_id tools to remove suffix:
  - `list_application_libraries_by_app_id` → `list_application_libraries`
  - `get_ADR_Protect_Rules_by_app_id` → `get_ADR_Protect_Rules`
  - `get_vulnerability_by_id` → `get_vulnerability`
  - `list_vulnerabilities_with_id` → `list_vulnerabilities`

- Updated tool descriptions to mention using `list_applications_with_name`
  first to get application ID from name

### Code Improvements
- Added input validation to `SCAService.getApplicationLibrariesByID()`
  for null/empty appID parameter

### Test Enhancements
- Added comprehensive unit tests for SCA service methods
- Added integration tests for ADR and SCA services with test data discovery
- Fixed Mockito strictness issues with lenient settings
- Fixed integration test for invalid CVE handling

### Documentation
- Updated 4 test plan files with AIML-189 consolidation notes
- Deleted 4 obsolete test plan files for removed app_name variants

## Test Results
- All 248 unit and integration tests passing
- mvn verify: SUCCESS

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@ChrisEdwards ChrisEdwards force-pushed the AIML-189-remove-duplicate-tools branch from 28729a4 to 4d8aacd Compare November 13, 2025 17:02
@ChrisEdwards ChrisEdwards changed the base branch from AIML-224-consolidate-route-coverage to main November 13, 2025 17:02
@ChrisEdwards ChrisEdwards marked this pull request as ready for review November 13, 2025 17:34
@Tool(name = "list_application_libraries_by_app_id", description = "Takes a application ID and returns the libraries used in the application, note if class usage count is 0 the library is unlikely to be used")
@Tool(name = "list_application_libraries", description = "Takes an application ID and returns the libraries used in the application. Use list_applications_with_name first to get the application ID from a name. Note: if class usage count is 0 the library is unlikely to be used")
public List<LibraryExtended> getApplicationLibrariesByID(String appID) throws IOException {
if (appID == null || appID.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

for future downstream changes....
Use StringUtils.isNotBlank() or the Spring StringUtils library...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its on my list. Sounds like a helpful library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also for your other tech debt cleanup tickets, lets replace to @Service classes that are using it's own constructor we can replace with @RequiredArgsConstructor
example:

@Service
@RequiredArgsConstructor
public class ADRService {

    private static final Logger logger = LoggerFactory.getLogger(ADRService.class);

    private final PaginationHandler paginationHandler;

    @Value("${contrast.host-name:${CONTRAST_HOST_NAME:}}")
    private String hostName;

    @Value("${contrast.api-key:${CONTRAST_API_KEY:}}")
    private String apiKey;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added to the list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a story to use lombok everywhere. I only used it in a few places for that first pr. We need to adopt it full on now.

@ChrisEdwards ChrisEdwards force-pushed the AIML-189-remove-duplicate-tools branch from 9f1fe96 to 4d8aacd Compare November 13, 2025 18:50
@jason-at-contrast
Copy link
Collaborator

jason-at-contrast commented Nov 13, 2025

So I'm having an issue with this branch.. new build, new deploy and i created a new chat so it doesn't have any context in it. then I prompt it:
list all applications in contrast security

it returns this:
The application list returned empty. This could mean there are no applications currently accessible or the wildcard search isn't returning results. Would you like me to try searching for a specific application name instead? For example, if you're looking for "dvapi" or another application, I can search for that specific name.

and I opened the readme.html and reran the list all applications in contrast security and i got this:

I don't have access to a tool that lists all applications in Contrast Security at the moment. The tool that would provide this functionality (list_applications_with_name) is currently disabled. If you know the name of a specific application you'd like to check, I can search for vulnerabilities or other information about it. Just let me know the application name.

But I know it's not a configuration issue because same chat:
what are the application id's that are in contrast security

comes back with:

Here are the applications in Contrast Security:

Application IDs:

Welcome to Tomcat - c6ad707e-fd5e-4cd3-934b-cb75b30db662 (Java, offline)
Web-Application - c15b8762-0574-4983-b18e-4b7f3aa76403 (Java, offline)
TestRequiredField - e87cc898-6f6b-42fb-9934-c01453be999f (Java, offline)
test-project - 8ec5a3f5-b659-4c15-a9db-43e3cd9049c8 (Java, offline)
ROOT - 6ae2561b-b978-4a5e-9427-b528a350519a (Java, offline)
petclinic-linux-container - b7eee43a-4c46-4dd5-bb01-c0c697c3e1b3 (Java, offline)
flask-app - c72a982c-041b-468c-b17a-77d884cb49c5 (Python, offline)
Email-Service - 09498e3e-d20b-4203-926d-25503a5f3cc7 (Java, offline)
dvapi - ba0184e9-4ae7-44c9-8482-4c6f6df0ff35 (Node, online)
DotnetDemo - 14be5c36-0c9f-4f73-ab00-8d37114eac50 (.NET Core, offline)
DemoApplication - 87d06873-711c-42bb-a8e5-07923f8b245d (.NET Core, offline)
cSharp-demo - 443f5d89-6265-495f-950f-9eab221ddf56 (.NET Core, offline)
contrast-node-demo - f3a1fcfd-ad2d-4f25-b2ea-1a27e175d12f (Node, offline)
contrast-demo - ffb0cac8-389f-4f1a-9240-c770e492c66e (Java, offline)
Note: dvapi is currently the only application with "online" status.`

@ChrisEdwards
Copy link
Collaborator Author

ChrisEdwards commented Nov 13, 2025

Jason, what are you using to load the application list? There is no pagination, and most tools crash because it's too much data. That is something I am fixing this week.
Copilot works. Otherwise try listing all vulnerabilities. That has pagination
I won't make another release until the list all applications tool has pagination

@jason-at-contrast
Copy link
Collaborator

So I'm having an issue with this branch.. new build, new deploy and i created a new chat so it doesn't have any context in it. then I prompt it: list all applications in contrast security

it returns this: The application list returned empty. This could mean there are no applications currently accessible or the wildcard search isn't returning results. Would you like me to try searching for a specific application name instead? For example, if you're looking for "dvapi" or another application, I can search for that specific name.

and I opened the readme.html and reran the list all applications in contrast security and i got this:

I don't have access to a tool that lists all applications in Contrast Security at the moment. The tool that would provide this functionality (list_applications_with_name) is currently disabled. If you know the name of a specific application you'd like to check, I can search for vulnerabilities or other information about it. Just let me know the application name.

But I know it's not a configuration issue because same chat: what are the application id's that are in contrast security

comes back with:

Here are the applications in Contrast Security:

Application IDs:

Welcome to Tomcat - c6ad707e-fd5e-4cd3-934b-cb75b30db662 (Java, offline) Web-Application - c15b8762-0574-4983-b18e-4b7f3aa76403 (Java, offline) TestRequiredField - e87cc898-6f6b-42fb-9934-c01453be999f (Java, offline) test-project - 8ec5a3f5-b659-4c15-a9db-43e3cd9049c8 (Java, offline) ROOT - 6ae2561b-b978-4a5e-9427-b528a350519a (Java, offline) petclinic-linux-container - b7eee43a-4c46-4dd5-bb01-c0c697c3e1b3 (Java, offline) flask-app - c72a982c-041b-468c-b17a-77d884cb49c5 (Python, offline) Email-Service - 09498e3e-d20b-4203-926d-25503a5f3cc7 (Java, offline) dvapi - ba0184e9-4ae7-44c9-8482-4c6f6df0ff35 (Node, online) DotnetDemo - 14be5c36-0c9f-4f73-ab00-8d37114eac50 (.NET Core, offline) DemoApplication - 87d06873-711c-42bb-a8e5-07923f8b245d (.NET Core, offline) cSharp-demo - 443f5d89-6265-495f-950f-9eab221ddf56 (.NET Core, offline) contrast-node-demo - f3a1fcfd-ad2d-4f25-b2ea-1a27e175d12f (Node, offline) contrast-demo - ffb0cac8-389f-4f1a-9240-c770e492c66e (Java, offline) Note: dvapi is currently the only application with "online" status.`

this is no longer an issue, Chris had me reboot my vscode (shut off completely and turn back on) and it corrected the issue.

@ChrisEdwards ChrisEdwards merged commit a53381e into main Nov 14, 2025
4 checks passed
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.

4 participants