feat: add --preview flag to preview aviator remediations before applying by dhanwanthp · Pull Request #1076 · fortify/fcli

dhanwanthp · 2026-08-20T11:18:46Z

This MR adds a new --preview flag to the fcli aviator ssc apply-remediations and fcli fod aviator apply-remediations commands, enabling users to preview what changes would be applied to their source code without actually modifying files.

Features:

Added --preview option to both SSC and FoD apply-remediations commands
When enabled, the tool performs full validation and processing without modifying source files
Returns detailed JSON output containing:
- All proposed code changes per issue ID
- File paths and encodings
- Line numbers and code snippets (before/after)
- Context metadata for fuzzy matching
- Skip reasons for any failed remediations

Usage:

# Preview changes without applying them
fcli aviator ssc apply-remediations --appversion myapp --preview

# Preview specific issues only
fcli aviator ssc apply-remediations --appversion myapp --preview --issue-ids ISSUE-123,ISSUE-456

# Apply changes (existing behavior, no --preview flag)
fcli aviator ssc apply-remediations --appversion myapp

Output format:-

{
  "previewMode": true,
  "totalRemediation": 10,
  "appliedRemediation": 8,
  "previewDetails": [
    {
      "issueId": "ISSUE-123",
      "status": "available",
      "files": {
        "src/Example.java": {
          "filename": "src/Example.java",
          "path": "src/Example.java",
          "encoding": "UTF-8",
          "changes": [
            {
              "changeIndex": 1,
              "lineFrom": 42,
              "lineTo": 44,
              "originalCode": "...",
              "newCode": "...",
              "context": { "linesBefore": 2, "linesAfter": 2, "content": "..." },
              "fuzzyMatched": false
            }
          ]
        }
      }
    }
  ]
}

…and to display the correct value for __action__

…rics for better readability, removed a redundant field and added tests for code that was not covered before.

rsenden · 2026-08-24T11:45:12Z

+     * Applies or previews remediations for each source entry until done or the issue-id filter is exhausted.
+     * Caller owns {@code source} lifecycle (try-with-resources).
+     */
+    public static ApplyResult apply(


As the number of parameters keep growing, maybe better to combine in a single object that implements builder pattern (through Lombok @Builder), i.e.,

apply(ApplyArgs.builder() .source(source) .sourceCodeDirectory(sourceCodeDirectory) ... .build();

Same could potentially be applied to other methods in Aviator code to reduce number of parameters.

rsenden · 2026-08-24T11:59:20Z

                unirest, logger, progressWriter, resolved.artifacts())) {
            ApplyResult applyResult = RemediationsApplyHelper.apply(
-                    source, sourceCodeDirectory, logger, issueIdFilter, LOG);
+                    source, sourceCodeDirectory, logger, issueIdFilter, LOG, previewMode);


Related to the apply comment elsewhere, what about:

Move the various options that define method parameter values into a separate Picocli ArgGroup that can be shared between SSC and FoD commands

Have that ArgGroup implement some interface with getters for each of the option values (through Lombok @Getter annotations on the options/class)

Have the RemediationsApplyHelper::apply method take an instance of that interface

This way, the FoD & SSC apply-remediations commands can simply pass the ArgGroup to the apply method (together with the loggers, which remain separate method parameters), and due to interface abstraction, RemediationsApplyHelper doesn't have a compile-time dependency on CLI-specific code (like the ArgGroup).

rsenden · 2026-08-24T12:02:01Z

                unirest, logger, progressWriter, resolved.artifacts())) {
            ApplyResult applyResult = RemediationsApplyHelper.apply(
-                    source, sourceCodeDirectory, logger, issueIdFilter, LOG);
+                    source, sourceCodeDirectory, logger, issueIdFilter, LOG, previewMode);


Why are you passing LOG to a different class? Usually, each class has its own Slf4j logger, which also makes it much easier to identify which class generated a particular log message.

rsenden · 2026-08-24T12:03:33Z

                unirest, logger, progressWriter, resolved.artifacts())) {
            ApplyResult applyResult = RemediationsApplyHelper.apply(
-                    source, sourceCodeDirectory, logger, issueIdFilter, LOG);
+                    source, sourceCodeDirectory, logger, issueIdFilter, LOG, previewMode);


I think there's some overlap between FoD and SSC implementations of the apply-remediations command; please check whether it makes sense to introduce a common AbstractAviatorApplyRemediationsCommand base class.

@slf4j

- Introduce AbstractAviatorApplyRemediationsCommand base class shared by SSC and FoD - Add ApplyRemediationsOptionsMixin and IApplyRemediationsOptions interface - Simplify RemediationsApplyHelper.apply() signature; add @slf4j logger instead of passing LOG - Add @builder to FileChange; replace IllegalArgumentException with AviatorBugException

rsenden · 2026-08-25T23:26:31Z

+    @Override
+    public final JsonNode getJsonNode() {
+        validateSourceSelector();
+        AviatorApplyRemediationsCliSupport.requireSourceDir(applyOptions.getSourceCodeDirectory());


Exceptions thrown by AviatorApplyRemediationsCliSupport reference explicit option names, whereas that class doesn't 'know' from which command class it's being invoked, and what options are provided by that command. The requiresSourceDir method can be easily moved to this abstract base class, as this base class declares the option for specifying source code directory. For requireIssueIdsCacheOnly, it's more difficult as this abstract base class doesn't declare the --from-cache option. I guess --from-cache is currently declared through picocli arg group as being exclusive to product-specific options, hence we can't move --from-cache to ApplyRemediationsOptionsMixin without loosing the picocli exclusivity. Given that this option is shared between SSC & FoD, maybe it's still worth declaring this option on this abstract base class, and manually check for exclusivity.

Effectively, I think it would be good to get rid of the AviatorApplyRemediationsCliSupport class, moving functionality to this abstract base class.

rsenden · 2026-08-25T23:34:36Z

+
+    protected abstract boolean isFromCacheSelected();
+
+    protected abstract JsonNode processFromCache(AviatorLoggerImpl logger, Set<String> issueIdFilter);


I haven't checked the details, but wouldn't applying from cache be very similar between SSC & FoD? Or, possibly even better, can't we get rid of separate methods for cache vs online, and instead have something like an IFpr[s]Supplier interface that, based on given options, provides FPR files from either cache, SSC, or FoD, with actual FPR processing logic shared between all three use cases?

rsenden · 2026-08-25T23:37:41Z

@@ -40,7 +40,18 @@ private AviatorRemediationMetricsHelper() {}
    public static RemediationMetric aggregateMetrics(Set<String> requestedIssueIds, Collection<RemediationMetric> metrics) {


This seems a fairly long method; can we improve this through self-describing sub-methods, and/or through utility (builder) methods on RemediationMetric?

rsenden · 2026-08-25T23:41:17Z

+ * @param fuzzyMatched Whether fuzzy matching was used
+ */
+@Reflectable
+public record ChangeDetail(


Given the many record constructor arguments, can we use Lombok @Builder to improve code readability and avoiding constructor arguments potentially being passed in wrong order?

@builder

- Remove AviatorApplyRemediationsCliSupport class and moved its functionality to AbstractAviatorApplyRemediationsCommand - Replace separate methods for cache vs online with openFprSource/buildResultNode/isCacheMode to leverage IRemediationsFprSource - Split aggregateMetrics into aggregateUnfiltered/aggregateFiltered sub-methods - Use Lombok @builder in ChangeDetail to improve code readability

…ng violations

rsenden · 2026-08-27T09:06:18Z

+    public final boolean isSingular() { return true; }
+
+    @Override
+    public JsonNode transformRecord(JsonNode record) { return record; }


As this is a no-op, better to remove this method and the corresponding IRecordTransformer interface from the class definition.

rsenden · 2026-08-27T09:20:38Z

+    @Override
+    public JsonNode transformRecord(JsonNode record) { return record; }
+
+    private void requireSourceDir() {


Given the default value for --source-dir, I don't think the null check is necessary? Or better yet, just use StringUtils::isBlank, which handles both null and blank values. Shouldn't we also validate that the path exists/is accessible? Given that exception explicitly mentions option name, it would be better to move this validation to the mixin that declares this option. This could be done either by an explicit validation method on the mixin, or directly in getSourceCodeDirectory (i.e., checks are done when value is retrieved, although pre-flight validation may be better).

rsenden · 2026-08-27T09:30:15Z

+                "--source-dir must specify a valid directory path");
+    }
+
+    private void requireIssueIdsCacheOnly() {


Similar to the above; as exception references option names, better to move this validation to the mixin.

rsenden · 2026-08-27T10:38:52Z

-    @Override
-    public JsonNode transformRecord(JsonNode record) {
-        return record;
+        return AviatorFoDApplyRemediationsHelper.buildOnlineResultNode(resolvedRelease, result);


Why is this done through IRecordTransformer instead of directly in the buildResultNode method?

rsenden · 2026-08-27T10:47:58Z

+        Set<String> issueIdFilter = AviatorIssueIdFilterUtils.normalizeIssueIds(applyOptions.getIssueIds());
+        try (IProgressWriter progressWriter = progressWriterFactoryMixin.create()) {
+            AviatorLoggerImpl logger = new AviatorLoggerImpl(progressWriter);
+            try (IRemediationsFprSource source = openFprSource(logger, progressWriter)) {


source can be easily mistaken for 'source file/dir'; can we consistently rename this variable to fprSource (also in for example RemediationsApplyHelper if applicable)

rsenden · 2026-08-27T11:36:01Z

+import lombok.Getter;
+import picocli.CommandLine.Mixin;
+
+public abstract class AbstractAviatorApplyRemediationsCommand extends AbstractOutputCommand


Although much better than before, I think definitions and responsibilities between abstract base class, concrete sub-classes, and options/mixins can be further improved.

Maybe something like the following?

Rename ApplyRemediationsOptionsMixin to AbstractApplyRemediationsOptionsMixin

Add validate method to this abstract mixin and its interface, calling individual validation methods for validating source selection, source dir, --issue-ids/--from-cache interdependency, ...

Have both SSC & FoD remediation mixins extend the abstract mixin

Declare remediation mixin in concrete FoD/SSC command class, accessible to abstract command class through getter

So, basically, generic & command-specific options are provided through single mixin on FoD/SSC command classes, and those mixins provide the necessary validation logic. This allows the abstract base class to focus on process, instead of handling both process and data (validation).

rsenden · 2026-08-27T11:39:02Z

-    private String sourceCodeDirectory = System.getProperty("user.dir");
-    @Option(names = {"--issue-ids"}, split = ",")
-    private List<String> issueIds;
+    private ResolvedOnlineArtifacts resolvedOnline;


As per fcli convention, command classes ideally shouldn't store state in instance fields

Dhanwanth Pratheep added 4 commits August 20, 2026 15:53

feat: add --preview flag to preview aviator remediations before applying

3864b09

fix: updated the output of --preview flag for apply-remediations comm…

205ecde

…and to display the correct value for __action__

fix: Changed preview to a real type, reordered some of the output met…

7bcba4a

…rics for better readability, removed a redundant field and added tests for code that was not covered before.

fix: align FoD --preview option with repository convention

7333299

dhanwanthp marked this pull request as ready for review August 24, 2026 08:21

dhanwanthp requested a review from rsenden August 24, 2026 08:23

rsenden reviewed Aug 24, 2026

View reviewed changes

dhanwanthp requested a review from rsenden August 25, 2026 10:32

rsenden requested changes Aug 25, 2026

View reviewed changes

Dhanwanth Pratheep added 2 commits August 27, 2026 11:54

fix: resolve build issue flagged by spotlessJavaCheck due to formatti…

85bc159

…ng violations

dhanwanthp requested a review from rsenden August 27, 2026 07:48

rsenden requested changes Aug 27, 2026

View reviewed changes


		protected abstract boolean isFromCacheSelected();

		protected abstract JsonNode processFromCache(AviatorLoggerImpl logger, Set<String> issueIdFilter);

		@@ -40,7 +40,18 @@ private AviatorRemediationMetricsHelper() {}
		public static RemediationMetric aggregateMetrics(Set<String> requestedIssueIds, Collection<RemediationMetric> metrics) {

Uh oh!

Conversation

dhanwanthp commented Aug 20, 2026

Features:

Usage:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants