feat(remediation): add remediation extractor with provider priority resolution - #599
Open
ruromero wants to merge 3 commits into
Open
feat(remediation): add remediation extractor with provider priority resolution#599ruromero wants to merge 3 commits into
ruromero wants to merge 3 commits into
Conversation
Reviewer's GuideImplements a remediation extraction pipeline that walks DA AnalysisReport provider/source/dependency/issue trees, resolves provider priority (Lightwell > Red Hat > generic), merges multiple CVE remediations per dependency to a single highest-version entry, incorporates recommendation-based package replacements, and returns a normalized remediation list suitable for consumption by downstream tooling. Flow diagram for remediation extraction and provider priority resolutionflowchart TD
analysisReport[AnalysisReport]
extractRemediations[extractRemediations]
providersLoop[Iterate providers]
extractFromSources[extractFromSources]
extractFromRecommendations[extractFromRecommendations]
processIssueRemediation[processIssueRemediation]
detectProviderPriority[detectProviderPriority]
compareVersions[compareVersions]
remediationsByDep[remediationsByDep Map]
outputList[Sorted remediation list]
analysisReport --> extractRemediations
extractRemediations --> providersLoop
providersLoop --> extractFromSources
providersLoop --> extractFromRecommendations
extractFromSources --> processIssueRemediation
processIssueRemediation --> detectProviderPriority
processIssueRemediation --> compareVersions
processIssueRemediation --> remediationsByDep
extractFromRecommendations --> detectProviderPriority
detectProviderPriority --> remediationsByDep
extractFromRecommendations --> remediationsByDep
remediationsByDep --> outputList
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 4 issues, and left some high level feedback:
- The
compareVersionshelper assumes purely dot-separated numeric segments, which may not correctly order versions with qualifiers like1.10.0.redhat-00001; consider using a more robust Maven/semver-aware comparison or a fallback that preserves qualifier significance. - Provider priority detection in
detectProviderPriorityis based on substring matches (.rhlw-,.redhat-), which could misclassify PURLs if those tokens appear outside the version qualifier; you may want to inspect the parsedPackageURL.versionor qualifiers instead of the raw string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `compareVersions` helper assumes purely dot-separated numeric segments, which may not correctly order versions with qualifiers like `1.10.0.redhat-00001`; consider using a more robust Maven/semver-aware comparison or a fallback that preserves qualifier significance.
- Provider priority detection in `detectProviderPriority` is based on substring matches (`.rhlw-`, `.redhat-`), which could misclassify PURLs if those tokens appear outside the version qualifier; you may want to inspect the parsed `PackageURL.version` or qualifiers instead of the raw string.
## Individual Comments
### Comment 1
<location path="src/remediation.js" line_range="301-302" />
<code_context>
+ * @param {string} b
+ * @returns {number}
+ */
+function compareVersions(a, b) {
+ const partsA = a.split('.').map(s => parseInt(s, 10) || 0)
+ const partsB = b.split('.').map(s => parseInt(s, 10) || 0)
+ const len = Math.max(partsA.length, partsB.length)
</code_context>
<issue_to_address>
**issue (bug_risk):** Version comparison treats any non-numeric segment as 0, which can misorder common semver patterns.
Non-numeric segments (e.g. `1.2.3-beta`, `2024.01-1`) are coerced to `0`, which can produce incorrect ordering or equality and may select the wrong fixed version when qualifiers are present. Please either use a semver/Maven-style comparator where appropriate, or keep the raw segments and apply a comparison that handles non-numeric parts lexically instead of collapsing them to `0`.
</issue_to_address>
### Comment 2
<location path="src/remediation.js" line_range="209-214" />
<code_context>
+ cves: [],
+ _priority: priority,
+ })
+ } else if (priority > existing._priority) {
+ existing.fixedInVersion = fixedInVersion
+ existing.fixedInPurl = recommendedPurl
+ existing.provider = providerName
+ existing.source = 'recommendation'
+ existing._priority = priority
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Recommendations only override existing entries on strictly higher priority, ignoring equal-priority but higher versions.
In `extractFromRecommendations`, entries with `priority === existing._priority` are skipped entirely, even when they have a higher `fixedInVersion`. In contrast, `processIssueRemediation` compares versions for equal-priority entries to keep the highest one. To avoid missing better remediations and to keep behavior consistent, update `extractFromRecommendations` to also compare versions when priorities are equal.
</issue_to_address>
### Comment 3
<location path="test/remediation.test.js" line_range="188-190" />
<code_context>
+ })
+ })
+
+ suite('version merging', () => {
+ /** Verifies that multiple CVEs on the same dependency produce a single entry with the highest fix version. */
+ test('multiple CVEs on same dependency produce single entry with highest version', () => {
+ // Given a dependency with two CVEs having different fix versions from the same provider
+ const report = buildReport({
</code_context>
<issue_to_address>
**suggestion (testing):** Add test coverage for advisory merging across multiple issues on the same dependency
The code merges advisories via `mergeAdvisories`, deduping by `id`, but current tests only cover a single-issue case. Please add a test where the same dependency appears in multiple issues with distinct advisories (e.g., one from `trustedContent.advisory` and one from `issue.advisories`) and assert that the resulting `advisories` array is the deduplicated union. This will better validate the multi-CVE advisory merge behavior.
Suggested implementation:
```javascript
// When extracting remediations
const result = extractRemediations(report)
// Then Red Hat should win
expect(result).to.have.lengthOf(1)
expect(result[0].fixedInPurl).to.include('.redhat-')
expect(result[0].provider).to.equal('redhat')
})
/** Verifies that advisories from multiple issues and trusted content merge into a deduplicated union. */
test('advisories from multiple issues on same dependency are merged and deduplicated', () => {
// Given the same dependency referenced in multiple issues and trusted content,
// with overlapping advisory IDs
const report = buildReport({
issues: [
{
id: 'issue-1',
dependencyRef: 'pkg:maven/org.apache.commons/commons-text@1.9.0?type=jar',
advisories: [
{
id: 'ADV-ISSUE-1',
url: 'https://example.com/advisory/ADV-ISSUE-1',
},
{
id: 'ADV-SHARED',
url: 'https://example.com/advisory/ADV-SHARED-from-issue',
},
],
},
{
id: 'issue-2',
dependencyRef: 'pkg:maven/org.apache.commons/commons-text@1.9.0?type=jar',
advisories: [
{
id: 'ADV-ISSUE-2',
url: 'https://example.com/advisory/ADV-ISSUE-2',
},
],
},
],
trustedContent: [
{
providerName: 'lightwell',
sourceName: 'lightwell-security',
trustedContentRef:
'pkg:maven/org.apache.commons/commons-text@1.9.0.rhlw-00001?type=jar',
fixedIn: null,
// One unique advisory and one that shares the same id as an issue advisory
advisories: [
{
id: 'ADV-TRUSTED-1',
url: 'https://lightwell.example.com/ADV-TRUSTED-1',
},
{
id: 'ADV-SHARED',
url: 'https://lightwell.example.com/ADV-SHARED-from-trusted',
},
],
},
],
})
// When extracting remediations
const result = extractRemediations(report)
// Then the advisories for the commons-text remediation should be the deduplicated union by id
expect(result).to.have.lengthOf(1)
const advisories = result[0].advisories
const advisoryIds = advisories.map((a) => a.id)
// We expect three distinct advisory IDs:
// - ADV-ISSUE-1 (issue-only)
// - ADV-ISSUE-2 (issue-only)
// - ADV-TRUSTED-1 (trusted-content-only)
// The overlapping ADV-SHARED should only appear once
expect(advisoryIds).to.have.members(['ADV-ISSUE-1', 'ADV-ISSUE-2', 'ADV-TRUSTED-1', 'ADV-SHARED'])
expect(advisories).to.have.lengthOf(4)
})
})
```
The above test assumes:
1. `buildReport` accepts `issues` and `trustedContent` properties where:
- `issues[*].dependencyRef` matches the dependency coordinate used elsewhere in the tests.
- `issues[*].advisories` is an array of advisory objects.
- `trustedContent[*].advisories` is an array (not a single `advisory` object). If your current trusted-content model uses a singular `advisory` field, adjust the fixture to match (e.g. change `advisories` to `advisory` and adapt how `mergeAdvisories` consumes it).
2. `extractRemediations(report)` returns an array of remediation objects where `remediation.advisories` is the merged advisory array for that dependency. If the property name differs (e.g. `mergedAdvisories` or `details.advisories`), update the test accordingly.
3. If the dependency-identifying property is not `dependencyRef` (e.g. `purl` or `dependency`), change `dependencyRef` in the fixture to the correct field name so all three records clearly refer to the same dependency.
Please align the field names in the new test with your existing `buildReport` and `mergeAdvisories` implementations to ensure the test accurately exercises the advisory-merge path.
</issue_to_address>
### Comment 4
<location path="test/remediation.test.js" line_range="361-363" />
<code_context>
+ })
+ })
+
+ suite('output structure', () => {
+ /** Verifies that the output includes all required fields with correct types. */
+ test('output includes all required fields', () => {
+ const report = buildReport({
+ trustedContentRef: 'pkg:maven/org.apache.commons/commons-text@1.10.0.rhlw-00001?type=jar',
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for defaulted and alternative fields (severity default, issue.cve, and sorting by purl)
The current test covers field presence and types but not key behaviours: defaulting `severity` to `UNKNOWN` when missing, handling CVE identifiers via both `issue.id` and `issue.cve`, and sorting results by `purl`. Please add targeted tests for each of these cases to fully validate `extractRemediations`’ behaviour.
Suggested implementation:
```javascript
},
})
const result = extractRemediations(report)
expect(result).to.have.lengthOf(1)
expect(result[0].fixedInPurl).to.include('.redhat-')
})
test('defaults missing severity to UNKNOWN', () => {
const report = buildReport({
trustedContentRef: 'pkg:maven/org.example/foo@1.0.0?type=jar',
// Intentionally omit severity to verify defaulting behaviour
severity: undefined,
fixedIn: '1.0.1',
advisory: { id: 'RHLW-2022-002', url: 'https://lightwell.example.com/RHLW-2022-002' },
})
const result = extractRemediations(report)
expect(result).to.have.lengthOf(1)
expect(result[0].severity).to.equal('UNKNOWN')
})
test('prefers issue.cve over issue.id when present', () => {
const report = buildReport({
trustedContentRef: 'pkg:maven/org.example/bar@2.0.0?type=jar',
severity: 'HIGH',
fixedIn: '2.0.1',
issue: {
// Both id and cve present – cve should be used as the canonical identifier
id: 'LIGHTWELL-2022-003',
cve: 'CVE-2022-0003',
},
advisory: { id: 'RHLW-2022-003', url: 'https://lightwell.example.com/RHLW-2022-003' },
})
const result = extractRemediations(report)
expect(result).to.have.lengthOf(1)
// The remediation should expose the CVE identifier when available
expect(result[0].issueId).to.equal('CVE-2022-0003')
})
test('results are sorted by purl', () => {
const report = buildReport({
// Provide multiple remediations with deliberately unsorted purls
remediations: [
{
trustedContentRef: 'pkg:maven/org.example/zzz@3.0.0?type=jar',
severity: 'LOW',
fixedIn: '3.0.1',
issue: { id: 'ISSUE-3' },
advisory: { id: 'RHLW-2022-004', url: 'https://lightwell.example.com/RHLW-2022-004' },
},
{
trustedContentRef: 'pkg:maven/org.example/aaa@1.0.0?type=jar',
severity: 'CRITICAL',
fixedIn: '1.0.1',
issue: { id: 'ISSUE-1' },
advisory: { id: 'RHLW-2022-005', url: 'https://lightwell.example.com/RHLW-2022-005' },
},
{
trustedContentRef: 'pkg:maven/org.example/mmm@2.0.0?type=jar',
severity: 'MEDIUM',
fixedIn: '2.0.1',
issue: { id: 'ISSUE-2' },
advisory: { id: 'RHLW-2022-006', url: 'https://lightwell.example.com/RHLW-2022-006' },
},
],
})
const result = extractRemediations(report)
expect(result).to.have.lengthOf(3)
const purls = result.map(r => r.purl)
// Verify that the output is sorted lexicographically by purl
expect(purls).to.deep.equal([...purls].sort())
})
})
```
The new tests assume the following about the existing helpers and data structures:
1. `buildReport` can accept:
- A flat object with `trustedContentRef`, `severity`, `fixedIn`, `issue`, and `advisory` to represent a single remediation.
- An object with `remediations: [...]` when testing multiple entries. If the current helper uses a different shape (e.g. `vulnerabilities`, `issues`, or `components`), adapt the arguments so that `extractRemediations` receives the intended inputs.
2. `extractRemediations` returns objects that include:
- `severity` (string), which should be `'UNKNOWN'` when no severity is provided in the source data.
- `issueId` (string), representing the chosen identifier; update the assertion to match the actual property name if it differs (e.g. `id`, `cve`, or `identifier`).
- `purl` (string) used for sorting; if the field is named differently (e.g. `packageUrl`), adjust the mapping in the sorting test accordingly.
3. If `buildReport` does not currently support constructing multiple remediations via a `remediations` array, you may need to:
- Extend the helper to accept and merge such arrays into the underlying report structure in the same way your production data is handled.
- Or alternatively, create multiple separate reports and merge their `extractRemediations` outputs before asserting sort order.
Ensure these tests are placed within the same `suite('output structure', ...)` block as the existing "output includes all required fields" test to keep related behaviour grouped together.
</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 #599 +/- ##
==========================================
- Coverage 91.04% 90.97% -0.08%
==========================================
Files 40 41 +1
Lines 8635 8977 +342
Branches 1501 1573 +72
==========================================
+ Hits 7862 8167 +305
- Misses 773 810 +37
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Collaborator
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review — Classified as suggestion (2 items). Both high-level suggestions (compareVersions numeric coercion and detectProviderPriority substring matching) were already addressed: compareVersions now falls back to lexicographic comparison (commit 6ecff9b), and provider priority detection was replaced with configurable providerPriority option (commit 6ecff9b, removing hardcoded vendor strings entirely). No sub-task created. |
Collaborator
Author
Verification Report for TC-5410 (commit 9b7ceeb)
Overall: PASSAll checks pass. The implementation satisfies the task requirements with an intentional design improvement: provider priority is configurable via This comment was AI-generated by sdlc-workflow/verify-pr v0.13.7. |
…esolution Adds src/remediation.js module that parses DA AnalysisReport responses and produces actionable upgrade instructions. Implements three-tier provider priority: Lightwell (.rhlw-) > Red Hat (.redhat-) > generic. For the same dependency affected by multiple CVEs, selects the highest fixedIn version from the highest-priority provider. Also extracts package replacement recommendations. Includes comprehensive test suite. Implements TC-5410 Assisted-by: Claude Code
… hardcoded Removes hardcoded vendor-specific priority detection (Red Hat, Lightwell PURL substring matching). Provider priority is now driven by the caller via options.providerPriority — an array of provider names in descending priority order. Providers not listed share the lowest priority. When omitted, all providers are treated equally and the highest fix version wins. Also fixes compareVersions to fall back to lexicographic comparison for non-numeric version segments. Implements TC-5410 Assisted-by: Claude Code
- Store provider rank in separate Map instead of _priority field on output objects - Normalize severity to uppercase in higherSeverity() for consistent output regardless of provider casing - Add version comparison in extractFromRecommendations at equal rank so recommendations don't silently lose to lower-version source fixes - Add tests for severity normalization and source+recommendation CVE/advisory preservation Implements TC-5410 Assisted-by: Claude Code
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
2 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
src/remediation.jsmodule exportingextractRemediations(analysisReport)that parses DA AnalysisReport responses and produces actionable upgrade instructions.rhlw-) > Red Hat (.redhat-) > genericfixedInversion from the highest-priority providerProviderReport.recommendationsImplements TC-5410
Summary by Sourcery
Introduce a remediation extraction module that derives prioritized upgrade instructions from DA AnalysisReport data and exposes them as a normalized output structure.
New Features:
Tests: