From 2abd828afcf51cf8a185520af33f0519db649598 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 15:55:21 -0300 Subject: [PATCH 1/6] refactor: clear the SonarCloud new-code findings on main --- .../thrillhousebot/github/GitHubApiError.java | 50 +++- .../github/RepoSettingsParser.java | 37 +-- .../review/FindingPipeline.java | 88 +++---- .../review/HeuristicCodeDetector.java | 12 +- .../review/JacocoCoverageReport.java | 16 +- .../review/ReviewDiffFormatter.java | 70 +++--- .../review/ai/FindingVerificationService.java | 216 +++++++++++++----- .../dashboard/AuthResourceTestFixtures.java | 9 +- .../github/GitHubApiErrorTest.java | 20 ++ .../review/BlockingStrictnessTest.java | 5 +- .../ai/FindingVerificationServiceTest.java | 11 +- 11 files changed, 355 insertions(+), 179 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index 0f85c217..9d50cf16 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/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. * - *

Deliberately ONE alternation rather than one pattern per shape: the shapes overlap, and a + *

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>."} 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,32 @@ 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 prefix = CREDENTIAL_SHAPED_PREFIX.matcher(text); + Matcher value = CREDENTIAL_SHAPED_VALUE.matcher(text); + var out = new StringBuilder(text.length()); + var from = 0; + while (from < text.length()) { + boolean prefixFound = prefix.find(from); + boolean valueFound = value.find(from); + if (!prefixFound && !valueFound) { + break; + } + Matcher leftmost = + prefixFound && (!valueFound || prefix.start() <= value.start()) ? prefix : value; + out.append(text, from, leftmost.start()).append(REDACTED); + from = leftmost.end(); + } + return out.append(text, from, text.length()).toString(); + } + /** * 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 +273,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; } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java index d2ab76f3..c1ae86f7 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java @@ -268,23 +268,30 @@ private static List sanitize(List 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; + } } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java index b5013d58..e7d784c3 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java +++ b/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. + * + *

{@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. + * + *

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. + * + *

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. - * - *

{@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. - * - *

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. - * - *

Deliberately not 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 diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/HeuristicCodeDetector.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/HeuristicCodeDetector.java index b68030ab..35f70c6f 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/HeuristicCodeDetector.java +++ b/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(); + } } /** diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java index 371636eb..e7249b12 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java +++ b/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 } 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> 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 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<>()); } } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java index cdbcd2f1..30d32b49 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java @@ -32,6 +32,7 @@ import java.util.Set; import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; +import org.jboss.logging.Logger; /** * Applies review-scoping rules from config: skip ignored file patterns, and optionally cap total @@ -42,6 +43,14 @@ @ApplicationScoped public class ReviewDiffFormatter { + /** + * Logger pinned to this class so the glob warning emitted from {@link IgnoreGlobs} 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 + * ReviewDiffFormatter$IgnoreGlobs}. + */ + private static final Logger LOG = Logger.getLogger(ReviewDiffFormatter.class); + record GlobMatcher(PathMatcher primary, PathMatcher suffix) {} /** @@ -67,6 +76,35 @@ static IgnoreGlobs compile(List patterns) { return compiled.isEmpty() ? NONE : new IgnoreGlobs(compiled); } + /** + * Compiles the usable patterns, dropping blank and invalid ones. Warns through {@link #LOG}, + * pinned to the enclosing class, so the operator-facing category is unchanged by this method + * living here. + */ + private static List compileGlobMatchers(List patterns) { + if (patterns == null || patterns.isEmpty()) { + return List.of(); + } + var matchers = new ArrayList(); + for (String raw : patterns) { + if (raw == null || raw.isBlank()) { + continue; + } + var pattern = raw.trim(); + try { + var primary = FileSystems.getDefault().getPathMatcher("glob:" + pattern); + PathMatcher suffix = + pattern.startsWith("**/") + ? FileSystems.getDefault().getPathMatcher("glob:" + pattern.substring(3)) + : null; + matchers.add(new GlobMatcher(primary, suffix)); + } catch (InvalidPathException | PatternSyntaxException e) { + LOG.warnf(e, "Ignoring invalid ignored-files glob pattern: %s", pattern); + } + } + return List.copyOf(matchers); + } + /** * Global ∪ per-repo. Per-repo patterns are strictly additive: the union can only ever take more * files out of review scope, never put back a file the global list excludes. @@ -143,7 +181,7 @@ public ReviewDiffFormatter(ThrillhouseConfig config) { * *

An unparseable or empty per-repo list degrades to the global set — a repository can never * shrink or replace the deployment default, and a bad pattern in its list is dropped by {@link - * #compileGlobMatchers} rather than failing the review. + * IgnoreGlobs#compileGlobMatchers} rather than failing the review. */ IgnoreGlobs ignoreGlobs(List perRepoPatterns) { if (perRepoPatterns == null || perRepoPatterns.isEmpty()) { @@ -152,36 +190,6 @@ IgnoreGlobs ignoreGlobs(List perRepoPatterns) { return globalGlobs.union(IgnoreGlobs.compile(perRepoPatterns)); } - /** - * Deliberately not moved into {@link IgnoreGlobs}, 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 ReviewDiffFormatter$IgnoreGlobs} and - * silently break any log filter keyed on the category. - */ - private static List compileGlobMatchers(List patterns) { - if (patterns == null || patterns.isEmpty()) { - return List.of(); - } - var matchers = new ArrayList(); - for (String raw : patterns) { - if (raw == null || raw.isBlank()) { - continue; - } - var pattern = raw.trim(); - try { - var primary = FileSystems.getDefault().getPathMatcher("glob:" + pattern); - PathMatcher suffix = - pattern.startsWith("**/") - ? FileSystems.getDefault().getPathMatcher("glob:" + pattern.substring(3)) - : null; - matchers.add(new GlobMatcher(primary, suffix)); - } catch (InvalidPathException | PatternSyntaxException e) { - Log.warnf(e, "Ignoring invalid ignored-files glob pattern: %s", pattern); - } - } - return List.copyOf(matchers); - } - boolean isIgnored(String filename) { return globalGlobs.matches(filename); } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index fb178aed..d0f1a6d1 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -140,9 +140,9 @@ public FindingVerificationService( /** * The same absence claim worded as a missing step ("no sanitization", "never escaped"), with room * for a few words between the negator and the neutralizing verb. Read as a union with {@link - * #UNMITIGATED_ADJECTIVE} and {@link #MITIGATION_ABSENT_SUBJECT}; the three are one claim split - * across three patterns only because one alternation of every wording is more than the regex - * complexity budget allows. + * #UNMITIGATED_ADJECTIVE} and the pronoun-subject wording {@link #claimsAbsenceAsSubject + * recognizes}; the three are one claim recognized separately only because one alternation of + * every wording is more than the regex complexity budget allows. */ private static final Pattern MITIGATION_ABSENT = Pattern.compile( @@ -151,26 +151,43 @@ public FindingVerificationService( Pattern.CASE_INSENSITIVE); /** - * The absence claim worded with a pronoun subject ("Nothing sanitizes the value", "nobody escapes - * it before render"), so a subject-worded absence registers just like the step-worded one; {@link - * #MITIGATION_ASSERTED} excludes the same pronouns from its subject slot, and {@link - * #assertsMitigation} drops an auxiliary-order match they precede ("Nothing is sanitized"), so - * the two never read one sentence both ways. A defense noun as the verb's direct object flips the - * sentence's meaning — "Nothing escapes validation" says every value IS validated — so the verb - * is held to its finite and participle forms (the noun could otherwise re-match as the verb - * through the word gap) and the trailing lookahead rejects a defense-stemmed word within two - * words of the verb, so a modified or tool-named object ("nothing escapes heavy validation", - * "nothing escapes the sanitizer") is rejected too, keeping the over-fire direction closed (#594) - * while "nothing escapes the value" stays an absence claim — a defense noun further than two - * words out ("nothing escapes the value before validation") no longer flips the reading. Kept a - * separate pattern only because folding the pronouns into {@link #MITIGATION_ABSENT}'s negator - * alternation puts that pattern over the regex complexity budget. - */ - private static final Pattern MITIGATION_ABSENT_SUBJECT = + * A neutralizing verb in its finite or participle forms — the verb of the pronoun-subject absence + * claim {@link #claimsAbsenceAsSubject} recognizes ("Nothing sanitizes the value", "nobody + * escapes it before render"). The verb is held to these forms because a defense noun in the same + * stems must never re-match as the verb through the subject gap. One token of a three-pattern + * read: {@link #NEGATING_SUBJECT_BEFORE_VERB} supplies the subject, {@link + * #DEFENSE_OBJECT_AFTER_VERB} the direct-object exclusion — one wording held in three patterns + * only because the single pattern that spelled all three out (subject, gap, verb and a trailing + * lookahead) was more than the regex complexity budget allows. + */ + private static final Pattern ABSENCE_SUBJECT_VERB = Pattern.compile( - "\\b(nothing|nobody)\\s+(\\w+[\\s-]+){0,3}(sanitiz|escap|validat|parameteriz|encod)" - + "(es|ed|ing)\\b(?!\\s+(\\w+\\s+){0,2}(sanitiz|escap|validat|encod|filter))", - Pattern.CASE_INSENSITIVE); + "\\b(sanitiz|escap|validat|parameteriz|encod)(es|ed|ing)\\b", Pattern.CASE_INSENSITIVE); + + /** + * The negating pronoun subject within three words of the text BEFORE a {@link + * #ABSENCE_SUBJECT_VERB} match — anchored to that boundary with {@code \z}, it is the verbatim + * subject-and-gap prefix of the one-pattern form. {@link #MITIGATION_ASSERTED_SUBJECT} excludes + * the same pronouns from its subject slot, and {@link #assertsMitigation} drops an + * auxiliary-order match they precede ("Nothing is sanitized"), so the two never read one sentence + * both ways. + */ + private static final Pattern NEGATING_SUBJECT_BEFORE_VERB = + Pattern.compile("\\b(nothing|nobody)\\s+(\\w+[\\s-]+){0,3}\\z", Pattern.CASE_INSENSITIVE); + + /** + * A defense-stemmed word within two words AFTER the verb — the verbatim body of the one-pattern + * form's trailing negative lookahead, applied by {@link #claimsAbsenceAsSubject} at the match end + * via {@code lookingAt}. A defense noun as the verb's direct object flips the sentence's meaning + * — "Nothing escapes validation" says every value IS validated — so a modified or tool-named + * object ("nothing escapes heavy validation", "nothing escapes the sanitizer") is rejected too, + * keeping the over-fire direction closed (#594) while "nothing escapes the value" stays an + * absence claim — a defense noun further than two words out ("nothing escapes the value before + * validation") no longer flips the reading. + */ + private static final Pattern DEFENSE_OBJECT_AFTER_VERB = + Pattern.compile( + "\\s+(\\w+\\s+){0,2}(sanitiz|escap|validat|encod|filter)", Pattern.CASE_INSENSITIVE); /** * A finding that RULES THE SINK OUT rather than reporting it ("so there is no SQL injection", @@ -230,44 +247,66 @@ public FindingVerificationService( Pattern.CASE_INSENSITIVE); /** - * A finding that states the mitigation IS present ("it is escaped on render", "React escapes - * them"). An absence claim about one layer must not floor the class when the same text says + * A finding that states the mitigation IS present ("it is escaped on render"), worded on a + * be-copula. An absence claim about one layer must not floor the class when the same text says * another layer neutralizes the value. Negated forms are excluded, so "is not escaped" and "was - * never sanitized" stay absence claims instead of defeating themselves. The auxiliary-order - * alternative cannot see its own subject, so a pronoun-negated subject ("Nothing is sanitized") - * is excluded in {@link #assertsMitigation}, which drops a match directly preceded by a {@link - * #NEGATING_SUBJECT} — folding that into this pattern as lookbehinds would push it further over - * the regex complexity budget. + * never sanitized" stay absence claims instead of defeating themselves. This auxiliary-order + * wording cannot see its own subject, so a pronoun-negated subject ("Nothing is sanitized") is + * excluded in {@link #assertsMitigation}, which drops a match directly preceded by a {@link + * #NEGATING_SUBJECT} — folding that in as lookbehinds would push the pattern over the regex + * complexity budget. * - *

Left exactly as it stands, {@code {0,1}} and all: its complexity is over the analyzer's - * budget and cannot come under it without dropping a copula, a verb form or the one-word gap, - * each of which narrows what it matches (#594, #608). Rewriting the quantifier alone would move - * that unfixable finding onto this change's own lines while fixing nothing. + *

Read as a union with {@link #MITIGATION_ASSERTED_GET} and {@link + * #MITIGATION_ASSERTED_SUBJECT}: one assertion split across three patterns only because one + * alternation of every auxiliary and the subject-slot wording is more than the regex complexity + * budget allows — and none of the copulas, verb forms or the one-word gap may be dropped to buy + * it back, since each narrows what it matches (#594, #608). + */ + private static final Pattern MITIGATION_ASSERTED_BE = + Pattern.compile( + "\\b(is|are|was|were)\\s+(?!(no|not|never)\\b)(\\w+[\\s-]+)?" + + "(sanitiz|escap|validat|parameteriz|encod)(ed|es|ing)\\b", + Pattern.CASE_INSENSITIVE); + + /** + * The same auxiliary-order assertion on its remaining auxiliaries — the get-passive ("gets + * escaped"), the bare perfect ("been sanitized") and the copula-elided adverb ("already + * validated"); the second leg of {@link #MITIGATION_ASSERTED_BE}'s union. + */ + private static final Pattern MITIGATION_ASSERTED_GET = + Pattern.compile( + "\\b(gets|been|already)\\s+(?!(no|not|never)\\b)(\\w+[\\s-]+)?" + + "(sanitiz|escap|validat|parameteriz|encod)(ed|es|ing)\\b", + Pattern.CASE_INSENSITIVE); + + /** + * The same assertion worded with the mitigating layer in the subject slot ("React escapes them"); + * the third leg of {@link #MITIGATION_ASSERTED_BE}'s union. The negating pronouns are excluded + * from the subject slot so a pronoun-worded absence claim never reads as a mitigation. */ - private static final Pattern MITIGATION_ASSERTED = + private static final Pattern MITIGATION_ASSERTED_SUBJECT = Pattern.compile( - "\\b(is|are|was|were|gets|been|already)\\s+(?!(no|not|never)\\b)(\\w+[\\s-]+){0,1}" - + "(sanitiz|escap|validat|parameteriz|encod)(ed|es|ing)\\b" - + "|\\b(?!(no|not|never|nothing|nobody)\\b)\\w+\\s+" + "\\b(?!(no|not|never|nothing|nobody)\\b)\\w+\\s+" + "(sanitizes|escapes|validates|parameterizes|encodes)\\b", Pattern.CASE_INSENSITIVE); /** - * A pronoun subject that negates the clause it opens: a {@link #MITIGATION_ASSERTED} match - * starting right after one ("Nothing is sanitized before render") is the absence claim in - * auxiliary order, not a mitigation. Anchored to the end of the text before the match, so a - * pronoun elsewhere in the finding changes nothing. + * A pronoun subject that negates the clause it opens: a {@link #MITIGATION_ASSERTED_BE} or {@link + * #MITIGATION_ASSERTED_GET} match starting right after one ("Nothing is sanitized before + * render") is the absence claim in auxiliary order, not a mitigation. Anchored to the end of the + * text before the match, so a pronoun elsewhere in the finding changes nothing. */ private static final Pattern NEGATING_SUBJECT = Pattern.compile("\\b(nothing|nobody)\\s*$", Pattern.CASE_INSENSITIVE); /** * The mitigation asserted with do-support ("the framework does escape the value", "React did - * sanitize it"): emphatic, but still a statement of fact, and invisible to {@link - * #MITIGATION_ASSERTED}'s copula and subject-slot alternatives. The verb must follow the - * auxiliary directly, so the negated "does not escape" stays an absence claim; modals are - * deliberately absent — "should escape" is a recommendation, not an assertion. Kept a separate - * pattern because {@link #MITIGATION_ASSERTED} is already over the regex complexity budget. + * sanitize it"): emphatic, but still a statement of fact, and invisible to the copula and + * subject-slot patterns {@link #MITIGATION_ASSERTED_BE}, {@link #MITIGATION_ASSERTED_GET} and + * {@link #MITIGATION_ASSERTED_SUBJECT}. The verb must follow the auxiliary directly, so the + * negated "does not escape" stays an absence claim; modals are deliberately absent — "should + * escape" is a recommendation, not an assertion. Kept a separate pattern because folding it into + * the others would put them back over the regex complexity budget. */ private static final Pattern MITIGATION_DO_SUPPORTED = Pattern.compile( @@ -280,13 +319,13 @@ public FindingVerificationService( * whatever the finding asserts around it survives. * *

Both floor defeaters are assertion tests, and neither regex can carry mood: "If the feedback - * API sanitizes body on write, the exploit is neutralized" satisfies {@link #MITIGATION_ASSERTED} - * on the token pair "API sanitizes" even though the sentence goes on to reject the hypothesis - * ("but a sanitizer you cannot see is not a sanitizer"). That phrasing is not incidental — #575's - * review prompt REQUIRES a demonstrated-sink finding whose mitigating layer was not shown to name - * the exact layer to verify, so the instruction manufactures the wording the defeater then reads - * as a mitigation (#608). Scoping to the clause is the same narrowing the hedging scan already - * needed for the same reason. + * API sanitizes body on write, the exploit is neutralized" satisfies {@link + * #MITIGATION_ASSERTED_SUBJECT} on the token pair "API sanitizes" even though the sentence goes + * on to reject the hypothesis ("but a sanitizer you cannot see is not a sanitizer"). That + * phrasing is not incidental — #575's review prompt REQUIRES a demonstrated-sink finding whose + * mitigating layer was not shown to name the exact layer to verify, so the instruction + * manufactures the wording the defeater then reads as a mitigation (#608). Scoping to the clause + * is the same narrowing the hedging scan already needed for the same reason. * *

The clause ends at a real clause boundary — strong punctuation, or a coordinator that opens * the consequent ("but", "so", "then"...) — and deliberately NOT at a comma. A coordinator @@ -315,9 +354,9 @@ public FindingVerificationService( * the model put them. * *

Plain "should" is NOT a marker. Only inverted "Should the API sanitize ..." is a hypothesis, - * and its bare infinitive matches none of {@link #MITIGATION_ASSERTED}'s verb forms, so listing - * it bought nothing — while "It should be noted that the API sanitizes body on write" is an - * assertion, and treating it as a hypothesis hid a real mitigation from the defeater. + * and its bare infinitive matches none of the mitigation-asserted patterns' verb forms, so + * listing it bought nothing — while "It should be noted that the API sanitizes body on write" is + * an assertion, and treating it as a hypothesis hid a real mitigation from the defeater. */ private static final Pattern CONDITIONAL_CLAUSE = Pattern.compile( @@ -671,16 +710,47 @@ && claimsNothingNeutralizesIt(text) } /** - * A {@link #MITIGATION_ASSERTED} or {@link #MITIGATION_DO_SUPPORTED} hit, unless the match opens - * with a negating pronoun subject: "Nothing is sanitized" is the absence claim in auxiliary - * order, and {@link #MITIGATION_ASSERTED}'s first alternative starts at the auxiliary so it never - * sees the subject. + * A hit from any of the mitigation-asserted patterns or {@link #MITIGATION_DO_SUPPORTED}, unless + * the match opens with a negating pronoun subject: "Nothing is sanitized" is the absence claim in + * auxiliary order, and {@link #MITIGATION_ASSERTED_BE} and {@link #MITIGATION_ASSERTED_GET} start + * at the auxiliary so they never see the subject. */ private static boolean assertsMitigation(String asserted) { - return hasUnnegatedMatch(MITIGATION_ASSERTED, asserted) + return hasUnnegatedAssertedMatch(asserted) || hasUnnegatedMatch(MITIGATION_DO_SUPPORTED, asserted); } + /** + * {@link #hasUnnegatedMatch} over the three mitigation-asserted patterns walked as ONE + * alternation: at each step the leftmost match across the three wins — a position tie in the + * patterns' declared order — and scanning resumes after it, exactly where the single pattern's + * own scan resumed. Checking the patterns one whole pass at a time instead would also surface + * matches the one-alternation scan stepped over, and a defeater must not fire on text its + * one-pattern form never read as a mitigation. + */ + private static boolean hasUnnegatedAssertedMatch(String asserted) { + Matcher be = MITIGATION_ASSERTED_BE.matcher(asserted); + Matcher get = MITIGATION_ASSERTED_GET.matcher(asserted); + Matcher subject = MITIGATION_ASSERTED_SUBJECT.matcher(asserted); + var from = 0; + while (from <= asserted.length()) { + Matcher leftmost = null; + for (Matcher wording : new Matcher[] {be, get, subject}) { + if (wording.find(from) && (leftmost == null || wording.start() < leftmost.start())) { + leftmost = wording; + } + } + if (leftmost == null) { + return false; + } + if (!NEGATING_SUBJECT.matcher(asserted.substring(0, leftmost.start())).find()) { + return true; + } + from = leftmost.end(); + } + return false; + } + private static boolean hasUnnegatedMatch(Pattern mitigation, String asserted) { Matcher asserts = mitigation.matcher(asserted); while (asserts.find()) { @@ -697,11 +767,31 @@ private static boolean namesInjectionSink(String text) { || (SQL.matcher(text).find() && STRING_BUILT.matcher(text).find()); } - /** The absence claim in any of its three wordings; one claim, three patterns. */ + /** The absence claim in any of its three wordings; one claim, three recognizers. */ private static boolean claimsNothingNeutralizesIt(String text) { return UNMITIGATED_ADJECTIVE.matcher(text).find() || MITIGATION_ABSENT.matcher(text).find() - || MITIGATION_ABSENT_SUBJECT.matcher(text).find(); + || claimsAbsenceAsSubject(text); + } + + /** + * The absence claim worded with a pronoun subject: a {@link #ABSENCE_SUBJECT_VERB} token whose + * text up to the token satisfies {@link #NEGATING_SUBJECT_BEFORE_VERB} and whose text from the + * token's end does not open on a {@link #DEFENSE_OBJECT_AFTER_VERB}. Checking every verb token + * against an anchored prefix and an anchored trailer decides exactly the parses the one-pattern + * form decided through backtracking and its trailing lookahead — including the parse where a + * defense noun flips an earlier verb's reading while a later verb in the same subject gap stays + * clean — so the split changes what the analyzer counts, not what the recognizer accepts. + */ + private static boolean claimsAbsenceAsSubject(String text) { + Matcher verb = ABSENCE_SUBJECT_VERB.matcher(text); + while (verb.find()) { + if (NEGATING_SUBJECT_BEFORE_VERB.matcher(text.substring(0, verb.start())).find() + && !DEFENSE_OBJECT_AFTER_VERB.matcher(text.substring(verb.end())).lookingAt()) { + return true; + } + } + return false; } /** diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/dashboard/AuthResourceTestFixtures.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/dashboard/AuthResourceTestFixtures.java index 99a0eddc..9cce9237 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/dashboard/AuthResourceTestFixtures.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/dashboard/AuthResourceTestFixtures.java @@ -64,9 +64,12 @@ static void wireDefaultOAuth( when(dashboardConfig.clientSecret()).thenReturn(Optional.of("test-secret")); when(dashboardConfig.redirectUri()).thenReturn("http://localhost:8080/api/auth/callback"); when(dashboardConfig.oauthUrl()).thenReturn("https://github.com/login/oauth"); - when(config.github()).thenReturn(mock(ThrillhouseConfig.GitHubConfig.class)); - when(config.review()).thenReturn(mock(ThrillhouseConfig.ReviewConfig.class)); - when(config.ai()).thenReturn(mock(ThrillhouseConfig.AiPricingConfig.class)); + var gitHub = mock(ThrillhouseConfig.GitHubConfig.class); + var review = mock(ThrillhouseConfig.ReviewConfig.class); + var aiPricing = mock(ThrillhouseConfig.AiPricingConfig.class); + when(config.github()).thenReturn(gitHub); + when(config.review()).thenReturn(review); + when(config.ai()).thenReturn(aiPricing); when(config.httpRequestTimeout()).thenReturn(Duration.ofSeconds(10)); } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index 4e5ce593..de8b2433 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java @@ -299,6 +299,26 @@ void masksAnythingShapedLikeACredential() { assertEquals(4, body.split("\\*\\*\\*", -1).length - 1, body); } + /** + * The overlap the credential javadoc pins: a token prefix whose word-character run swallows a + * following "Bearer" is masked as ONE leftmost match, so the bearer wording never survives as + * an unmasked shape of its own. + */ + @Test + void masksAnOverlappingTokenAndBearerAsTheLeftmostMatch() { + var body = loggedBody(outbound(401, "ghp_abcdefgBearer abcdefghij0123456789")); + + assertEquals("*** abcdefghij0123456789", body); + } + + /** The other direction: a bearer value that is itself a token keeps nothing past the mask. */ + @Test + void masksABearerCarryingATokenValueAsOneLeftmostMatch() { + var body = loggedBody(outbound(401, "Bearer ghp_abcdefghij.tail and more")); + + assertEquals("*** and more", body); + } + @Test void capsAnOverLongBodySoOneFailureCannotFloodTheLog() { var body = loggedBody(outbound(500, "x".repeat(4_000))); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/BlockingStrictnessTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/BlockingStrictnessTest.java index 04a0c712..6928f0fd 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/BlockingStrictnessTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/BlockingStrictnessTest.java @@ -15,6 +15,7 @@ */ package dev.thiagogonzaga.thrillhousebot.review; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -61,7 +62,7 @@ private static Finding finding(RiskLevel risk, Confidence confidence) { void withheldByConfidenceIsolatesTheHedgeAsTheReason( BlockingStrictness mode, RiskLevel risk, Confidence confidence, boolean withheld) { var f = finding(risk, confidence); - assertTrue(withheld == mode.withheldByConfidence(f), mode + " " + risk + "/" + confidence); + assertEquals(withheld, mode.withheldByConfidence(f), mode + " " + risk + "/" + confidence); // Withheld and blocking are mutually exclusive by construction: a withheld finding is one the // gate rejected, so a mode can never claim both about the same finding. assertFalse(mode.withheldByConfidence(f) && mode.isBlocking(f)); @@ -86,7 +87,7 @@ void withheldByConfidenceIsolatesTheHedgeAsTheReason( }) void isBlockingKeepsItsPreSplitOutcomes( BlockingStrictness mode, RiskLevel risk, Confidence confidence, boolean blocks) { - assertTrue(blocks == mode.isBlocking(finding(risk, confidence))); + assertEquals(blocks, mode.isBlocking(finding(risk, confidence))); } @ParameterizedTest diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java index 783c1dab..54530ed2 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java @@ -1185,7 +1185,7 @@ private static ReviewResponse.Finding reactConditionalXss(String risk, String co @Test void floorsAnInjectionSinkFindingWhoseOnlySanitizerMentionIsAConditionalItRejects() { // #608: the finding argues its own severity is critical and publishes MEDIUM, because - // MITIGATION_ASSERTED read "API sanitizes" out of the conditional "If the feedback API + // MITIGATION_ASSERTED_SUBJECT read "API sanitizes" out of the conditional "If the feedback API // sanitizes body on write" — a hypothesis the very next clause rejects. #575's prompt REQUIRES // that clause on this class, so the defeater fired on wording the prompt guarantees is there. when(reviewConfig.verifierEnabled()).thenReturn(false); @@ -1318,8 +1318,8 @@ void floorsAFindingWhoseAbsenceClaimIsWordedAsNothingSanitizes() { @Test void floorsAFindingWhoseAbsenceClaimIsWordedAsNothingIsSanitized() { - // The auxiliary-order twin of the pronoun-subject absence: MITIGATION_ASSERTED's first - // alternative starts at "is" and never sees the negating subject, so "Nothing is sanitized" + // The auxiliary-order twin of the pronoun-subject absence: MITIGATION_ASSERTED_BE starts at + // "is" and never sees the negating subject, so "Nothing is sanitized" // read as a mitigation and defeated the very floor the pronoun negators enable. when(reviewConfig.verifierEnabled()).thenReturn(false); ReviewResponse original = @@ -1342,8 +1342,9 @@ void floorsAFindingWhoseAbsenceClaimIsWordedAsNothingIsSanitized() { @Test void doesNotFloorWhenADoSupportedMitigationFollowsThePronounAbsence() { - // "does escape" is emphatic but still a statement of fact, invisible to MITIGATION_ASSERTED's - // copula and subject-slot shapes. With the pronoun absence match dropped by NEGATING_SUBJECT, + // "does escape" is emphatic but still a statement of fact, invisible to the + // MITIGATION_ASSERTED_* copula and subject-slot shapes. With the pronoun absence match dropped + // by NEGATING_SUBJECT, // this text would otherwise floor although it says another layer neutralizes the value — // the over-fire direction (#594). when(reviewConfig.verifierEnabled()).thenReturn(false); From ba2a446969c1725a32eecc41af63535cc10aad13 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 16:06:36 -0300 Subject: [PATCH 2/6] test: cover every branch of the split-pattern recognizers --- .../review/ai/FindingVerificationService.java | 4 +- .../github/GitHubApiErrorTest.java | 6 ++ .../ai/FindingVerificationServiceTest.java | 74 +++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index d0f1a6d1..64cf205e 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -733,7 +733,7 @@ private static boolean hasUnnegatedAssertedMatch(String asserted) { Matcher get = MITIGATION_ASSERTED_GET.matcher(asserted); Matcher subject = MITIGATION_ASSERTED_SUBJECT.matcher(asserted); var from = 0; - while (from <= asserted.length()) { + while (true) { Matcher leftmost = null; for (Matcher wording : new Matcher[] {be, get, subject}) { if (wording.find(from) && (leftmost == null || wording.start() < leftmost.start())) { @@ -746,9 +746,9 @@ private static boolean hasUnnegatedAssertedMatch(String asserted) { if (!NEGATING_SUBJECT.matcher(asserted.substring(0, leftmost.start())).find()) { return true; } + // No wording matches an empty string, so the scan always advances. from = leftmost.end(); } - return false; } private static boolean hasUnnegatedMatch(Pattern mitigation, String asserted) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index de8b2433..f498c94f 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java @@ -319,6 +319,12 @@ void masksABearerCarryingATokenValueAsOneLeftmostMatch() { assertEquals("*** and more", body); } + /** A body that is nothing but a token prefix shape, masked to its very last character. */ + @Test + void masksATokenStandingAlone() { + assertEquals("only ***", loggedBody(outbound(401, "only ghp_0123456789abcd"))); + } + @Test void capsAnOverLongBodySoOneFailureCannotFloodTheLog() { var body = loggedBody(outbound(500, "x".repeat(4_000))); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java index 54530ed2..c649cca2 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java @@ -1316,6 +1316,31 @@ void floorsAFindingWhoseAbsenceClaimIsWordedAsNothingSanitizes() { assertEquals("low", result.findings().get(0).confidence()); } + @Test + void floorsAPronounAbsenceEvenWhenAnEarlierParticipleHasNoSubjectAtAll() { + // "HTML-escaped" is a neutralizing-verb token with no pronoun subject before it and no copula + // either — not an absence claim and not an asserted mitigation. The pronoun-subject scan must + // step over it and still read the "nothing sanitizes" that follows. + when(reviewConfig.verifierEnabled()).thenReturn(false); + ReviewResponse original = + response( + new ReviewResponse.Finding( + "medium", + "low", + "src/components/Comment.tsx", + 14, + "User comment written to innerHTML", + "User text, HTML-escaped only in unit fixtures, reaches innerHTML; nothing" + + " sanitizes it in production.", + null, + null)); + + var result = service.verify(SESSION, original, "diff", "stack", ""); + + assertEquals("high", result.findings().get(0).risk()); + assertEquals("low", result.findings().get(0).confidence()); + } + @Test void floorsAFindingWhoseAbsenceClaimIsWordedAsNothingIsSanitized() { // The auxiliary-order twin of the pronoun-subject absence: MITIGATION_ASSERTED_BE starts at @@ -1367,6 +1392,55 @@ void doesNotFloorWhenADoSupportedMitigationFollowsThePronounAbsence() { assertEquals("low", result.findings().get(0).risk()); } + @Test + void doesNotFloorWhenANegatedAbsenceIsFollowedByASubjectSlotMitigation() { + // The mitigation-asserted union walked leftmost-first: the negated "Nothing is sanitized" + // match is stepped over and the later "React escapes" subject-slot assertion still defeats + // the floor — the same reading the single-alternation pattern gave this text. + when(reviewConfig.verifierEnabled()).thenReturn(false); + ReviewResponse original = + response( + new ReviewResponse.Finding( + "low", + "high", + "src/components/Comment.tsx", + 14, + "User comment written to innerHTML", + "Nothing is sanitized on write, yet React escapes the value at render, so the" + + " sink never sees live markup.", + null, + null)); + + var result = service.verify(SESSION, original, "diff", "stack", ""); + + assertSame(original, result); + assertEquals("low", result.findings().get(0).risk()); + } + + @Test + void doesNotFloorWhenTheSubjectSlotMitigationPrecedesTheCopulaOne() { + // Same union, opposite order: the subject-slot match sits before the copula match, so the + // leftmost pick has to displace the copula candidate rather than keep it. + when(reviewConfig.verifierEnabled()).thenReturn(false); + ReviewResponse original = + response( + new ReviewResponse.Finding( + "low", + "high", + "src/components/Comment.tsx", + 14, + "User comment written to innerHTML", + "React escapes the value at render because it is sanitized by the framework, so" + + " unsanitized markup never reaches the sink.", + null, + null)); + + var result = service.verify(SESSION, original, "diff", "stack", ""); + + assertSame(original, result); + assertEquals("low", result.findings().get(0).risk()); + } + @Test void doesNotReadDoesNotProhibitSqlInjectionAsASinkDenial() { // "prohibit" belongs to the same warding-off family as "prevent": the negation targets the From 9e9736890fc7e63aac72a92c5591890e90f8e4b1 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 16:19:47 -0300 Subject: [PATCH 3/6] perf(review): scan the pronoun-absence prefix and trailer as regions, not substrings --- .../review/ai/FindingVerificationService.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index 64cf205e..3253dd36 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -785,9 +785,14 @@ private static boolean claimsNothingNeutralizesIt(String text) { */ private static boolean claimsAbsenceAsSubject(String text) { Matcher verb = ABSENCE_SUBJECT_VERB.matcher(text); + // Regions instead of substrings, so stepping through the verb tokens never copies the text; + // with the matchers' default opaque and anchoring bounds a region IS the whole input to the + // pattern, so {@code \z} stops at the region end and the semantics stay those of a substring. + Matcher subject = NEGATING_SUBJECT_BEFORE_VERB.matcher(text); + Matcher object = DEFENSE_OBJECT_AFTER_VERB.matcher(text); while (verb.find()) { - if (NEGATING_SUBJECT_BEFORE_VERB.matcher(text.substring(0, verb.start())).find() - && !DEFENSE_OBJECT_AFTER_VERB.matcher(text.substring(verb.end())).lookingAt()) { + if (subject.region(0, verb.start()).find() + && !object.region(verb.end(), text.length()).lookingAt()) { return true; } } From b8ebae4c8489c5158e12cda83c261dcef2c0653b Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 17:13:36 -0300 Subject: [PATCH 4/6] perf(review): walk the asserted-mitigation union with cached candidates and region-bound negation checks --- .../review/ai/FindingVerificationService.java | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index 3253dd36..165cae85 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -729,25 +729,46 @@ private static boolean assertsMitigation(String asserted) { * one-pattern form never read as a mitigation. */ private static boolean hasUnnegatedAssertedMatch(String asserted) { - Matcher be = MITIGATION_ASSERTED_BE.matcher(asserted); - Matcher get = MITIGATION_ASSERTED_GET.matcher(asserted); - Matcher subject = MITIGATION_ASSERTED_SUBJECT.matcher(asserted); + Matcher[] wordings = { + MITIGATION_ASSERTED_BE.matcher(asserted), + MITIGATION_ASSERTED_GET.matcher(asserted), + MITIGATION_ASSERTED_SUBJECT.matcher(asserted) + }; + Matcher negation = NEGATING_SUBJECT.matcher(asserted); + var starts = new int[] {-1, -1, -1}; + var ends = new int[wordings.length]; + var exhausted = new boolean[wordings.length]; var from = 0; while (true) { - Matcher leftmost = null; - for (Matcher wording : new Matcher[] {be, get, subject}) { - if (wording.find(from) && (leftmost == null || wording.start() < leftmost.start())) { - leftmost = wording; + var leftmost = -1; + for (var i = 0; i < wordings.length; i++) { + if (!exhausted[i] && starts[i] < from) { + // Only a consumed or overtaken candidate is re-found; one at or past `from` is still + // that wording's next match, since the find that produced it proved nothing of that + // wording starts before it. This keeps the walk a single pass per wording instead of + // rescanning all three from every resume point. + if (wordings[i].find(from)) { + starts[i] = wordings[i].start(); + ends[i] = wordings[i].end(); + } else { + exhausted[i] = true; + } + } + if (!exhausted[i] && (leftmost == -1 || starts[i] < starts[leftmost])) { + leftmost = i; } } - if (leftmost == null) { + if (leftmost == -1) { return false; } - if (!NEGATING_SUBJECT.matcher(asserted.substring(0, leftmost.start())).find()) { + // A region instead of a substring, so dropping a negated match never copies the text; the + // pattern's $ honors the region end under the matcher's default anchoring bounds. + if (!negation.region(0, starts[leftmost]).find()) { return true; } // No wording matches an empty string, so the scan always advances. - from = leftmost.end(); + from = ends[leftmost]; + starts[leftmost] = -1; } } From 66cb2290ec6039855cab4e0b599a13931822e384 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 17:27:31 -0300 Subject: [PATCH 5/6] refactor(review): split the union walk's candidate refresh out of the negation loop --- .../review/ai/FindingVerificationService.java | 51 ++++++++++++------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index 165cae85..31e75c84 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -737,27 +737,9 @@ private static boolean hasUnnegatedAssertedMatch(String asserted) { Matcher negation = NEGATING_SUBJECT.matcher(asserted); var starts = new int[] {-1, -1, -1}; var ends = new int[wordings.length]; - var exhausted = new boolean[wordings.length]; var from = 0; while (true) { - var leftmost = -1; - for (var i = 0; i < wordings.length; i++) { - if (!exhausted[i] && starts[i] < from) { - // Only a consumed or overtaken candidate is re-found; one at or past `from` is still - // that wording's next match, since the find that produced it proved nothing of that - // wording starts before it. This keeps the walk a single pass per wording instead of - // rescanning all three from every resume point. - if (wordings[i].find(from)) { - starts[i] = wordings[i].start(); - ends[i] = wordings[i].end(); - } else { - exhausted[i] = true; - } - } - if (!exhausted[i] && (leftmost == -1 || starts[i] < starts[leftmost])) { - leftmost = i; - } - } + var leftmost = leftmostWording(wordings, starts, ends, from); if (leftmost == -1) { return false; } @@ -772,6 +754,37 @@ private static boolean hasUnnegatedAssertedMatch(String asserted) { } } + /** A wording 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 wording holding the leftmost live candidate at or past {@code from}, or -1 + * when every wording is out of matches; a position tie keeps the lowest index, the patterns' + * declared order. Only a consumed or overtaken candidate is re-found — one at or past {@code + * from} is still that wording's next match, since the find that produced it proved nothing of + * that wording starts before it — so the walk stays a single pass per wording instead of + * rescanning all three from every resume point. + */ + private static int leftmostWording(Matcher[] wordings, int[] starts, int[] ends, int from) { + var leftmost = -1; + var best = NO_MORE_MATCHES; + for (var i = 0; i < wordings.length; i++) { + if (starts[i] < from) { + if (wordings[i].find(from)) { + starts[i] = wordings[i].start(); + ends[i] = wordings[i].end(); + } else { + starts[i] = NO_MORE_MATCHES; + } + } + if (starts[i] < best) { + best = starts[i]; + leftmost = i; + } + } + return leftmost; + } + private static boolean hasUnnegatedMatch(Pattern mitigation, String asserted) { Matcher asserts = mitigation.matcher(asserted); while (asserts.find()) { From c447436100ece480a85be89fc1faf989fd623d2c Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 17:44:22 -0300 Subject: [PATCH 6/6] perf(github): redact with one live candidate per credential shape instead of rescanning the tail --- .../thrillhousebot/github/GitHubApiError.java | 52 +++++++++++++++---- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index 9d50cf16..5350cf0b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -232,24 +232,56 @@ private static Duration atLeastZero(Duration delay) { * empty string, so every step advances. */ private static String redactCredentials(String text) { - Matcher prefix = CREDENTIAL_SHAPED_PREFIX.matcher(text); - Matcher value = CREDENTIAL_SHAPED_VALUE.matcher(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 (from < text.length()) { - boolean prefixFound = prefix.find(from); - boolean valueFound = value.find(from); - if (!prefixFound && !valueFound) { + while (true) { + var leftmost = leftmostShape(shapes, starts, ends, from); + if (leftmost == -1) { break; } - Matcher leftmost = - prefixFound && (!valueFound || prefix.start() <= value.start()) ? prefix : value; - out.append(text, from, leftmost.start()).append(REDACTED); - from = leftmost.end(); + 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