AIML-239: Part 3 - Consolidate Domain 7 (Vulnerabilities) #37
+2,740
−438
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChrisEdwards
added a commit
that referenced
this pull request
Nov 20, 2025
Fixed compilation errors in AssessServiceIntegrationTest where test calls were passing parameters in wrong order. The searchVulnerabilities() method signature has 'statuses' as 4th parameter, but tests were trying to pass 'appId' (which was removed in the org-level consolidation). Changes: - Fixed 4 test method calls to use correct parameter order - Changed 'appId' parameter to 'statuses' in all searchVulnerabilities() calls - All 270 unit tests pass - All 50 integration tests pass (6 SAST skipped as expected) Resolves code review feedback from PR #37 (mcp-jap) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
ChrisEdwards
added a commit
that referenced
this pull request
Nov 20, 2025
Fixed compilation errors in AssessServiceIntegrationTest where test calls were passing parameters in wrong order. The searchVulnerabilities() method signature has 'statuses' as 4th parameter, but tests were trying to pass 'appId' (which was removed in the org-level consolidation). Changes: - Fixed 4 test method calls to use correct parameter order - Changed 'appId' parameter to 'statuses' in all searchVulnerabilities() calls - All 270 unit tests pass - All 50 integration tests pass (6 SAST skipped as expected) Resolves code review feedback from PR #37 (mcp-jap) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
ChrisEdwards
force-pushed
the
AIML-239-part3-domain-7-vulnerabilities
branch
from
7c47059 to
cb49709
Compare
ChrisEdwards
force-pushed
the
AIML-239-part2-domains-4-7
branch
from
dd44371 to
b778794
Compare
Create new app-scoped vulnerability search tool consolidating 3 old tools (list_vulnerabilities, list_vulns_by_app_and_metadata, list_vulns_by_app_latest_session) into single unified interface. Features: - Required appId parameter with validation - All standard filters (severity, status, environment, dates, tags) - Session-based filtering: useLatestSession, sessionMetadataName/Value - Fallback: Returns all app vulns with warning when no session found - Case-insensitive in-memory session metadata filtering - Always expands: SESSION_METADATA, SERVER_ENVIRONMENTS, APPLICATION - Returns PaginatedResponse<VulnLight> Testing: - Add 11 comprehensive unit tests - All 268 unit tests + 50 integration tests passing Part of Phase 1 of 4-phase vulnerability tool consolidation (mcp-dd4). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Phase 4 of vulnerability consolidation adds 4 critical unit tests: - useLatestSession fallback: Verifies warning when no session found - Null SDK response: Tests error handling for null API responses - Session metadata filtering: Case-insensitive metadata matching with pagination - Standard filters: Validates all filter parameters pass correctly to SDK All 270 tests passing (up from 266). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…7sa, mcp-3av) Phase 2 (mcp-7sa): - Renamed getAllVulnerabilities → searchVulnerabilities - Renamed tool: list_all_vulnerabilities → search_vulnerabilities - Removed appId parameter (org-level only) - Always use getTracesInOrg() endpoint - Updated tool description with cross-references Phase 3 (mcp-3av): - Deleted listVulnsByAppId() (list_vulnerabilities) - Deleted listVulnsByAppIdAndSessionMetadata() (list_vulns_by_app_and_metadata) - Deleted listVulnsByAppIdForLatestSession() (list_vulns_by_app_latest_session) - Migrated integration tests to use search_app_vulnerabilities Result: 6 vulnerability tools → 4 tools (33% reduction) All 266 unit tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fixed compilation errors in AssessServiceIntegrationTest where test calls were passing parameters in wrong order. The searchVulnerabilities() method signature has 'statuses' as 4th parameter, but tests were trying to pass 'appId' (which was removed in the org-level consolidation). Changes: - Fixed 4 test method calls to use correct parameter order - Changed 'appId' parameter to 'statuses' in all searchVulnerabilities() calls - All 270 unit tests pass - All 50 integration tests pass (6 SAST skipped as expected) Resolves code review feedback from PR #37 (mcp-jap) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…sults Addressed two NPE issues identified by codex autonomous agent: 1. **AssessService** (mcp-d7h): Fixed NullPointerException in search_app_vulnerabilities when sessionMetadataValue or metadataItem.getValue() is null. - Treat null sessionMetadataValue as wildcard (match on name only) - Add null check for metadataItem.getValue() before equalsIgnoreCase - Added 2 unit tests covering both null scenarios 2. **SastService** (mcp-dvf): Fixed NullPointerException in get_scan_results when project.lastScanId() is null (common for projects with no completed scans). - Check if lastScanId is null before calling scans.get() - Check if scan is null after retrieval - Return descriptive IOException with clear user-facing message - Updated existing test + added new test for null scan scenario All 272 unit tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…taName (mcp-b9y) ## Problem The useLatestSession parameter only worked when sessionMetadataName was also provided. When used alone, it silently did nothing and returned vulnerabilities from ALL sessions. Root cause: The code had two separate filter objects (filterForm and filterBody) with two API call paths. The agent session ID was set on filterBody, but when sessionMetadataName was NOT provided, the code used filterForm (which lacked the session ID), causing the filter to be lost. ## Solution: Unified In-Memory Session Filtering - Eliminated filterBody object entirely - use only filterForm - All session filtering (useLatestSession AND sessionMetadataName) now happens consistently in-memory after fetching from the API - One filter object, one code path for session filtering - Removed mutual exclusivity validation - both filters can now work together ## Changes - AssessService.java: - Removed filterBody creation and manual field copying (lines 701-721) - Unified session filtering logic using needsInMemorySessionFiltering flag - Added agent session ID filtering before sessionMetadataName filtering - Updated pagination to handle both filter types - Removed validation preventing both filters from being used together - AssessServiceTest.java: - Updated mutual exclusivity test to verify both filters work together - Added test: useLatestSession works alone (main bug fix verification) - Fixed existing session metadata tests to use new 3-parameter getTraces API - Added lenient stubbing for mocks that may not be accessed ## Expected Outcomes ✅ useLatestSession works independently (no sessionMetadataName required) ✅ Both filters can be combined (useLatestSession + sessionMetadataName) ✅ Simplified architecture (one filter object, one code path) ✅ Consistent behavior (all session filtering is in-memory) ✅ Proper pagination (works for all filtering scenarios) ## Testing All 274 tests pass including: - New test: searchAppVulnerabilities_should_filter_by_useLatestSession_alone - New test: searchAppVulnerabilities_should_support_both_useLatestSession_and_sessionMetadataName_together - All existing session metadata filtering tests updated and passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Session metadata values can contain sensitive information like branch names, build IDs, environment names, and developer identifiers. Logging these at INFO level means they appear in production logs where they may be exposed to unauthorized viewers. This change moves the log statement in searchAppVulnerabilities from INFO to DEBUG level, ensuring sensitive session metadata values are only logged when DEBUG logging is explicitly enabled. Changes: - AssessService.java:629 - Changed log.info() to log.debug() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Added 3 unit tests to verify status filters work correctly after mcp-b9y's unified refactoring eliminated the filterBody object: 1. searchAppVulnerabilities_should_apply_status_filters_with_sessionMetadataName() - Verifies explicit status filters are passed to SDK when using sessionMetadataName - Uses ArgumentCaptor to verify SDK receives status filters 2. searchAppVulnerabilities_should_apply_status_filters_with_useLatestSession() - Verifies explicit status filters are passed to SDK when using useLatestSession - Tests that status filtering works independently with session ID filtering 3. searchAppVulnerabilities_should_use_smart_defaults_with_sessionMetadataName() - Verifies smart default statuses (Reported, Suspicious, Confirmed) are applied - Confirms Fixed and Remediated are excluded from smart defaults - Tests status filtering with session metadata filtering combined All tests use ArgumentCaptor to verify the CRITICAL fix: status filters are now always passed to SDK in the filterForm, regardless of session filtering mode. This verifies the bug is fixed by the unified architecture approach. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ry (mcp-b7s) - Update sessionMetadataName @ToolParam: Document case-insensitive matching, guide users to call get_session_metadata(appId) to discover available fields - Update sessionMetadataValue @ToolParam: Document case-insensitive matching, guide users to discover values via get_session_metadata(appId) - Remove hardcoded examples (Branch, Environment) that suggested fixed fields - Session metadata fields are dynamic per-customer configuration, not enums 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Problem: - When filtering by sessionMetadataName, vulnerabilities appearing in multiple sessions were added to results multiple times - Caused inflated totals, broken pagination, and duplicate entries - Common scenario: vuln found in both "main" and "develop" branches Root Cause: - Nested loop used `break` which only exited innermost loop - Code continued iterating over remaining SessionMetadata objects - Same vulnerability added multiple times if it matched in multiple sessions Solution: - Added labeled break `sessionLoop:` on SessionMetadata loop - Changed `break` to `break sessionLoop` to exit both inner loops - Ensures each vulnerability appears at most once in results - Continues processing other vulnerabilities correctly Test Coverage: - Added test case for vulnerability with 2 matching session metadata objects - Verifies vulnerability appears exactly once (not duplicated) - All existing tests pass (62 tests total) Benefits: - Accurate pagination counts - No duplicate vulnerabilities in results - Slight performance improvement (stops after first match per vuln) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Implement anonymous builders following established pattern from AnonymousApplicationBuilder to reduce test boilerplate and improve readability. Created 6 builders total (3 in this commit, 3 previously): New builders in this commit: - AnonymousMetadataItemBuilder: Simple metadata item mocking - AnonymousTraceBuilder: Complex trace mocking with SessionMetadata helpers - AnonymousProjectBuilder: SAST Project interface mocking Previously committed builders: - AnonymousSessionMetadataBuilder: Session metadata with MetadataItem lists - AnonymousScanBuilder: SAST Scan interface mocking with SARIF support - AnonymousLibraryExtendedBuilder: SCA library mocking with many fields Refactored test files: - AssessServiceTest: 20+ trace mocks simplified (SessionMetadata patterns) - SastServiceTest: All 5 Project and 1 Scan mocks refactored - SCAServiceTest: All 3 LibraryExtended helper methods refactored Benefits: - Reduces 10+ lines of mock setup to 1-3 lines per object - Lenient stubbing avoids UnnecessaryStubbingException - Fluent API improves test readability - Convenience methods simplify complex nested object creation All 278 tests pass. Addresses mcp-hyj. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add symmetric validation to reject sessionMetadataValue when provided without sessionMetadataName. Previously, the tool silently ignored sessionMetadataValue in this scenario, causing users to receive unfiltered results without any indication of the misconfiguration. Changes: - Added validation check rejecting null/empty sessionMetadataName with non-null sessionMetadataValue (mirrors existing reverse check) - Updated @tool description with explicit note about parameter dependency - Updated JavaDoc for both parameters stating "Must be provided with..." - Updated @throws documentation to cover both validation directions Testing: - Added testGetRouteCoverage_SessionMetadataValue_WithoutName() test - Added testGetRouteCoverage_SessionMetadataValue_WithEmptyName() test - All 327 tests passing (26 RouteCoverageService tests) Users now receive immediate, clear error messages instead of silently incorrect results. AI models consuming the tool see explicit documentation about the parameter dependency. Addresses mcp-rlc. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
ChrisEdwards
force-pushed
the
AIML-239-part3-domain-7-vulnerabilities
branch
from
8f343b1 to
edb3567
Compare
ChrisEdwards
changed the base branch from
AIML-239-part2-domains-4-7
to
main
ChrisEdwards
changed the title
Establish "never null collections" pattern across Application class: - Add defensive getters for all collection fields (tags, techs, roles, policies, metadataEntities, validationErrorFields, missingRequiredFields) - Update AnonymousApplicationBuilder to enforce pattern in tests - Simplify AssessService.matchesMetadataFilter() (no null check needed) - Add test for untagged apps scenario - Update ApplicationJsonParsingTest to expect empty list instead of null This prevents NPE when filtering by tag on apps that have no tags (extremely common default state in most organizations).
Remove dead code: appId field was stored but never accessed in production. Routing is explicit in AssessService via different SDK calls, so appId in VulnerabilityFilterParams served no purpose. Changes: - Remove appId from record definition and of() method (7 args now, was 8) - Update 2 call sites in AssessService.java - Remove testAppIdPassedThrough test, update all others to 7 args All 280 tests passing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
This PR completes the AIML-239 MCP tool consolidation epic by consolidating the vulnerability domain (Domain 7 of 7).
Problems solved:
list_vulnerabilities,list_vulns_by_app_and_metadata, andlist_vulns_by_app_latest_sessionall searched app-scoped vulnerabilities with slightly different parameterslist_all_vulnerabilitiesvslist_vulnerabilitiesvslist_vulns_*sessionMetadataName-useLatestSessionsilently failed when used aloneBusiness value:
search_*for flexible filtering,get_*for single itemsuseLatestSessionnow works independentlyWhat
Tool Changes
list_all_vulnerabilitiessearch_vulnerabilitieslist_vulnerabilitiessearch_app_vulnerabilitieslist_vulns_by_app_and_metadatasearch_app_vulnerabilitieslist_vulns_by_app_latest_sessionsearch_app_vulnerabilitiesget_vulnerabilityget_vulnerabilitylist_vulnerability_typeslist_vulnerability_typesAdditional Improvements
Bug fixes discovered during development:
useLatestSessionto work withoutsessionMetadataName(was silently broken)Code quality improvements:
How
Unified Session Filtering Architecture (mcp-b9y)
Before: Two separate filter objects (
filterFormandfilterBody) with two code paths:sessionMetadataNameprovided → usedfilterBodywith agent session ID ✅useLatestSessionprovided → usedfilterForm(missing session ID) ❌After: Single unified approach:
filterForm) for all scenariosuseLatestSessionANDsessionMetadataNamedo in-memory filteringAPI Routing Separation
search_vulnerabilities→ Always usesContrastSDK.getTracesInOrg(orgID, filterForm)(org-level)search_app_vulnerabilities→ Always usesContrastSDK.getTraces(orgID, appId, filterForm)(app-level)This separation eliminates complex conditional routing and makes each tool's behavior predictable.
Never Null Collections Pattern (mcp-7jf)
Established project standard: Collection fields must never return null. Applied to
Application.java:Prevents NPE across all filtering operations without defensive null checks everywhere.
Step-by-Step Walkthrough
Phase 1: Create
search_app_vulnerabilities(mcp-5es)File:
AssessService.java- New method ~200 linesNew unified app-scoped search consolidating 3 old tools:
appIdsessionMetadataName,sessionMetadataValue,useLatestSessionKey behaviors:
useLatestSessionfallback: Returns all app vulnerabilities with warning if no session foundSESSION_METADATA,SERVER_ENVIRONMENTS,APPLICATIONPhase 2: Rename
search_vulnerabilities(mcp-7sa)File:
AssessService.java- Method rename + simplificationgetAllVulnerabilities()→searchVulnerabilities()list_all_vulnerabilities→search_vulnerabilitiesappIdparameter - org-level only nowgetTracesInOrg()endpointPhase 3: Remove old tools (mcp-3av)
File:
AssessService.java- Deleted 3 methodsDeleted methods:
listVulnsByAppId()(waslist_vulnerabilities)listVulnsByAppIdAndSessionMetadata()(waslist_vulns_by_app_and_metadata)listVulnsByAppIdForLatestSession()(waslist_vulns_by_app_latest_session)Migrated integration tests to use
search_app_vulnerabilities.Phase 4: Bug fixes from code review (mcp-fs0 children)
mcp-b9y - Unified session filtering:
filterBodyobject entirelyuseLatestSessionworks independentlymcp-3sy - Status filter verification:
mcp-7jf - Null safety for collections:
Application.java(7 collection fields)AnonymousApplicationBuilderto enforce patternmatchesMetadataFilter()- no null check neededmcp-wts - Log verbosity:
mcp-b7s - Documentation:
@ToolParamdescriptions for sessionMetadata paramsget_session_metadata(appId)for discoverymcp-m0x - Dead code removal:
appIdfromVulnerabilityFilterParamsrecordPhase 5: Test infrastructure (mcp-hyj)
Created 7 anonymous builders for test mocks:
AnonymousApplicationBuilder- Application mockingAnonymousTraceBuilder- Trace with SessionMetadata helpersAnonymousSessionMetadataBuilder- Session metadata listsAnonymousMetadataItemBuilder- Simple metadata itemsAnonymousProjectBuilder- SAST Project interfaceAnonymousScanBuilder- SAST Scan interfaceAnonymousLibraryExtendedBuilder- SCA library mockingBenefits: Reduces 10+ lines of mock setup to 1-3 lines, lenient stubbing prevents
UnnecessaryStubbingException.Testing
Test Summary
Coverage by Area
AssessService vulnerability tools:
AssessServiceTest.javaIntegration tests:
testSearchAppVulnerabilitiesWithSessionMetadata- Basic app searchtestSearchAppVulnerabilitiesForLatestSessionWithDynamicSessionId- useLatestSession behaviorNew test patterns:
Files Changed
AssessService.javasearchAppVulnerabilities, renamedsearchVulnerabilities, deleted 3 old methods, unified session filteringVulnerabilityFilterParams.javaappIdparameterApplication.javaSastService.javaRouteCoverageService.javaAssessServiceTest.java7 Anonymous*Builder.javaCLAUDE.mdDependency Context
This PR is based on
main(after PR #35 and #36 merged).Previous PRs in this epic:
After this PR, AIML-239 is complete:
action_entitynaming conventionBeads Completed