refactor: clear the SonarCloud new-code findings on main

src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java

@@ -20,6 +20,7 @@
 import java.time.Duration;
 import java.time.Instant;
 import java.util.Optional;
+import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 /**
@@ -50,19 +51,26 @@ public final class GitHubApiError {
    * an error body, but a body is untrusted text on its way to a log file, so a token-shaped run of
    * characters is masked rather than trusted to be harmless.
    *
-   * <p>Deliberately ONE alternation rather than one pattern per shape: the shapes overlap, and a
+   * <p>Deliberately ONE alternation, read as a union across this pattern and {@link
+   * #CREDENTIAL_SHAPED_VALUE}, rather than one masking pass per shape: the shapes overlap, and a
    * single pass masks each overlap as the leftmost match, whereas masking in passes decides the
    * overlap by pass order and leaves material the one-pass form masks. Whichever order is chosen,
    * {@code "…ghp_<7 chars>Bearer <20 chars>"} keeps its token prefix unmasked if the bearer pass
    * runs first, and {@code "Bearer ghp_<10 chars>.<tail>"} keeps the tail of the bearer value if
-   * the token pass does. So this stays one pattern even though it reads as four.
+   * the token pass does. So {@link #redactCredentials} still scans once, taking the leftmost match
+   * across both patterns — split in two, prefixes here and value shapes there, only because one
+   * alternation of all four shapes is more than the regex complexity budget allows.
    */
-  private static final Pattern CREDENTIAL_SHAPED =
+  private static final Pattern CREDENTIAL_SHAPED_PREFIX =
+      Pattern.compile("(?i)(gh[pousr]_\\w{10,})|(github_pat_\\w{10,})");
+
+  /**
+   * The bearer and JWT shapes — the value half of {@link #CREDENTIAL_SHAPED_PREFIX}'s union, tried
+   * second on a position tie exactly as the one-alternation form tried its alternatives in order.
+   */
+  private static final Pattern CREDENTIAL_SHAPED_VALUE =
       Pattern.compile(
-          "(?i)(gh[pousr]_\\w{10,})"
-              + "|(github_pat_\\w{10,})"
-              + "|(bearer\\s+[\\w.~+/=-]{10,})"
-              + "|(eyJ[\\w-]{8,}\\.[\\w-]{8,}\\.[\\w-]{8,})");
+          "(?i)(bearer\\s+[\\w.~+/=-]{10,})" + "|(eyJ[\\w-]{8,}\\.[\\w-]{8,}\\.[\\w-]{8,})");
 
   /**
    * The wording GitHub uses when it is throttling rather than refusing. A secondary rate limit and
@@ -216,6 +224,64 @@ private static Duration atLeastZero(Duration delay) {
     return delay.isNegative() ? Duration.ZERO : delay;
   }
 
+  /**
+   * One left-to-right masking pass over both credential patterns: at each step the leftmost match
+   * wins, a position tie goes to the prefix shapes, and scanning resumes after the mask — the
+   * verbatim {@code replaceAll} semantics of the four shapes as one alternation, kept even though
+   * the alternation itself had to be split to fit the regex complexity budget. No shape matches an
+   * empty string, so every step advances.
+   */
+  private static String redactCredentials(String text) {
+    Matcher[] shapes = {
+      CREDENTIAL_SHAPED_PREFIX.matcher(text), CREDENTIAL_SHAPED_VALUE.matcher(text)
+    };
+    var starts = new int[] {-1, -1};
+    var ends = new int[shapes.length];
+    var out = new StringBuilder(text.length());
+    var from = 0;
+    while (true) {
+      var leftmost = leftmostShape(shapes, starts, ends, from);
+      if (leftmost == -1) {
+        break;
+      }
+      out.append(text, from, starts[leftmost]).append(REDACTED);
+      from = ends[leftmost];
+      starts[leftmost] = -1;
+    }
+    return out.append(text, from, text.length()).toString();
+  }
+
+  /** A shape with no further matches; loses every comparison for the leftmost slot. */
+  private static final int NO_MORE_MATCHES = Integer.MAX_VALUE;
+
+  /**
+   * The index of the shape holding the leftmost live candidate at or past {@code from}, or -1 when
+   * both shapes are out of matches; a position tie keeps the lowest index, the one-alternation
+   * form's alternative order. Only a consumed or overtaken candidate is re-found — one at or past
+   * {@code from} is still that shape's next match, since the find that produced it proved nothing
+   * of that shape starts before it — so each shape is scanned once end to end rather than from
+   * every resume point.
+   */
+  private static int leftmostShape(Matcher[] shapes, int[] starts, int[] ends, int from) {
+    var leftmost = -1;
+    var best = NO_MORE_MATCHES;
+    for (var i = 0; i < shapes.length; i++) {
+      if (starts[i] < from) {
+        if (shapes[i].find(from)) {
+          starts[i] = shapes[i].start();
+          ends[i] = shapes[i].end();
+        } else {
+          starts[i] = NO_MORE_MATCHES;
+        }
+      }
+      if (starts[i] < best) {
+        best = starts[i];
+        leftmost = i;
+      }
+    }
+    return leftmost;
+  }
+
   /**
    * The response body as loggable text. An inbound response is buffered first so reading it here
    * does not consume it for the caller that later inspects the same exception; a response built
@@ -239,7 +305,7 @@ private static String clean(String raw) {
       return "";
     }
     var collapsed = WHITESPACE.matcher(raw.strip()).replaceAll(" ");
-    var redacted = CREDENTIAL_SHAPED.matcher(collapsed).replaceAll(REDACTED);
+    var redacted = redactCredentials(collapsed);
     if (redacted.length() <= MAX_BODY_CHARS) {
       return redacted;
     }

src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java

@@ -268,23 +268,30 @@ private static List<String> sanitize(List<String> raw, String source) {
     for (String value : raw) {
       // Never null: entries come from asText() (empty string at worst) or String.split.
       var pattern = value.trim();
-      if (pattern.isEmpty()) {
-        continue;
+      if (isKeepablePattern(pattern, source)) {
+        if (patterns.size() >= MAX_PATTERNS) {
+          log.warn(
+              "Repository config {}: more than {} ignore patterns; using the first {}",
+              source,
+              MAX_PATTERNS,
+              MAX_PATTERNS);
+          break;
+        }
+        patterns.add(pattern);
       }
-      if (pattern.length() > MAX_PATTERN_LENGTH) {
-        log.warn("Repository config {}: dropping over-long ignore pattern", source);
-        continue;
-      }
-      if (patterns.size() >= MAX_PATTERNS) {
-        log.warn(
-            "Repository config {}: more than {} ignore patterns; using the first {}",
-            source,
-            MAX_PATTERNS,
-            MAX_PATTERNS);
-        break;
-      }
-      patterns.add(pattern);
     }
     return List.copyOf(patterns);
   }
+
+  /** Whether a trimmed entry survives sanitization: blanks drop silently, over-long ones warn. */
+  private static boolean isKeepablePattern(String pattern, String source) {
+    if (pattern.isEmpty()) {
+      return false;
+    }
+    if (pattern.length() > MAX_PATTERN_LENGTH) {
+      log.warn("Repository config {}: dropping over-long ignore pattern", source);
+      return false;
+    }
+    return true;
+  }
 }

src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java

@@ -47,6 +47,7 @@
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.stream.IntStream;
+import org.jboss.logging.Logger;
 
 /**
  * The post-AI finding chain: validate quotes, dedupe, verify against the diff, drop already-replied
@@ -57,6 +58,14 @@
 @ApplicationScoped
 public class FindingPipeline {
 
+  /**
+   * Logger pinned to this class so the empty-batch warning emitted from {@link BatchPrompts} keeps
+   * the category operators already filter on: the build-time {@code Log} facade binds its category
+   * to the class holding the call, which for a nested type would silently relabel the WARN to
+   * {@code FindingPipeline$BatchPrompts}.
+   */
+  private static final Logger LOG = Logger.getLogger(FindingPipeline.class);
+
   /** Directory rows listed in the scope header before the remainder is rolled up by count. */
   private static final int MAX_SCOPE_DIRECTORIES = 10;
 
@@ -109,6 +118,45 @@ AiReviewService.PromptInputs forBatch(
               template.repoInstructions(), heuristicFailureModesFor(batch)));
     }
 
+    /**
+     * The heuristic failure-mode review dimension (#123 / #420) for one batch, appended to that
+     * batch's trailing guidance.
+     *
+     * <p>{@link ReviewPromptAssembler} decides this section from {@code ctx.diff()}, which {@link
+     * ReviewContextLoader} leaves empty whenever token budgeting is on — the shipped default — so
+     * the whole dimension was silently absent from every default-configuration review (#486 P3). It
+     * is decided here instead because this is the first point that holds the material the call
+     * actually receives: the plan's batch text. Only the batches whose own slice introduces a
+     * decision rule pay for it, which is stricter than the whole-diff gate it replaces. The two
+     * cannot both emit the section: this runs only on a budgeted plan, and the assembler's gate
+     * only has material when budgeting is off — the loader keys the empty {@code ctx.diff()} on the
+     * setting the planner keys {@code budgeted} on.
+     *
+     * <p>Sizing: the section is a single fixed constant, and the planner sized the shared overhead
+     * before it existed. That mirrors the {@linkplain #withheldMaterialNotice withheld-material
+     * notice} this class already adds after planning, and it is what the token safety margin (10%
+     * of the input cap by default, ~4800 tokens against this section's ~700) is held back for.
+     *
+     * <p>Warns through {@link #LOG}, pinned to the enclosing class, so the operator-facing category
+     * is unchanged by this method living in {@link BatchPrompts}.
+     */
+    private static String heuristicFailureModesFor(DiffBudgetPlanner.DiffBatch batch) {
+      var scanned = heuristicScanSource(batch.text());
+      if (scanned.isBlank()) {
+        // Loud on purpose: an empty input is exactly what made this dimension die unnoticed, and a
+        // detector fed nothing reports "no heuristic code" in the same voice as a detector that
+        // read
+        // the diff and found none.
+        LOG.warnf(
+            "Review batch covering %d file(s) carries no diff text, so the heuristic failure-mode"
+                + " review dimension has no material to evaluate and is omitted from that call —"
+                + " this is a planning defect, not a pull request that introduces no heuristic code",
+            batch.files().size());
+        return "";
+      }
+      return ReviewPromptAssembler.heuristicFailureModesSection(scanned);
+    }
+
     /**
      * Copies the shared prompt context, swapping the diff, base-comparison and trailing-guidance
      * slots.
@@ -1043,46 +1091,6 @@ private static String renamePath(GitHubPullRequestClient.FileDiff file) {
         : previous + " → " + file.filename();
   }
 
-  /**
-   * The heuristic failure-mode review dimension (#123 / #420) for one batch, appended to that
-   * batch's trailing guidance.
-   *
-   * <p>{@link ReviewPromptAssembler} decides this section from {@code ctx.diff()}, which {@link
-   * ReviewContextLoader} leaves empty whenever token budgeting is on — the shipped default — so the
-   * whole dimension was silently absent from every default-configuration review (#486 P3). It is
-   * decided here instead because this is the first point that holds the material the call actually
-   * receives: the plan's batch text. Only the batches whose own slice introduces a decision rule
-   * pay for it, which is stricter than the whole-diff gate it replaces. The two cannot both emit
-   * the section: this runs only on a budgeted plan, and the assembler's gate only has material when
-   * budgeting is off — the loader keys the empty {@code ctx.diff()} on the setting the planner keys
-   * {@code budgeted} on.
-   *
-   * <p>Sizing: the section is a single fixed constant, and the planner sized the shared overhead
-   * before it existed. That mirrors the {@linkplain #withheldMaterialNotice withheld-material
-   * notice} this class already adds after planning, and it is what the token safety margin (10% of
-   * the input cap by default, ~4800 tokens against this section's ~700) is held back for.
-   *
-   * <p>Deliberately <em>not</em> moved into {@link BatchPrompts}, its only caller: {@code Log}
-   * binds its category to the enclosing class at build time, so relocating the warning below would
-   * relabel an operator-facing WARN from this class to {@code FindingPipeline$BatchPrompts} and
-   * silently break any log filter keyed on the category.
-   */
-  private static String heuristicFailureModesFor(DiffBudgetPlanner.DiffBatch batch) {
-    var scanned = heuristicScanSource(batch.text());
-    if (scanned.isBlank()) {
-      // Loud on purpose: an empty input is exactly what made this dimension die unnoticed, and a
-      // detector fed nothing reports "no heuristic code" in the same voice as a detector that read
-      // the diff and found none.
-      Log.warnf(
-          "Review batch covering %d file(s) carries no diff text, so the heuristic failure-mode"
-              + " review dimension has no material to evaluate and is omitted from that call —"
-              + " this is a planning defect, not a pull request that introduces no heuristic code",
-          batch.files().size());
-      return "";
-    }
-    return ReviewPromptAssembler.heuristicFailureModesSection(scanned);
-  }
-
   /**
    * The batch text with each rendered {@code ### path (status, +a -d)} header rewritten to the
    * unified-diff {@code +++ b/path} form {@link HeuristicCodeDetector} scopes files by. Without it

src/main/java/dev/thiagogonzaga/thrillhousebot/review/HeuristicCodeDetector.java

@@ -164,12 +164,6 @@ final class HeuristicCodeDetector {
 
   private HeuristicCodeDetector() {}
 
-  private static boolean isTestPath(String path) {
-    return TEST_DIRECTORY_SEGMENT.matcher(path).find()
-        || TEST_FILENAME_MARKER.matcher(path).find()
-        || JAVA_TEST_SUFFIX.matcher(path).find();
-  }
-
   /** Per-file scanning state derived from a {@code +++ } diff header line. */
   private record FileScope(boolean testFile, boolean javaScript) {
     private static final FileScope NONE = new FileScope(false, false);
@@ -182,6 +176,12 @@ private static FileScope of(String headerLine) {
       var path = header.group(1);
       return new FileScope(isTestPath(path), JS_REGEX_PATH.matcher(path).find());
     }
+
+    private static boolean isTestPath(String path) {
+      return TEST_DIRECTORY_SEGMENT.matcher(path).find()
+          || TEST_FILENAME_MARKER.matcher(path).find()
+          || JAVA_TEST_SUFFIX.matcher(path).find();
+    }
   }
 
   /**

src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java

@@ -64,6 +64,15 @@ final class JacocoCoverageReport {
   /** Ceiling on uncovered lines recorded per source file. */
   static final int MAX_LINES_PER_FILE = 5_000;
 
+  /**
+   * Separator between a JaCoCo {@code <package name>} and a source file name. Not a filesystem
+   * separator and never platform-dependent: the package name is the JVM internal binary name, which
+   * the class-file format defines as '/'-separated on every platform, and the repository paths it
+   * is matched against are git paths, also always '/'. A {@code File.separator} here would break
+   * every report produced on Windows.
+   */
+  private static final String BINARY_PACKAGE_SEPARATOR = "/";
+
   /** No coverage data — the value every failure path degrades to. */
   static final JacocoCoverageReport EMPTY = new JacocoCoverageReport(Map.of());
 
@@ -194,12 +203,7 @@ private static Map<String, NavigableSet<Integer>> readSourceFiles(XMLStreamReade
           var name = attribute(reader, "name", "");
           current = null;
           if (!name.isBlank() && result.size() < MAX_SOURCE_FILES) {
-            // '/' is not a filesystem separator here and must not be made platform-dependent:
-            // a JaCoCo <package name> is the JVM internal binary name, which the class-file
-            // format defines as '/'-separated on every platform, and the repository paths it is
-            // matched against are git paths, also always '/'. A File.separator here would break
-            // every report produced on Windows.
-            var path = packageName.isBlank() ? name : packageName + "/" + name;
+            var path = packageName.isBlank() ? name : packageName + BINARY_PACKAGE_SEPARATOR + name;
             current = result.computeIfAbsent(path, unused -> new TreeSet<>());
           }
         }