fix(hardened): preserve multiple recommendations per base image#637
Merged
Conversation
Reviewer's GuideThis PR fixes hardened-image recommendation loss when multiple hardened images share the same base image, by making the hardened index and downstream pipeline list-valued instead of single-valued, and ensures filtered responses preserve all recommendations. Sequence diagram for multi-recommendation lookupBySbomIdsequenceDiagram
actor Client
participant HardenedImageProvider
participant HardenedImageIndex
Client->>HardenedImageProvider: lookupBySbomId(sbomId)
HardenedImageProvider->>HardenedImageProvider: extract baseImageRef from sbomId
HardenedImageProvider->>HardenedImageIndex: get(baseImageRef)
HardenedImageIndex-->>HardenedImageProvider: List<IndexedRecommendation>
HardenedImageProvider-->>Client: Map<PackageRef, List<IndexedRecommendation>>
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In RecommendationAggregation.normalizeRecommendations, any map entries whose values are neither List nor IndexedRecommendation are silently discarded; consider logging or handling this case explicitly so unexpected payloads don't get dropped without visibility.
- HardenedImageIndex.get now returns an empty list by default, but HardenedImageProvider.lookup delegates directly to it; if callers distinguish between 'no entry' and 'entry with zero recommendations', you may want a method or contract comment that clarifies this behavior or exposes a null-vs-empty distinction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In RecommendationAggregation.normalizeRecommendations, any map entries whose values are neither List nor IndexedRecommendation are silently discarded; consider logging or handling this case explicitly so unexpected payloads don't get dropped without visibility.
- HardenedImageIndex.get now returns an empty list by default, but HardenedImageProvider.lookup delegates directly to it; if callers distinguish between 'no entry' and 'entry with zero recommendations', you may want a method or contract comment that clarifies this behavior or exposes a null-vs-empty distinction.
## Individual Comments
### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/model/trustify/HardenedImageIndex.java" line_range="31-40" />
<code_context>
+ private volatile Map<String, List<IndexedRecommendation>> index = new ConcurrentHashMap<>();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning an unmodifiable map still exposes mutable lists, allowing external mutation of the index.
In `HardenedImageIndex#getAll()`, `Collections.unmodifiableMap(index)` only protects the map structure; the `List<IndexedRecommendation>` values remain mutable and can be changed by callers, undermining the index’s invariants and thread-safety. If external code should not modify the index, also make the lists unmodifiable (e.g. via `Collections.unmodifiableList`) or return a fully defensive copy of both the map and its lists.
Suggested implementation:
```java
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
```
```java
public void replaceAll(Map<String, List<IndexedRecommendation>> newData) {
ConcurrentHashMap<String, List<IndexedRecommendation>> newIndex = new ConcurrentHashMap<>();
for (Map.Entry<String, List<IndexedRecommendation>> entry : newData.entrySet()) {
List<IndexedRecommendation> recommendations = entry.getValue();
// Normalize null to an empty, unmodifiable list to keep invariants simple.
if (recommendations == null) {
recommendations = Collections.emptyList();
}
// Store an unmodifiable view so external code cannot mutate the internal index.
newIndex.put(entry.getKey(), Collections.unmodifiableList(recommendations));
}
this.index = newIndex;
}
/**
* Returns a snapshot of the current index. The returned map and its lists are
* unmodifiable, so callers cannot mutate the internal state.
*/
public Map<String, List<IndexedRecommendation>> getAll() {
Map<String, List<IndexedRecommendation>> snapshot = new HashMap<>();
for (Map.Entry<String, List<IndexedRecommendation>> entry : index.entrySet()) {
// Lists in the index are already unmodifiable, but we still wrap defensively
// to ensure the snapshot cannot be mutated even if the internal representation changes.
snapshot.put(entry.getKey(), Collections.unmodifiableList(entry.getValue()));
}
return Collections.unmodifiableMap(snapshot);
}
```
I assumed an existing `getAll()` that previously returned `Collections.unmodifiableMap(index)`. If its signature or name differ, adjust the `getAll` method accordingly. Also, if `null` lists should be treated differently (e.g., preserved as `null` instead of normalized to empty lists), tweak the `replaceAll` handling to match your domain requirements.
</issue_to_address>
### Comment 2
<location path="src/test/java/io/github/guacsec/trustifyda/integration/providers/trustify/hardened/HardenedImageProviderTest.java" line_range="443-452" />
<code_context>
+ /// Verifies that lookupBySbomId returns all recommendations when multiple hardened images match.
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions in `testLookupBySbomIdReturnsMultipleRecommendations` to verify the actual recommendations returned.
Right now the test only checks that two recommendations are returned, which verifies the 1→N behavior but not that the actual recommendations are correct. Please also assert on the contents—for example, extract the package names from `recommendations` and verify they match `hardened-nginx` and `hardened-nginx-v2` (order-independent). This will catch cases where duplicates or wrong recommendations are returned but the `size() == 2` assertion still passes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
============================================
+ Coverage 56.77% 56.86% +0.09%
- Complexity 815 823 +8
============================================
Files 94 94
Lines 4793 4815 +22
Branches 618 624 +6
============================================
+ Hits 2721 2738 +17
- Misses 1800 1802 +2
- Partials 272 275 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review —
|
Contributor
Author
Verification Report for TC-4933 (commit 354d54b)
Overall: FAILCommit This comment was AI-generated by sdlc-workflow/verify-pr v0.11.0. |
Collaborator
PR Verification Report — TC-4933Task: TC-4933 — Preserve multiple hardened image recommendations per base image Verdicts
FindingsScope Containment — WARNAll 3 task-specified files are present. 5 additional files are reasonable supporting changes for the type signature refactor:
Commit Traceability — FAILThe single commit Test Quality — WARNTest methods in Test Change Classification — ADDITIVE
This comment was AI-generated by sdlc-workflow/verify-pr v0.11.0. |
ruromero
requested changes
Jun 25, 2026
ruromero
left a comment
ruromero
left a comment
Collaborator
There was a problem hiding this comment.
Fix the commit traceability and the /// comment. Please make sure to add it to the conventions file if it isn't there to prevent it from happening again.
…933) HashMap.put() in HardenedImageResponseHandler silently overwrote earlier entries when multiple hardened images shared the same compare_to base image. Changed to list-based data structures throughout the pipeline so all recommendations are preserved. Also fixed filterCves to carry allRecommendations through to buildRecommendationsMap, preventing data loss on CVE-filtered responses. Added test comment style convention to CONVENTIONS.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @ruromero review — Classified as code change request — reviewer directs fixing commit traceability (missing TC-4933 reference), reverting |
Contributor
Author
Verification Report for TC-4933 (commit 74b0b4f)
Overall: WARNScope Containment and Diff Size are WARN because 6 files beyond the 3 task-specified files were changed. All additional files are justified: Scope Containment — WARNAll 3 task-specified files are present. 6 additional files:
Acceptance Criteria — PASS
Test Change Classification — ADDITIVE
This comment was AI-generated by sdlc-workflow/verify-pr v0.11.0. |
ruromero
approved these changes
Jun 29, 2026
ruromero
pushed a commit
to ruromero/trustify-dependency-analytics
that referenced
this pull request
Jun 30, 2026
…933) (guacsec#637) HashMap.put() in HardenedImageResponseHandler silently overwrote earlier entries when multiple hardened images shared the same compare_to base image. Changed to list-based data structures throughout the pipeline so all recommendations are preserved. Also fixed filterCves to carry allRecommendations through to buildRecommendationsMap, preventing data loss on CVE-filtered responses. Added test comment style convention to CONVENTIONS.md. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
HashMap.put()overwrite bug inHardenedImageResponseHandler.parseAndInvertMapping()— when multiple hardened images share the samecompare_tobase image, all recommendations are now preserved instead of only the last oneMap<String, IndexedRecommendation>→Map<String, List<IndexedRecommendation>>) throughout the hardened image pipelineProviderResponseHandler.filterCves()to preserveallRecommendationswhen creating filtered PackageItem copies, preventing data loss on CVE-filtered responsesJira
TC-4933
Test plan
testParseAndInvertMappingPreservesMultipleRecommendationsPerBasevalidates 1→N mappingtestLookupBySbomIdReturnsMultipleRecommendationsvalidates end-to-end multi-recommendation lookuptestInvalidGzipDeflatedContentEncoding) is unrelated — fails on main as well🤖 Generated with Claude Code
Summary by Sourcery
Support multiple hardened image recommendations per base image and propagate them through the provider and aggregation pipeline.
Bug Fixes:
Enhancements:
Tests: