feat: add remediation report generator for PR body output - #601
Open
ruromero wants to merge 5 commits into
Open
Conversation
Reviewer's GuideImplements a remediation report generator that transforms remediation data into multiple PR-body-friendly formats (per-dependency markdown, bundled-by-severity markdown, dry-run summary, and JSON), plus a deterministic deduplication key builder, all covered by a comprehensive new test suite. 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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/remediation_report.js" line_range="60-69" />
<code_context>
+ lines.push('')
+ lines.push('| CVE | Severity | Advisory |')
+ lines.push('| --- | --- | --- |')
+ for (const cve of rem.cves) {
+ const advisoryLinks = formatAdvisoryLinks(rem.advisories)
+ lines.push(`| ${cve} | ${rem.severity} | ${advisoryLinks} |`)
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recomputing advisory links inside the CVE loop.
`advisoryLinks` depends only on `rem.advisories`, so it has the same value for every CVE. Compute it once before the loop (e.g. `const advisoryLinks = formatAdvisoryLinks(rem.advisories);`) and reuse it in the loop to avoid redundant work and clarify intent.
```suggestion
if (rem.cves.length > 0) {
lines.push('### Vulnerabilities resolved')
lines.push('')
lines.push('| CVE | Severity | Advisory |')
lines.push('| --- | --- | --- |')
const advisoryLinks = formatAdvisoryLinks(rem.advisories)
for (const cve of rem.cves) {
lines.push(`| ${cve} | ${rem.severity} | ${advisoryLinks} |`)
}
}
```
</issue_to_address>
### Comment 2
<location path="test/remediation_report.test.js" line_range="257-274" />
<code_context>
+ })
+ })
+
+ suite('JSON format', () => {
+ /** Verifies that JSON format returns valid JSON string. */
+ test('returns valid JSON string of remediations', () => {
+ const remediations = [buildRemediation()]
+
+ const report = generateReport(remediations, { format: 'json' })
+ const parsed = JSON.parse(report)
+
+ expect(parsed).to.deep.equal(remediations)
+ })
+
+ /** Verifies that empty remediations produce empty JSON array. */
+ test('returns empty JSON array for no remediations', () => {
+ const report = generateReport([], { format: 'json' })
+
+ expect(report).to.equal('[]')
+ })
+ })
+
+ suite('edge cases', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for precedence when `format: 'json'` is combined with `dryRun` or `groupBy` options.
Because the logic returns early when `format === 'json'`, we should add cases like `generateReport(remediations, { format: 'json', dryRun: true })` and `{ format: 'json', groupBy: 'bundle' }` to document and lock in the intended precedence (e.g., JSON always winning) and guard against regressions in future refactors.
```suggestion
suite('JSON format', () => {
/** Verifies that JSON format returns valid JSON string. */
test('returns valid JSON string of remediations', () => {
const remediations = [buildRemediation()]
const report = generateReport(remediations, { format: 'json' })
const parsed = JSON.parse(report)
expect(parsed).to.deep.equal(remediations)
})
/** Verifies that empty remediations produce empty JSON array. */
test('returns empty JSON array for no remediations', () => {
const report = generateReport([], { format: 'json' })
expect(report).to.equal('[]')
})
/** Verifies that JSON format takes precedence over dry-run option. */
test('JSON format takes precedence over dry-run option', () => {
const remediations = [buildRemediation()]
const report = generateReport(remediations, { format: 'json', dryRun: true })
const parsed = JSON.parse(report)
expect(parsed).to.deep.equal(remediations)
})
/** Verifies that JSON format takes precedence over groupBy option. */
test('JSON format takes precedence over groupBy option', () => {
const remediations = [buildRemediation()]
const report = generateReport(remediations, { format: 'json', groupBy: 'bundle' })
const parsed = JSON.parse(report)
expect(parsed).to.deep.equal(remediations)
})
})
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+257
to
+274
| suite('JSON format', () => { | ||
| /** Verifies that JSON format returns valid JSON string. */ | ||
| test('returns valid JSON string of remediations', () => { | ||
| const remediations = [buildRemediation()] | ||
|
|
||
| const report = generateReport(remediations, { format: 'json' }) | ||
| const parsed = JSON.parse(report) | ||
|
|
||
| expect(parsed).to.deep.equal(remediations) | ||
| }) | ||
|
|
||
| /** Verifies that empty remediations produce empty JSON array. */ | ||
| test('returns empty JSON array for no remediations', () => { | ||
| const report = generateReport([], { format: 'json' }) | ||
|
|
||
| expect(report).to.equal('[]') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for precedence when format: 'json' is combined with dryRun or groupBy options.
Because the logic returns early when format === 'json', we should add cases like generateReport(remediations, { format: 'json', dryRun: true }) and { format: 'json', groupBy: 'bundle' } to document and lock in the intended precedence (e.g., JSON always winning) and guard against regressions in future refactors.
Suggested change
| suite('JSON format', () => { | |
| /** Verifies that JSON format returns valid JSON string. */ | |
| test('returns valid JSON string of remediations', () => { | |
| const remediations = [buildRemediation()] | |
| const report = generateReport(remediations, { format: 'json' }) | |
| const parsed = JSON.parse(report) | |
| expect(parsed).to.deep.equal(remediations) | |
| }) | |
| /** Verifies that empty remediations produce empty JSON array. */ | |
| test('returns empty JSON array for no remediations', () => { | |
| const report = generateReport([], { format: 'json' }) | |
| expect(report).to.equal('[]') | |
| }) | |
| }) | |
| suite('JSON format', () => { | |
| /** Verifies that JSON format returns valid JSON string. */ | |
| test('returns valid JSON string of remediations', () => { | |
| const remediations = [buildRemediation()] | |
| const report = generateReport(remediations, { format: 'json' }) | |
| const parsed = JSON.parse(report) | |
| expect(parsed).to.deep.equal(remediations) | |
| }) | |
| /** Verifies that empty remediations produce empty JSON array. */ | |
| test('returns empty JSON array for no remediations', () => { | |
| const report = generateReport([], { format: 'json' }) | |
| expect(report).to.equal('[]') | |
| }) | |
| /** Verifies that JSON format takes precedence over dry-run option. */ | |
| test('JSON format takes precedence over dry-run option', () => { | |
| const remediations = [buildRemediation()] | |
| const report = generateReport(remediations, { format: 'json', dryRun: true }) | |
| const parsed = JSON.parse(report) | |
| expect(parsed).to.deep.equal(remediations) | |
| }) | |
| /** Verifies that JSON format takes precedence over groupBy option. */ | |
| test('JSON format takes precedence over groupBy option', () => { | |
| const remediations = [buildRemediation()] | |
| const report = generateReport(remediations, { format: 'json', groupBy: 'bundle' }) | |
| const parsed = JSON.parse(report) | |
| expect(parsed).to.deep.equal(remediations) | |
| }) | |
| }) |
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — this proposes adding option-precedence tests. CONVENTIONS.md requires 82% line coverage (met at 98.93%) but has no rules about combinatorial option testing or precedence coverage. Valid test coverage improvement but not convention-backed. No sub-task created.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
==========================================
+ Coverage 90.95% 91.22% +0.27%
==========================================
Files 41 42 +1
Lines 8984 9175 +191
Branches 1573 1624 +51
==========================================
+ Hits 8171 8370 +199
+ Misses 813 805 -8
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 review — Classified as suggestion — the review body summarizes the same 2 issues raised as inline comments (loop-invariant hoisting and option-precedence tests). Both are valid code quality suggestions but neither matches a CONVENTIONS.md rule or established codebase pattern. No sub-task created. |
Collaborator
Author
Verification Report for TC-5413 (commit 431fc33)
Overall: WARNThe only non-PASS finding is the CVSS score column mentioned in Acceptance Criterion 1 but absent from the implementation. This is a spec inconsistency, not a code defect — the upstream remediation extractor (TC-5410) does not extract CVSS data, so the report module has no CVSS values to render. The task description's template included a CVSS column, but the input data shape doesn't support it. Recommend updating the Jira task's AC1 to remove the CVSS reference, or adding CVSS extraction to the upstream extractor as a follow-up. This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
Add report generator that transforms remediation extractor output into structured markdown for PR bodies, CLI dry-run output, and JSON format. Includes per-dependency and bundled report modes, provider source attribution, and a deterministic deduplication key generator for PR branch/title matching. Implements TC-5413 Assisted-by: Claude Code
Address review feedback from sourcery-ai: move formatAdvisoryLinks() call above the CVE loop since it depends only on rem.advisories (same value every iteration), and add tests documenting that JSON format takes precedence over dryRun and groupBy options. Implements TC-5413 Assisted-by: Claude Code
a-oren
requested changes
Aug 4, 2026
a-oren
left a comment
a-oren
left a comment
Contributor
There was a problem hiding this comment.
src/index.js re-exports extractRemediations from remediation.js as part of the public API surface. The new generateReport and generateDeduplicationKey are not re-exported. Every other public function in this library is accessible through the index, this omission breaks that convention and forces consumers to import from a deep path.
| * markdown for PR bodies, CLI dry-run output, and JSON. | ||
| */ | ||
|
|
||
| const SEVERITY_ORDER = ['UNKNOWN', 'LOW', 'MEDIUM', 'HIGH', 'CRITICAL'] |
Contributor
There was a problem hiding this comment.
src/remediation_report.js:6 defines SEVERITY_ORDER identically to src/remediation.js. This is a DRY violation, the constant should be imported from remediation.js (or extracted to a shared module) rather than duplicated.
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — SEVERITY_ORDER is defined identically in both src/remediation.js:335 and src/remediation_report.js:6. Sub-task TC-5513 created to address this feedback.
| '', | ||
| ] | ||
|
|
||
| if (rem.cves.length > 0) { |
Contributor
There was a problem hiding this comment.
will throw a TypeError if cves is undefined or null. Same issue in generateBundledReport (line 102) with rem.cves.join(', ')
Suggested change
| if (rem.cves.length > 0) { | |
| if (rem.cves && rem.cves.length > 0) { |
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — rem.cves access without null guard will throw TypeError if cves is undefined/null. Same issue in generateBundledReport at line 102. Sub-task TC-5514 created to address this feedback.
Collaborator
Author
Collaborator
Author
Verification Report for TC-5413 (commit 4bdfeba)
Overall: WARNReview feedback (3 code change requests from a-oren):
Acceptance Criteria: CVSS score column mentioned in AC1 but absent — upstream extractor (TC-5410) doesn't extract CVSS data. Spec inconsistency, not code defect. Root-cause investigation: Two systemic improvements identified:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
Address review feedback: add generateReport and generateDeduplicationKey to the barrel export in src/index.js, matching the convention that all public functions are accessible through the package entry point. Implements TC-5515 Assisted-by: Claude Code
Address review feedback: deduplicate the SEVERITY_ORDER constant by exporting it from remediation.js and importing it in remediation_report.js. Implements TC-5513 Assisted-by: Claude Code
Address review feedback: add defensive null checks before accessing rem.cves.length in generatePerDependencyReport and rem.cves.join() in generateBundledReport to prevent TypeError when cves is null or undefined. Add 4 test cases covering both report modes. Implements TC-5514 Assisted-by: Claude Code
Collaborator
Author
Verification Report for TC-5413 (commit f26910c)
Overall: WARNReview feedback resolved: All 3 code change requests from a-oren (SEVERITY_ORDER DRY violation, null guard for rem.cves, missing barrel exports) have been addressed in fix commits b943125, f26910c, and 6e72189 respectively. Remaining WARNs:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
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
src/remediation_report.jswithgenerateReport()andgenerateDeduplicationKey()exportsTest plan
Implements TC-5413
Summary by Sourcery
Add a remediation report generator for security dependency updates with support for multiple output formats and a deterministic deduplication key.
New Features:
Tests: