Merge pull request #170 from rostilos/1.5.5-rc

feat: Update issue classification logic to route unresolved snippets to AI for verification

Author: rostilos

java-ecosystem/libs/analysis-engine/src/main/java/org/rostilos/codecrow/analysisengine/service/IssueReconciliationEngine.java

@@ -360,10 +360,18 @@ public List<ContentClassification> classifyByContent(
 
                 results.add(new ContentClassification(
                         issue, Classification.CONFIRMED, updatedLine, updatedLineHash));
-            } else if (hasAnyReliableAnchor(issue)) {
-                results.add(new ContentClassification(
-                        issue, Classification.RESOLVED, null, null));
             } else {
+                // Content anchor not found — route to AI instead of auto-resolving.
+                // The anchor (codeSnippet / lineHash) may be stale or imprecise:
+                //  - lineHash can be NULL (file content unavailable at ingestion time)
+                //  - codeSnippet from LLM may not be verbatim (diff markers, truncation)
+                // When lineHash is null, only the (unreliable) snippet is checked.
+                // Auto-resolving here causes mass false-positive resolutions.
+                log.debug("Classify: content not found for {} issue (id={}) at line {} — "
+                                + "routing to AI. hasSnippet={}, hasLineHash={}",
+                        scope, issue.getId(), currentLine,
+                        issue.getCodeSnippet() != null && !issue.getCodeSnippet().isBlank(),
+                        issue.getLineHash() != null);
                 results.add(new ContentClassification(
                         issue, Classification.NEEDS_AI, null, null));
             }
@@ -468,8 +476,12 @@ public List<SweepResult> sweepDeterministic(
             }
 
             if (!contentFound) {
-                results.add(new SweepResult(issue, true,
-                        "Code snippet/hash no longer found in file (deterministic sweep)"));
+                // Don't auto-resolve: hash comparison alone is not reliable enough.
+                // The codeSnippet from the LLM may not be verbatim, and lineHash
+                // may be null (never computed at ingestion time). Prefer keeping
+                // stale issues over falsely resolving valid ones.
+                results.add(new SweepResult(issue, false,
+                        "Content anchor not found — skipped (not auto-resolved, requires AI verification)"));
             } else {
                 results.add(new SweepResult(issue, false, "Content anchor still present"));
             }
@@ -611,10 +623,14 @@ public List<ContentClassification> classifyByContent(
 
                 results.add(new ContentClassification(
                         issue, Classification.CONFIRMED, updatedLine, updatedLineHash));
-            } else if (hasAnyReliableAnchor(issue)) {
-                results.add(new ContentClassification(
-                        issue, Classification.RESOLVED, null, null));
             } else {
+                // Content anchor not found — route to AI instead of auto-resolving.
+                // Same reasoning as base classifyByContent: anchors can be stale/null.
+                log.debug("Classify (AST): content not found for {} issue (id={}) at line {} — "
+                                + "routing to AI. hasSnippet={}, hasLineHash={}",
+                        scope, issue.getId(), currentLine,
+                        issue.getCodeSnippet() != null && !issue.getCodeSnippet().isBlank(),
+                        issue.getLineHash() != null);
                 results.add(new ContentClassification(
                         issue, Classification.NEEDS_AI, null, null));
             }

java-ecosystem/libs/analysis-engine/src/main/java/org/rostilos/codecrow/analysisengine/service/branch/BranchIssueReconciliationService.java

@@ -265,12 +265,30 @@ public int sweepDeterministicResolutions(
                             request.getCommitHash(), filePath);
                 }
                 if (fileContent == null) {
-                    // File doesn't exist — resolve all issues for this file
-                    for (BranchIssue bi : fileIssues) {
-                        resolveIssue(bi, request.getCommitHash(), request.getSourcePrNumber(),
-                                "File no longer exists on branch (deterministic sweep)",
-                                "deterministic-sweep");
-                        resolvedCount++;
+                    // Content unavailable — verify file existence explicitly.
+                    // getFileContent returning null could be an API error/timeout/rate-limit,
+                    // not actual file deletion. Must confirm before resolving (fail-open).
+                    boolean confirmedDeleted = false;
+                    try {
+                        confirmedDeleted = !operationsService.checkFileExistsInBranch(
+                                client, vcsRepoInfo.getRepoWorkspace(), vcsRepoInfo.getRepoSlug(),
+                                request.getTargetBranchName(), filePath);
+                    } catch (Exception ex) {
+                        log.warn("Sweep: file existence check failed for {} — skipping (fail-open): {}",
+                                filePath, ex.getMessage());
+                        continue; // Don't resolve on error
+                    }
+                    if (confirmedDeleted) {
+                        for (BranchIssue bi : fileIssues) {
+                            resolveIssue(bi, request.getCommitHash(), request.getSourcePrNumber(),
+                                    "File no longer exists on branch (deterministic sweep)",
+                                    "deterministic-sweep");
+                            resolvedCount++;
+                        }
+                    } else {
+                        log.warn("Sweep: file {} exists but content unavailable — "
+                                + "skipping {} issues (fail-open to prevent false resolution)",
+                                filePath, fileIssues.size());
                     }
                     continue;
                 }

java-ecosystem/libs/analysis-engine/src/test/java/org/rostilos/codecrow/analysisengine/service/IssueReconciliationEngineTest.java

@@ -253,16 +253,17 @@ void snippetFoundInContent_shouldBeConfirmed() {
         }
 
         @Test
-        void snippetGone_shouldBeResolved() {
+        void snippetGone_shouldBeRoutedToAi() {
             String content = "alpha\ngamma\ndelta";
             var hashes = LineHashSequence.from(content);
-            // Snippet "beta" at line 2 — but "beta" no longer exists
+            // Snippet "beta" at line 2 — but "beta" no longer exists.
+            // Should route to AI for verification, not auto-resolve.
             var results = engine.classifyByContent(
                     List.of(issue(2, "beta")),
                     hashes);
             assertThat(results).hasSize(1);
             assertThat(results.get(0).classification())
-                    .isEqualTo(IssueReconciliationEngine.Classification.RESOLVED);
+                    .isEqualTo(IssueReconciliationEngine.Classification.NEEDS_AI);
         }
 
         @Test
@@ -279,16 +280,18 @@ void lineHashFoundButNoSnippet_shouldBeConfirmed() {
         }
 
         @Test
-        void lineHashGone_shouldBeResolved() {
+        void lineHashGone_shouldBeRoutedToAi() {
             String content = "alpha\ngamma\ndelta";
             var hashes = LineHashSequence.from(content);
             String betaHash = LineHashSequence.hashLine("beta");
+            // lineHash for "beta" no longer in file.
+            // Should route to AI for verification, not auto-resolve.
             var results = engine.classifyByContent(
                     List.of(issue(2, null, betaHash)),
                     hashes);
             assertThat(results).hasSize(1);
             assertThat(results.get(0).classification())
-                    .isEqualTo(IssueReconciliationEngine.Classification.RESOLVED);
+                    .isEqualTo(IssueReconciliationEngine.Classification.NEEDS_AI);
         }
 
         @Test
@@ -335,19 +338,18 @@ void contextHashChanged_blockScope_shouldBeNeedsAi() {
         void multipleIssues_mixedClassifications() {
             String content = "alpha\nbeta\ngamma\ndelta";
             var hashes = LineHashSequence.from(content);
-            String betaHash = LineHashSequence.hashLine("beta");
 
             var results = engine.classifyByContent(List.of(
                     issue(2, "beta"),           // CONFIRMED (snippet found)
-                    issue(3, "zzzz"),           // RESOLVED (snippet gone)
+                    issue(3, "zzzz"),           // NEEDS_AI (snippet gone — routed to AI)
                     issueWithScope(1, null, IssueScope.FILE) // NEEDS_AI (FILE scope)
             ), hashes);
 
             assertThat(results).hasSize(3);
             assertThat(results.get(0).classification())
                     .isEqualTo(IssueReconciliationEngine.Classification.CONFIRMED);
             assertThat(results.get(1).classification())
-                    .isEqualTo(IssueReconciliationEngine.Classification.RESOLVED);
+                    .isEqualTo(IssueReconciliationEngine.Classification.NEEDS_AI);
             assertThat(results.get(2).classification())
                     .isEqualTo(IssueReconciliationEngine.Classification.NEEDS_AI);
         }
@@ -404,26 +406,29 @@ void snippetStillPresent_shouldNotResolve() {
         }
 
         @Test
-        void snippetGone_shouldResolve() {
+        void snippetGone_shouldNotAutoResolve() {
             String content = "alpha\ngamma\ndelta";
             var hashes = LineHashSequence.from(content);
+            // Snippet "beta" gone from file — but sweep should NOT auto-resolve.
+            // Hash comparison alone is unreliable (LLM snippet not verbatim, null lineHash).
             var results = engine.sweepDeterministic(
                     List.of(issue(2, "beta")),
                     hashes);
             assertThat(results).hasSize(1);
-            assertThat(results.get(0).resolved()).isTrue();
+            assertThat(results.get(0).resolved()).isFalse();
         }
 
         @Test
-        void lineHashGone_shouldResolve() {
+        void lineHashGone_shouldNotAutoResolve() {
             String content = "alpha\ngamma\ndelta";
             var hashes = LineHashSequence.from(content);
             String betaHash = LineHashSequence.hashLine("beta");
+            // lineHash for "beta" gone — but sweep should NOT auto-resolve.
             var results = engine.sweepDeterministic(
                     List.of(issue(2, null, betaHash)),
                     hashes);
             assertThat(results).hasSize(1);
-            assertThat(results.get(0).resolved()).isTrue();
+            assertThat(results.get(0).resolved()).isFalse();
         }
 
         @Test
@@ -454,17 +459,16 @@ void blockScope_someSnippetLinesPresent_shouldNotResolve() {
         void multipleIssues_mixedResults() {
             String content = "alpha\nbeta\ngamma";
             var hashes = LineHashSequence.from(content);
-            String betaHash = LineHashSequence.hashLine("beta");
             String zzzzHash = LineHashSequence.hashLine("zzzz");
 
             var results = engine.sweepDeterministic(List.of(
                     issue(2, "beta"),           // present → not resolved
-                    issue(2, null, zzzzHash)    // gone → resolved
+                    issue(2, null, zzzzHash)    // gone → still not auto-resolved (requires AI)
             ), hashes);
 
             assertThat(results).hasSize(2);
             assertThat(results.get(0).resolved()).isFalse();
-            assertThat(results.get(1).resolved()).isTrue();
+            assertThat(results.get(1).resolved()).isFalse();
         }
     }
 
@@ -490,10 +494,12 @@ void classifyByContent_nullAst_shouldFallBackToBase() {
         void sweepDeterministic_nullAst_shouldFallBackToBase() {
             String content = "alpha\ngamma\ndelta";
             var hashes = LineHashSequence.from(content);
+            // AST overload with null tree/resolver falls back to base method,
+            // which no longer auto-resolves (anchor not found → skip, not resolve)
             var results = engine.sweepDeterministic(
                     List.of(issue(2, "beta")), hashes, null, null);
             assertThat(results).hasSize(1);
-            assertThat(results.get(0).resolved()).isTrue();
+            assertThat(results.get(0).resolved()).isFalse();
         }
 
         @Test