feat(cli): add remediate command with dry-run and apply modes - #604
Open
ruromero wants to merge 7 commits into
Open
feat(cli): add remediate command with dry-run and apply modes#604ruromero wants to merge 7 commits into
ruromero wants to merge 7 commits into
Conversation
Add the `remediate` CLI command that orchestrates the full remediation pipeline: discover manifests → scan via DA backend → extract remediations with priority resolution → apply version updates to manifest files or show dry-run preview. Supports pom.xml (Maven) and libs.versions.toml (Gradle) manifests, directory-mode recursive discovery, provider/source filtering, and bundled/per-dependency report grouping. Exit codes: 0 = success, 1 = error, 2 = dry-run with remediations found. Implements TC-5414 Assisted-by: Claude Code
Reviewer's GuideAdds a new Sequence diagram for the new remediate CLI remediation pipelinesequenceDiagram
actor User
participant CLI as cli_remediate_handler
participant Remediation as runRemediation
participant Env as getCustom
participant Provider as match
participant Analysis as analysis.requestStack
participant Extract as extractRemediations
participant Report as generateReport
participant MavenUpdater as updateMavenVersions
participant TomlUpdater as updateTomlVersions
participant Backend as Trustify_DA_backend
User->>CLI: remediate path --dry-run/--apply
CLI->>Remediation: runRemediation(path, options)
Remediation->>Env: getCustom(TRUSTIFY_DA_BACKEND_URL, null, opts)
Env-->>Remediation: backendUrl
Remediation->>Provider: match(manifestPath, availableProviders, opts)
Provider-->>Remediation: provider
Remediation->>Analysis: analysis.requestStack(provider, manifestPath, backendUrl, false, opts)
Analysis->>Backend: requestStack
Backend-->>Analysis: analysisReport
Analysis-->>Remediation: analysisReport
Remediation->>Extract: extractRemediations(analysisReport, options)
Extract-->>Remediation: remediations
alt apply mode and maven manifest
Remediation->>MavenUpdater: updateMavenVersions(content, versionChanges)
MavenUpdater-->>Remediation: { applied, content }
Remediation->>FS: writeFileSync(manifestPath, content)
else apply mode and toml manifest
Remediation->>TomlUpdater: updateTomlVersions(content, versionChanges)
TomlUpdater-->>Remediation: { applied, content }
Remediation->>FS: writeFileSync(manifestPath, content)
end
Remediation->>Report: generateReport(allRemediations, { groupBy, dryRun })
Report-->>Remediation: report
Remediation-->>CLI: { exitCode, output }
CLI->>Console: console.log(output)
CLI->>Process: process.exit(exitCode)
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 3 issues, and left some high level feedback:
- Consider validating the CLI flags so
--dry-runand--applycannot be used together (or have a clearly defined precedence), e.g., via yargsconflicts/impliesor an explicit check inrunRemediation, to avoid ambiguous behavior. - In
runRemediation,match()failures for specific manifests are silently swallowed; it may be more user-friendly to surface which manifests were skipped (e.g., collecting and reporting them in the final output) so users understand why some files weren’t remediated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating the CLI flags so `--dry-run` and `--apply` cannot be used together (or have a clearly defined precedence), e.g., via yargs `conflicts`/`implies` or an explicit check in `runRemediation`, to avoid ambiguous behavior.
- In `runRemediation`, `match()` failures for specific manifests are silently swallowed; it may be more user-friendly to surface which manifests were skipped (e.g., collecting and reporting them in the final output) so users understand why some files weren’t remediated.
## Individual Comments
### Comment 1
<location path="src/cli.js" line_range="482" />
<code_context>
+ normalize: true,
+ }
+ ).options({
+ 'dry-run': {
+ alias: 'd',
+ type: 'boolean',
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify interaction between --dry-run and --apply to avoid surprising destructive changes.
If a user passes both `--dry-run` and `--apply`, the command still performs writes because `apply` is handled independently of `dryRun`. This can violate the expectation that `--dry-run` is always non-destructive. Please either make the flags mutually exclusive (e.g., `yargs.conflicts('dry-run', 'apply')`) or ensure `dryRun` takes precedence in `runRemediation` so no writes happen when `dryRun` is true.
</issue_to_address>
### Comment 2
<location path="src/remediate.js" line_range="138-141" />
<code_context>
+ continue
+ }
+
+ const analysisReport = await analysis.requestStack(provider, manifestPath, url, false, opts)
+ const remediations = extractRemediations(analysisReport, {
+ providerPriority: providers ? providers.split(',') : undefined,
+ })
+
</code_context>
<issue_to_address>
**suggestion:** Normalize provider list values to avoid issues with whitespace in comma-separated input.
`providers.split(',')` will return values with leading whitespace (e.g., `['redhat', ' lightwell']` for `--providers 'redhat, lightwell'`), which can break matching if exact IDs are expected. Consider normalizing with trimming and filtering, e.g. `providers.split(',').map(p => p.trim()).filter(Boolean)`.
```suggestion
const analysisReport = await analysis.requestStack(provider, manifestPath, url, false, opts)
const providerPriority = providers
? providers
.split(',')
.map(p => p.trim())
.filter(Boolean)
: undefined
const remediations = extractRemediations(analysisReport, {
providerPriority,
})
```
</issue_to_address>
### Comment 3
<location path="test/remediate.test.js" line_range="318-320" />
<code_context>
+ })
+ })
+
+ suite('error handling', () => {
+ /** Verifies that unsupported manifest types throw an error. */
+ test('throws for unsupported manifest type', async () => {
+ const { dir, cleanup } = createTempDir({ 'requirements.txt': 'flask==2.0' })
+ try {
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the case where provider matching fails and manifests are silently skipped
In `runRemediation`, errors from `match(manifestPath, availableProviders, opts)` are caught and the manifest is silently skipped. Current tests only cover unsupported manifest types (e.g. requirements.txt). Please add a test that stubs `match` to throw for a supported manifest (e.g. pom.xml) and asserts that `runRemediation` completes, reports no remediations, and leaves the file unchanged. This will lock in the intended catch-and-skip behavior.
Suggested implementation:
```javascript
suite('error handling', () => {
/** Verifies that provider matching errors are caught and manifests are skipped. */
test('skips manifests when provider matching fails', async () => {
const originalPom = '<project><modelVersion>4.0.0</modelVersion></project>'
const { dir, cleanup } = createTempDir({ 'pom.xml': originalPom })
try {
const pomPath = path.join(dir, 'pom.xml')
// Arrange: make match throw for this supported manifest
matchStub.throws(new Error('test provider matching failure'))
// Act: run remediation
const result = await runRemediation(pomPath, { dryRun: true })
// Assert: runRemediation completes, reports no remediations, and leaves file unchanged
expect(result.exitCode).to.equal(0)
expect(result.output.toLowerCase()).to.include('no remediations')
expect(readFileSync(pomPath, 'utf8')).to.equal(originalPom)
} finally {
cleanup()
}
})
```
To wire this up fully, you may also need to:
1. Ensure `readFileSync` is imported at the top of `test/remediate.test.js` if it is not already:
- For example: `const { readFileSync } = require('fs')`.
2. Confirm that `matchStub` is initialized in the same scope as other tests (likely in a `beforeEach` using `sinon.stub(...)`), so this new test can reuse it.
3. If the "no remediations" message differs in your CLI output, adjust the `expect(result.output.toLowerCase()).to.include('no remediations')` assertion to match the exact wording used elsewhere in the tests for the "no remediations" scenario.
</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
+318
to
+320
| suite('error handling', () => { | ||
| /** Verifies that unsupported manifest types throw an error. */ | ||
| test('throws for unsupported manifest type', async () => { |
There was a problem hiding this comment.
suggestion (testing): Add coverage for the case where provider matching fails and manifests are silently skipped
In runRemediation, errors from match(manifestPath, availableProviders, opts) are caught and the manifest is silently skipped. Current tests only cover unsupported manifest types (e.g. requirements.txt). Please add a test that stubs match to throw for a supported manifest (e.g. pom.xml) and asserts that runRemediation completes, reports no remediations, and leaves the file unchanged. This will lock in the intended catch-and-skip behavior.
Suggested implementation:
suite('error handling', () => {
/** Verifies that provider matching errors are caught and manifests are skipped. */
test('skips manifests when provider matching fails', async () => {
const originalPom = '<project><modelVersion>4.0.0</modelVersion></project>'
const { dir, cleanup } = createTempDir({ 'pom.xml': originalPom })
try {
const pomPath = path.join(dir, 'pom.xml')
// Arrange: make match throw for this supported manifest
matchStub.throws(new Error('test provider matching failure'))
// Act: run remediation
const result = await runRemediation(pomPath, { dryRun: true })
// Assert: runRemediation completes, reports no remediations, and leaves file unchanged
expect(result.exitCode).to.equal(0)
expect(result.output.toLowerCase()).to.include('no remediations')
expect(readFileSync(pomPath, 'utf8')).to.equal(originalPom)
} finally {
cleanup()
}
})To wire this up fully, you may also need to:
- Ensure
readFileSyncis imported at the top oftest/remediate.test.jsif it is not already:- For example:
const { readFileSync } = require('fs').
- For example:
- Confirm that
matchStubis initialized in the same scope as other tests (likely in abeforeEachusingsinon.stub(...)), so this new test can reuse it. - If the "no remediations" message differs in your CLI output, adjust the
expect(result.output.toLowerCase()).to.include('no remediations')assertion to match the exact wording used elsewhere in the tests for the "no remediations" scenario.
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — proposes additional test coverage for provider matching failure skip behavior. No documented convention or established codebase pattern requires this specific test. No sub-task created.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #604 +/- ##
==========================================
+ Coverage 91.22% 91.33% +0.10%
==========================================
Files 42 43 +1
Lines 9175 9471 +296
Branches 1624 1695 +71
==========================================
+ Hits 8370 8650 +280
- Misses 805 821 +16
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 —
|
Collaborator
Author
Verification Report for TC-5414 (commit 464608d)
Overall: WARNTwo review feedback items require code changes before merge:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
…ovider list - Add yargs conflicts declaration between --dry-run and --apply flags, matching the existing pattern used by image/stack/stack-batch commands - Guard the apply block with `if (apply && !dryRun)` as defense-in-depth - Normalize provider list with .trim().filter(Boolean) to handle whitespace in comma-separated input, matching workspace.js and java_maven.js patterns Implements TC-5536, TC-5537 Assisted-by: Claude Code
Yargs conflicts declaration does not work with boolean options that have default:false — yargs treats both as "present" since they always exist in argv. Removing the defaults matches the sibling pattern (html/summary options have no default). Fixes TC-5536 Assisted-by: Claude Code
…tent The DA backend returns fixedIn as an array of version strings (e.g., ['3.8.6.1']), not as a PURL. The extractor was silently skipping these because PackageURL.fromString() failed on the array. Now constructs a PURL from the dependency ref when fixedIn is an array. Also adds trustedContent preference: when both trustedContent and fixedIn exist for the same dependency, the trustedContent version is preserved during merge rather than being overridden by a higher fixedIn version. This ensures Red Hat patches (e.g., 2.17.1.redhat-00002) are preferred over major version jumps (e.g., 3.0.0). Fixes TC-5414 Assisted-by: Claude Code
a-oren
requested changes
Aug 6, 2026
| * @returns {string} | ||
| * @throws {Error} if TRUSTIFY_DA_BACKEND_URL is unset | ||
| */ | ||
| function resolveBackendUrl(opts) { |
Contributor
There was a problem hiding this comment.
Other commands in cli.js (like license) call selectTrustifyDABackend(opts) for URL resolution. The remediate handler delegates to runRemediation(), which calls resolveBackendUrl() using getCustom('TRUSTIFY_DA_BACKEND_URL', ...). These are different codepaths. It's a consistency gap. The fix would be to import and call selectTrustifyDABackend(opts) instead of the custom resolveBackendUrl(), which would also eliminate the duplicated code.
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5557 created to address this feedback.
Comment on lines
+496
to
+500
| describe: 'Comma-separated provider list (e.g., redhat,lightwell)', | ||
| }, | ||
| sources: { | ||
| type: 'string', | ||
| describe: 'Comma-separated source list', |
Contributor
There was a problem hiding this comment.
Based on the pattern from other commands like component, stack, image, and stack-batch, the descriptions should be:
- providers: 'Comma-separated list of vulnerability providers (env: TRUSTIFY_DA_PROVIDERS)'
- sources: 'Comma-separated list of vulnerability sources (env: TRUSTIFY_DA_SOURCES)'
The PR currently has:
- providers: 'Comma-separated provider list (e.g., redhat,lightwell)'
- sources: 'Comma-separated source list'
So both TRUSTIFY_DA_PROVIDERS and TRUSTIFY_DA_SOURCES env var references are missing, and the wording style differs from every other command.
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5558 created to address this feedback.
| * @param {string} dirPath - absolute path to the directory | ||
| * @returns {string[]} array of absolute paths to supported manifest files | ||
| */ | ||
| function discoverManifests(dirPath) { |
Contributor
There was a problem hiding this comment.
Other common directories that should likely be skipped: target/, build/, .gradle/, .mvn/, vendor/.
A pom.xml inside target/ is a generated artifact, not a source manifest.
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — proposes additional directory exclusions (target/, build/, .gradle/, .mvn/, vendor/). Valid improvement but no documented convention or established pattern requires these specific exclusions. No sub-task created.
| return fixedIn | ||
| } | ||
| if (Array.isArray(fixedIn) && fixedIn.length > 0) { | ||
| const version = fixedIn[0] |
Contributor
There was a problem hiding this comment.
When fixedIn is an array, only the first element is used (fixedIn[0]). If the array contains multiple fix versions, the rest are silently ignored.
Should we pick the highest version instead of the first, consistent with how processIssueRemediation already resolves conflicts via compareVersions()?
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — this concern is already addressed by commit 0171316 (TC-5555) which adds pluggable version selection strategies. The default closestCoverageStrategy picks the best version from the fixedIn array based on major version proximity, not just fixedIn[0]. No sub-task created.
| const resolvedPath = path.resolve(targetPath) | ||
|
|
||
| let manifestPaths | ||
| const stat = fs.statSync(resolvedPath) |
Contributor
There was a problem hiding this comment.
If resolvedPath doesn't exist, this throws a raw Node.js error, so the user sees that raw ENOENT message instead of something friendlier
Suggested change
| const stat = fs.statSync(resolvedPath) | |
| let stat | |
| try { | |
| stat = fs.statSync(resolvedPath) | |
| } catch { | |
| throw new Error(`Path not found: ${resolvedPath}`) | |
| } |
Collaborator
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5559 created to address this feedback.
Add a strategy interface with two methods (selectVersion, resolveConflict) to extractRemediations. Ship two built-in strategies: - closestCoverageStrategy (default): prefers same-major-version patches over cross-major jumps. For quarkus-resteasy@2.13.5.Final, now recommends 2.13.9.Final-redhat-00003 instead of 3.8.6.1. - highestStrategy: original behavior, always picks highest version for maximum CVE coverage. API consumers can override via options.versionStrategy. Implements TC-5555 Assisted-by: Claude Code
Stop passing dryRun to generateReport so both modes show the full per-dependency security report instead of a simplified table for dry-run. Implements TC-5414 Assisted-by: Claude Code
Collaborator
Author
Verification Report for TC-5414 (commit 39294df)
Overall: WARNThree new code change requests from a-oren requiring fixes:
Two suggestions acknowledged without sub-tasks:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
- Use selectTrustifyDABackend() instead of custom resolveBackendUrl() to match other CLI commands (TC-5557) - Align --providers/--sources option descriptions with other commands, include env var references (TC-5558) - Add friendly error message for non-existent path instead of raw ENOENT (TC-5559) Implements TC-5557, TC-5558, TC-5559 Assisted-by: Claude Code
Collaborator
Author
|
FYI I have tested it locally and this is an example output with |
Strum355
reviewed
Aug 7, 2026
Comment on lines
+488
to
+493
| apply: { | ||
| alias: 'a', | ||
| type: 'boolean', | ||
| desc: 'Apply changes to manifest files', | ||
| conflicts: 'dry-run', | ||
| }, |
Member
There was a problem hiding this comment.
What happens if neither apply nor dry-run are passed? Feels like apply should be the default
| * @param {string} dirPath - absolute path to the directory | ||
| * @returns {string[]} array of absolute paths to supported manifest files | ||
| */ | ||
| function discoverManifests(dirPath) { |
Member
There was a problem hiding this comment.
I havent looked at existing implementations too much, so maybe theres no overlap, but it feels like we're slowly amassing a number of functions that do somewhat similar things (workspace/manifest discovery), is there anything we could re-use instead of having this function or are all the implementations different enough that they warrant their own functions?
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
4 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
remediateCLI command orchestrating the full remediation pipeline: manifest discovery → DA backend scan → remediation extraction → version updates or dry-run preview--dry-runand--applymutually exclusive via yargsconflicts+ defense-in-depth guard (TC-5536).trim().filter(Boolean)to handle whitespace in comma-separated input (TC-5537)closestCoverageStrategy(default — prefers same-major patches) andhighestStrategy(original behavior) (TC-5555)Implements TC-5414
Test plan
closestCoverageStrategypicks2.13.9.Final-redhat-00003over3.8.6.1for quarkus-resteasyhighestStrategypicks3.8.6.1for the same scenario🤖 Generated with Claude Code