feat(aviator): Improve apply-remediations UX with --latest, --all-ope…#948
Conversation
…n-issues, and --since options This enhancement provides a more flexible and user-friendly experience for applying Aviator auto-remediations by introducing multiple artifact selection modes: - --latest: Automatically select the most recent Aviator-processed artifact - --all-open-issues: Process all artifacts with open issues in bulk - --since: Filter artifacts by upload date (relative: 7d, 2w, 1M; absolute: 2025-01-01) Key Changes: - Replaced required --artifact-id with flexible selection modes - Added SinceOptionHelper for robust date/period parsing - Enhanced SSCArtifactHelper with getLatestAviatorArtifact() and getAllAviatorArtifacts() - Improved command validation with mutual exclusivity checks - Added comprehensive unit tests for all new options - Updated i18n messages with detailed usage descriptions Technical Details: - SinceOptionHelper supports relative periods (d, w, M, y) and absolute ISO-8601 dates - DateTimePeriodHelper integration for consistent period parsing across fcli - Proper UTC timezone handling for date comparisons - Backward compatible - existing --artifact-id usage unchanged Closes: #XXX
| AviatorEntitlementCommands.class, | ||
| AviatorSSCCommands.class, | ||
| // AviatorFoDCommands.class, | ||
| AviatorFoDCommands.class, |
There was a problem hiding this comment.
There's some discussion as to whether this should live under fcli aviator or fcli fod. Given that Aviator-related functionality is handled by FoD in the background, none of the other fcli aviator commands is relevant for FoD (like user/admin session management, app & entitlement managent, ...). So, unless we plan on supporting any of this functionality also for FoD, moving this to fcli fod might make more sense.
There was a problem hiding this comment.
I'd lean towards keeping this under fcli aviator fod and disagree with moving it to fcli fod, for the following reasons:
-
Feature ownership is Aviator, not FoD. The command's primary purpose is applying AI-generated code remediations to source files using Aviator's engine (ApplyAutoRemediationOnSource). FoD's only role is providing the FPR file download — it's the data source, not the product context. The same logic would apply to fcli aviator ssc apply-remediations (it also just downloads an FPR from SSC), and we wouldn't move that to fcli ssc.
-
Symmetry and discoverability. Having fcli aviator ssc apply-remediations and fcli aviator fod apply-remediations as parallel commands gives users a consistent mental model — regardless of which product stores their scans, the Aviator remediation experience lives in the same place. A developer thinking "I want Aviator to fix my code" will look under fcli aviator, not fcli fod.
-
It's true that most fcli aviator commands (user/admin session, app, entitlement management) are SSC-specific and not applicable to FoD. But apply-remediations is precisely the one Aviator feature that IS relevant to FoD users — it's the core end-user-facing capability. That distinction is a reason to grow fcli aviator fod further, not to move this command out.
Happy to discuss further if there's a strong reason for the alternative, but the current placement feels architecturally correct.
| } | ||
|
|
||
|
|
||
| private void validateOptions() { |
There was a problem hiding this comment.
Please use Picocli ArgGroups for handling exclusivity and related options, either as inner classes in this class, or as a separate single class (with sub-arggroups as inner classes) in the cli.mixin package.
There was a problem hiding this comment.
Done! Created a separate AviatorSSCApplyRemediationsArtifactSelectorMixin class in the cli.mixin package with ArtifactSelectionArgGroup as an inner class annotated with @Arggroup(exclusive = true, multiplicity = "1"), making --artifact-id, --latest, and --all-open-issues mutually exclusive via Picocli.
One constraint worth noting: Picocli does not support @mixin inside @Arggroup classes (InitializationException: @Mixins are not supported on @ArgGroups), and placing the same option name (e.g. --appversion) in multiple exclusive sub-groups raises a DuplicateOptionAnnotationsException. To work around this, --av/--appversion and --delim are declared at the top level of the mixin (outside the ArgGroup) with a validate() method enforcing that --av is required with --latest/--all-open-issues and forbidden with --artifact-id. All other exclusivity constraints are fully handled declaratively by Picocli.
| AviatorEntitlementCommands.class, | ||
| AviatorSSCCommands.class, | ||
| // AviatorFoDCommands.class, | ||
| AviatorFoDCommands.class, |
There was a problem hiding this comment.
As discussed with Frans, since none of the other fcli aviator commands make sense for FoD (Aviator user/admin session, app & entitlement management), we'll want to move this to the FoD module, i.e., have fcli fod aviator apply-remediations. This likely requires adding a dependency from fcli-fod module on fcli-aviator-common module, and possibly some refactoring (moving code from fcli-aviator to fcli-aviator-common if FoD Aviator commands depend on any code in fcli-aviator module.
| } | ||
|
|
||
|
|
||
| private void validateOptions() { |
There was a problem hiding this comment.
Please use Picocli ArgGroups to model exclusivity and related options, to have Picocli take care of validation and provide consistent error messaging across fcli. The ArgGroups could be either inner classes in this command class, or potentially a single top-level class (with inner classes for sub-arggroups) in the cli.mixin package.
| Path fprPath = Files.createTempFile("aviator_" + ad.getId() + "_", ".fpr"); | ||
| try (IProgressWriter progressWriter = progressWriterFactoryMixin.create()){ | ||
| logger.progress("Status: Downloading Audited FPR from SSC"); | ||
| try (IProgressWriter progressWriter = progressWriterFactoryMixin.create()) { |
There was a problem hiding this comment.
There's already a progress writer instantiated in getJsonNode method; nested progress writers are not supported and could cause issues. Also, to avoid having to pass around unirest, logger, and progressWriter all the time, probably better to have an inner class that takes these through Lombok RequiredArgsConstructor and performs all of the processing.
There was a problem hiding this comment.
Done! Both issues have been addressed:
Nested progress writer removed — downloadArtifactFpr() no longer calls progressWriterFactoryMixin.create(); it now uses the single progressWriter instance created in getJsonNode().
Inner class added — Introduced a private ArtifactProcessor inner class annotated with Lombok's @requiredargsconstructor, which holds unirest, logger, and progressWriter as final fields. All processing methods (processAllAviatorArtifacts, processFprRemediations, downloadArtifactFpr) are moved into this class, and the outer class methods access the inner class fields directly — no more parameter passing.
|
|
||
| JsonNode data = response.get("data"); | ||
| if (data == null || !data.isArray() || data.size() == 0) { | ||
| throw new FcliSimpleException( |
There was a problem hiding this comment.
Should this throw an exception, or simply return null?
There was a problem hiding this comment.
Throwing is the right choice here. The method returns a non-nullable SSCArtifactDescriptor, and the caller (processFprRemediations) uses it directly — returning null would just push the failure further down as an NPE with no context.
This check also catches a distinct failure case: no artifacts exist at all for the app version (e.g. invalid app version ID or nothing ever uploaded), which is different from the check further down that throws when artifacts exist but none are Aviator-processed. Throwing here gives the user an immediate, actionable error message for each scenario.
| for (JsonNode artifact : data) { | ||
| String originalFileName = artifact.path("originalFileName").asText(""); | ||
| if (!originalFileName.startsWith("aviator_")) { continue; } | ||
| if (sinceDate != null && !isUploadDateOnOrAfter(artifact, sinceDate)) { continue; } |
There was a problem hiding this comment.
Is this logic correct? If upload date for current non-Aviator artifact is older than sinceDate, then all subsequent artifacts will also be older than sinceDate, so once we reach this point, we'll never find an Aviator artifact that's newer than sinceDate.
There was a problem hiding this comment.
Great catch, you're absolutely right! Since artifacts are fetched in DESC order (newest first), once we encounter an artifact with uploadDate < sinceDate, all subsequent artifacts will also be older — so continue was wrong and was causing unnecessary iterations. Fixed by:
Checking the date first (before the filename check) — if it's too old, no point checking the filename
Changing continue → break to stop the loop immediately
Also took the opportunity to switch getAllAviatorArtifacts() from ASC to DESC ordering for the same reason — with ASC, a recent --since filter would wastefully fetch all older pages before reaching matching artifacts. With DESC, we hit the cutoff early, break, then reverse the result list before returning to maintain the ascending order contract.
| return getDescriptor(artifact); | ||
| } | ||
|
|
||
| throw new FcliSimpleException(buildNoArtifactsMessage(appVersionId, sinceDate)); |
There was a problem hiding this comment.
Same as above; null or exception?
|
|
||
| while (true) { | ||
| JsonNode response = unirest.get(SSCUrls.PROJECT_VERSION_ARTIFACTS(appVersionId)) | ||
| .queryString("orderby", "uploadDate ASC") |
There was a problem hiding this comment.
Same comment about orderBy as above, but given that you're trying to order by ascending date, this might have more impact. Also, if a customer has years of uploads, searching from the oldest date might not be very efficient.
| * @return SSCArtifactDescriptor of the most recent qualifying Aviator artifact | ||
| * @throws FcliSimpleException if no matching Aviator artifacts found | ||
| */ | ||
| public static final SSCArtifactDescriptor getLatestAviatorArtifact(UnirestInstance unirest, String appVersionId, OffsetDateTime sinceDate) { |
There was a problem hiding this comment.
Any way to improve this code/have less code duplication between the various Aviator-related methods?
There was a problem hiding this comment.
Done! Extracted the shared logic into three private helpers:
fetchAviatorArtifacts(unirest, appVersionId, sinceDate, maxResults) — single unified method handling DESC fetch, Aviator filename filtering, date-based early termination, and pagination; maxResults = 1 gives getLatestAviatorArtifact() behaviour, Integer.MAX_VALUE gives getAllAviatorArtifacts() behaviour
isAviatorArtifact(artifact) — extracts the aviator_ filename check
shouldStopProcessing(artifact, sinceDate) — extracts the date cutoff early-termination check
Both public methods are now thin wrappers — getLatestAviatorArtifact() calls fetchAviatorArtifacts(..., 1) and returns the first result, while getAllAviatorArtifacts() calls fetchAviatorArtifacts(..., MAX_VALUE) and reverses before returning to maintain the ascending order contract.
- Use Picocli ArgGroups for mutually exclusive --artifact-id, --latest, --all-open-issues options in new AviatorSSCApplyRemediationsArtifactSelectorMixin; note @mixin is not supported inside @ArgGroups, so --av/--delim options are inlined with manual validation in validate() - Refactor AviatorSSCApplyRemediationsCommand to use inner ArtifactProcessor class with @requiredargsconstructor, eliminating repeated unirest/logger/ progressWriter parameters and removing nested progress writer in downloadArtifactFpr() - Fix isSingular() to always return true; all three modes (--artifact-id, --latest, --all-open-issues) return exactly one aggregated ObjectNode, so output structure must be consistent regardless of option chosen - Refactor SSCArtifactHelper to reduce code duplication between getLatestAviatorArtifact() and getAllAviatorArtifacts() by extracting shared logic into fetchAviatorArtifacts(), isAviatorArtifact(), and shouldStopProcessing() helpers - Fix loop early-termination bug in getLatestAviatorArtifact(): change continue to break when uploadDate < sinceDate, since artifacts are ordered DESC and all subsequent artifacts will also be too old - Switch getAllAviatorArtifacts() from ASC to DESC ordering to enable early termination when --since is used; reverse result list before returning to maintain ascending order contract - Add SLF4J logger to SSCArtifactHelper and log a warning with artifact id when uploadDate cannot be parsed, instead of silently returning false - Add explanatory comment on the no-data check in getLatestAviatorArtifact() clarifying why an exception is thrown rather than returning null - Remove AviatorSSCApplyRemediationsCommandTest as validation is now handled declaratively by Picocli ArgGroups
…nd add usage examples - Move apply-remediations command from fcli-aviator to fcli-fod module (FoDAviatorApplyRemediationsCommand, FoDAviatorCommands, AviatorFoDApplyRemediationsHelper) - Remove FoD aviator subcommand from AviatorCommands; add aviator subcommand to FoDCommands - Add fcli-aviator dependency to fcli-fod build.gradle.kts - Update FoDMessages.properties with i18n keys for the moved command - Add concrete usage examples to apply-remediations command description covering --artifact-id, --latest, --all-open-issues, --since, and --source-dir combinations - Remove CHANGELOG.md (not applicable for this branch)
2 participants
This enhancement provides a more flexible and user-friendly experience for applying Aviator auto-remediations for SSC by introducing multiple artifact selection modes:
Enabled apply-remediations command for FoD as well