diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java index 6447568812..05fe0e949e 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/AppliedChangeLedger.java @@ -84,6 +84,8 @@ private static AppliedChange fromPending(PendingAppliedChange pac) { return AppliedChange.builder() .originalLineFrom(pac.lineFrom()) .originalLineTo(pac.lineTo()) + .declaredLineFrom(pac.declaredLineFrom()) + .declaredLineTo(pac.declaredLineTo()) .deltaLines(pac.deltaLines()) .comparisonCode(pac.comparisonCode()) .lineNormalizedCode(pac.lineNormalizedCode()) diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java index d9eb175472..24f0a32f30 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/HunkClassifier.java @@ -80,13 +80,13 @@ public List classifyRemediationHunks(Remediation remediation, Path */ private HunkOutcome classifyRange(int lineFrom, int lineTo, List applied, String candidateComparisonCode) { for (AppliedChange ac : applied) { - if (ac.coversFully(lineFrom, lineTo)) { - if (ac.contentCovers(candidateComparisonCode, lineFrom, lineTo)) { + if (ac.coversDeclaredRange(lineFrom, lineTo)){ + if (ac.contentCovers(candidateComparisonCode, lineFrom, lineTo)) { return HunkOutcome.SUPERSEDED; } return HunkOutcome.POSSIBLY_REMEDIATED; } - if (ac.overlapsPartially(lineFrom, lineTo)) { + if (ac.overlapsDeclaredRangePartially(lineFrom, lineTo)) { return HunkOutcome.CONFLICTS; } } diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java index b714e621f8..9fd2a17167 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/classifier/PendingAppliedChange.java @@ -14,6 +14,16 @@ import java.nio.file.Path; -public record PendingAppliedChange(Path filePath, int lineFrom, int lineTo, int deltaLines, - String comparisonCode, String lineNormalizedCode) { +import lombok.Builder; + +@Builder +public record PendingAppliedChange( + Path filePath, + int lineFrom, + int lineTo, + int declaredLineFrom, + int declaredLineTo, + int deltaLines, + String comparisonCode, + String lineNormalizedCode) { } diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java index f37376b2fc..89e5b71bde 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChange.java @@ -26,18 +26,24 @@ public final class AppliedChange { private final int originalLineFrom; private final int originalLineTo; + private final int declaredLineFrom; + private final int declaredLineTo; private final int deltaLines; private final String comparisonCode; private final String[] lineNormalizedContent; public AppliedChange(int originalLineFrom, int originalLineTo, int deltaLines, String comparisonCode) { - this(originalLineFrom, originalLineTo, deltaLines, comparisonCode, null); + this(originalLineFrom, originalLineTo, originalLineFrom, originalLineTo, deltaLines, comparisonCode, null); + } @Builder - public AppliedChange(int originalLineFrom, int originalLineTo, int deltaLines, String comparisonCode, String lineNormalizedCode) { + public AppliedChange(int originalLineFrom, int originalLineTo, int declaredLineFrom, int declaredLineTo, int deltaLines, String comparisonCode, String lineNormalizedCode) + { this.originalLineFrom = originalLineFrom; this.originalLineTo = originalLineTo; + this.declaredLineFrom = declaredLineFrom; + this.declaredLineTo = declaredLineTo; this.deltaLines = deltaLines; this.comparisonCode = comparisonCode; // Store line-by-line normalized content for offset-anchored comparison (newlines preserved, each line normalized) @@ -63,10 +69,25 @@ public boolean coversFully(int lineFrom, int lineTo) { /** True if [lineFrom, lineTo] overlaps this change's original range without either side fully containing the other. */ public boolean overlapsPartially(int lineFrom, int lineTo) { - boolean disjoint = lineTo < originalLineFrom || lineFrom > originalLineTo; - return !disjoint; + boolean overlaps = lineFrom <= originalLineTo && originalLineFrom <= lineTo; + boolean candidateContainsThis = lineFrom <= originalLineFrom && originalLineTo <= lineTo; + return overlaps && !coversFully(lineFrom, lineTo) && !candidateContainsThis; + } + + + /** True if this change's declared range fully contains [lineFrom, lineTo]. */ + public boolean coversDeclaredRange(int lineFrom, int lineTo) { + return declaredLineFrom <= lineFrom && lineTo <= declaredLineTo; } + /** True if [lineFrom, lineTo] overlaps this change's declared range without either side fully containing the other. */ + public boolean overlapsDeclaredRangePartially(int lineFrom, int lineTo) { + boolean overlaps = lineFrom <= declaredLineTo && declaredLineFrom <= lineTo; + boolean candidateContainsThis = lineFrom <= declaredLineFrom && declaredLineTo <= lineTo; + return overlaps && !coversDeclaredRange(lineFrom, lineTo) && !candidateContainsThis; + } + + /** * Below this length a normalized comparison code (e.g. {@code "return;"}) is too short and * generic to prove that a substring hit inside a broader fix's content is the same fix, diff --git a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java index df63f82cf6..2a7f5cefec 100644 --- a/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java +++ b/fcli-core/fcli-aviator-common/src/main/java/com/fortify/cli/aviator/fpr/remediation/writer/FileWriteCoordinator.java @@ -134,7 +134,16 @@ private void processFileChanges(Remediation remediation, FileChange fileChange, int linesAfterChange = updatedContent.split("\n", -1).length; int delta = linesAfterChange - linesBeforeChange; String lineNormalizedCode = hunk.lineNormalizedCode(filename); - ledger.stage(new PendingAppliedChange(filePath, actualLineFrom, actualLineTo, delta, comparisonCode, lineNormalizedCode)); + ledger.stage(PendingAppliedChange.builder() + .filePath(filePath) + .lineFrom(actualLineFrom) + .lineTo(actualLineTo) + .declaredLineFrom(declaredLineFrom) + .declaredLineTo(declaredLineTo) + .deltaLines(delta) + .comparisonCode(comparisonCode) + .lineNormalizedCode(lineNormalizedCode) + .build()); appliedKeysOut.add(key); appliedInThisFile++; } diff --git a/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java b/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java index a81c57b1b0..18424ba44f 100644 --- a/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java +++ b/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/processor/RemediationProcessorTest.java @@ -1049,6 +1049,42 @@ void secondFprDoesNotApplyOverAFixTheFirstFprAlreadyRewrote() throws Exception { assertEquals(1, second.skippedRemediations()); assertEquals(afterFirst, Files.readString(sourceFile), "the second FPR must leave the file untouched"); } + /** + * Regression test for coordinate-space mismatch bug: HunkClassifier must compare declared + * (pristine-file) line numbers against declared ranges, not actual (post-shift) ranges, when + * pre-classifying narrower hunks nested inside a broader fix that shifted them. The broader + * fix (wide-deletes) declares lines 1-5 and deletes them, replacing with 2 lines (delta -3). + * The narrower fix (narrow-inside) declares line 3, which sits inside 1-5's declared range. + * Before the fix, classifyRange would compare declared-3 against the broader hunk's ACTUAL + * range (which is now at a shifted position), missing the nesting; after the fix, it compares + * declared-against-declared and correctly classifies as POSSIBLY_REMEDIATED. + */ + @Test + void narrowerRemediationInsideBroaderDeleteIsClassifiedAsPossiblyRemediated() throws Exception { + String originalSource = "line1\nline2\nline3\nline4\nline5\nline6\n"; + writeSourceFile("Example.java", originalSource); + Path fprPath = buildFpr(List.of( + new MultiHunkRemediationSpec("wide-deletes", List.of(new FileSpec("Example.java", List.of( + new HunkSpec(1, 5, 0, 0, + "line1\nline2\nline3\nline4\nline5", + "line1\nline2\nline3\nline4\nline5", + "replacement1\nreplacement2"))))), + new MultiHunkRemediationSpec("narrow-inside", List.of(new FileSpec("Example.java", List.of( + new HunkSpec(3, 3, 1, 1, "line2\nline3\nline4", + "line3", "line3-modified"))))))); + + RemediationMetric metric = apply(fprPath); + + assertEquals(2, metric.totalRemediations()); + assertEquals(1, metric.appliedRemediations(), "the broader fix must be applied"); + assertEquals(1, metric.possiblyRemediatedRemediations(), + "the narrower fix's declared line 3 sits inside the broader fix's declared 1-5, so it must be " + + "pre-classified as POSSIBLY_REMEDIATED even though its actual position shifted"); + assertEquals(0, metric.skippedRemediations(), + "no remediation should be skipped; the narrower one must not fail with ANCHOR_DOES_NOT_MATCH"); + } + + private record HunkSpec(int lineFrom, int lineTo, int contextBefore, int contextAfter, String context, String originalCode, String newCode) {} @@ -1295,12 +1331,12 @@ void previewModePopulatesPreviewDetailsWithChanges() throws Exception { assertEquals("available", detail.status()); assertNotNull(detail.files()); assertEquals(1, detail.files().size()); - + var filePreview = detail.files().get("Example.java"); assertNotNull(filePreview); assertEquals("UTF-8", filePreview.encoding()); assertEquals(1, filePreview.changes().size()); - + var change = filePreview.changes().get(0); assertEquals(1, change.changeIndex()); assertEquals(3, change.lineFrom()); @@ -1344,7 +1380,7 @@ void previewModeCapturesSkipReasonsInPreviewDetails() throws Exception { String validHash = TestHashUtil.sha256Base64Unix("class Valid { void run() { old(); } }"); String missingHash = "dGVzdGhhc2g="; // arbitrary hash for missing file - + String xml = """ @@ -1414,7 +1450,7 @@ void previewModeCapturesSkipReasonsInPreviewDetails() throws Exception { .findFirst().orElseThrow(); assertEquals("ISSUE-VALID", available.issueId()); assertNotNull(available.files().get("Valid.java")); - + var skipped = metric.previewDetails().stream() .filter(d -> "skipped".equals(d.status())) .findFirst().orElseThrow(); diff --git a/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChangeTest.java b/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChangeTest.java index cc902959a2..71cb8f4024 100644 --- a/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChangeTest.java +++ b/fcli-core/fcli-aviator-common/src/test/java/com/fortify/cli/aviator/fpr/remediation/model/AppliedChangeTest.java @@ -29,9 +29,24 @@ class AppliedChangeTest { */ @Test void unavailableComparisonCodeIsNotTreatedAsProvenCoverage() { - assertFalse(new AppliedChange(1, 3, 0, "W1W2W3").contentCovers(null, 2, 2), + assertFalse( + AppliedChange.builder() + .originalLineFrom(1) + .originalLineTo(3) + .deltaLines(0) + .comparisonCode("W1W2W3") + .build() + .contentCovers(null, 2, 2), "a candidate whose content could not be computed has not been proven covered"); - assertFalse(new AppliedChange(1, 3, 0, null).contentCovers("M2", 2, 2), + + assertFalse( + AppliedChange.builder() + .originalLineFrom(1) + .originalLineTo(3) + .deltaLines(0) + .comparisonCode(null) + .build() + .contentCovers("M2", 2, 2), "an applied change whose content is unknown cannot prove it covers anything"); } }