From cd8bd800a7c4887e7e5206b944fdc4efc72a25c7 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 9 Aug 2026 14:58:51 +0000 Subject: [PATCH 1/6] fix(review): charge the config-key fetch budget per attempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In collectDefinitions the fetched++ counter sat inside `if (content != null)`, and fetchContent returns null on an exception, an absent body, AND blank content. Under a blanket failure (rate limit, expired token) or a repository of blank/unreadable candidates the loop therefore issued one serial API call per candidate — 300+ in a large monorepo — against a MAX_FILES_FETCHED of 8, deepening the throttle it had already hit. The javadoc already promised the walk runs "until the fetch budget is spent". The budget is now charged per attempt: fetched++ moves above the null check. Refs audit F3 --- .../review/ConfigKeyContextResolver.java | 6 +++++- .../review/ConfigKeyContextResolverTest.java | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java index 8f89117a..b614d706 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java @@ -363,9 +363,13 @@ private List collectDefinitions( if (fetched >= MAX_FILES_FETCHED || found.size() >= MAX_KEYS_RENDERED) { break; } + // Charge the budget per ATTEMPT, not per success: fetchContent returns null on an exception, + // an absent body, and blank content alike, so under a blanket failure (rate limit, expired + // token) or a repo of blank/unreadable candidates a per-success budget would issue one serial + // API call per candidate — hundreds in a large monorepo — deepening the very throttle it hit. + fetched++; var content = fetchContent(auth, owner, repo, path, ref); if (content != null) { - fetched++; absorbFile(path, content.split("\n", -1), normalized, found); } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java index fb4082ff..e8010dd8 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java @@ -651,6 +651,26 @@ void shouldNotFetchMoreFilesThanTheBudgetAllows() { .getFileContent(any(), any(), any(), any(), any(), eq("headsha")); } + @Test + void shouldChargeTheFetchBudgetPerAttemptNotPerSuccess() { + // Every candidate is blank, so fetchContent returns null for each. Under a blanket failure + // (rate limit, expired token) or a repo of blank candidates the budget must still be spent + // per + // ATTEMPT, or the loop issues one serial API call per candidate against a budget of 8. + var paths = new ArrayList(); + for (int i = 0; i < ConfigKeyContextResolver.MAX_FILES_FETCHED + 4; i++) { + paths.add("module" + i + "/application.properties"); + } + givenRepository(paths.toArray(new String[0])); + for (String path : paths) { + givenFile(path, " \n \n"); + } + + assertEquals("", resolve(List.of(docDiff(".env.example", "WEBHOOK_DEDUP_TTL=24h")))); + verify(prClient, times(ConfigKeyContextResolver.MAX_FILES_FETCHED)) + .getFileContent(any(), any(), any(), any(), any(), eq("headsha")); + } + @Test void shouldCapRenderedKeysAndTotalCharacters() { var properties = new StringBuilder(); From bf3f24343d118103513db34fe8445083e5ed6c04 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 9 Aug 2026 15:03:16 +0000 Subject: [PATCH 2/6] fix(review): stop treating hostnames and Java FQNs as config keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PROPERTY_TOKEN (three-plus lowercase dotted segments) matched hostnames like api.github.com and reverse-domain package names like org.eclipse.microprofile.rest.client. These resolve against unrelated config lines and could fill all five MAX_KEYS_RENDERED slots, crowding out the actually-documented key. collectTokens also ran ENV_TOKEN to exhaustion before PROPERTY_TOKEN, so the javadoc's "first-mention order" was false. Tokens are now matched by a single combined KEY_TOKEN pattern in one positional pass, so first-mention order holds. A dotted token is dropped when it sits inside a URL on the source line, or when its first or last segment is a common TLD — a hostname trails with the TLD, a reverse-domain FQN leads with it. Only tokens from the diff ever become keys, so the survivors are already the documented ones. Refs audit F4 --- .../review/ConfigKeyContextResolver.java | 62 +++++++++++++++++-- .../review/ConfigKeyContextResolverTest.java | 29 +++++++++ 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java index b614d706..3c32bc7a 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java @@ -123,6 +123,24 @@ public class ConfigKeyContextResolver { Pattern.compile( "\\b[a-z][a-z0-9]{0,63}(?:\\.[a-z0-9]" + SEG + "(?:-[a-z0-9]" + SEG + "){0,8}){2,16}\\b"); + /** + * Both key shapes in one pattern, matched in a single left-to-right pass so tokens surface in the + * order the line mentions them. The two alternatives start on disjoint character classes + * (uppercase vs lowercase), so there is no ambiguity and no extra backtracking beyond what each + * bounded pattern already does. + */ + private static final Pattern KEY_TOKEN = + Pattern.compile(ENV_TOKEN.pattern() + "|" + PROPERTY_TOKEN.pattern()); + + /** + * Trailing/leading dotted segments that mark a token as a hostname or a reverse-domain Java FQN + * rather than a configuration key. A hostname trails with the TLD ({@code api.github.com}); a + * reverse-domain package name leads with it ({@code org.eclipse.microprofile.rest.client}). No + * real config key begins or ends with a bare TLD segment, so both ends are checked. + */ + private static final Set TLD_SEGMENTS = + Set.of("com", "org", "net", "io", "co", "dev", "gov", "edu", "info", "app", "ai", "me", "us"); + /** Extensions of source files that can hold a config mapping. */ private static final Set SOURCE_EXTENSIONS = Set.of("java", "kt", "py", "ts", "js", "go", "rb", "rs"); @@ -203,14 +221,46 @@ static List extractTokens(List files) } private static void collectTokens(String addedText, Set tokens) { - var env = ENV_TOKEN.matcher(addedText); - while (env.find() && tokens.size() < MAX_TOKENS) { - tokens.add(env.group()); + var matcher = KEY_TOKEN.matcher(addedText); + while (matcher.find() && tokens.size() < MAX_TOKENS) { + var token = matcher.group(); + if (looksLikeConfigKey(token, addedText, matcher.start())) { + tokens.add(token); + } + } + } + + /** + * Whether a matched token is plausibly a configuration key rather than a hostname or a Java + * fully-qualified name that happens to share the dotted-lowercase shape and would otherwise + * resolve against unrelated config lines and crowd out the actually-documented key. An {@code + * UPPER_SNAKE} env name is always a key. A dotted property token is rejected when it sits inside + * a URL on the source line, or when its first or last segment is a common TLD. + */ + private static boolean looksLikeConfigKey(String token, String line, int start) { + if (token.indexOf('.') < 0) { + return true; + } + if (isInsideUrl(line, start)) { + return false; + } + var segments = token.split("\\."); + return !TLD_SEGMENTS.contains(segments[0]) + && !TLD_SEGMENTS.contains(segments[segments.length - 1]); + } + + /** Whether the whitespace-delimited run of {@code text} containing offset {@code at} is a URL. */ + private static boolean isInsideUrl(String text, int at) { + var from = at; + while (from > 0 && !Character.isWhitespace(text.charAt(from - 1))) { + from--; } - var property = PROPERTY_TOKEN.matcher(addedText); - while (property.find() && tokens.size() < MAX_TOKENS) { - tokens.add(property.group()); + var to = at; + while (to < text.length() && !Character.isWhitespace(text.charAt(to))) { + to++; } + var word = text.substring(from, to); + return word.contains("://") || word.contains("www."); } /** The patch's added content ({@code +} lines, excluding the {@code +++} file header). */ diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java index e8010dd8..306ab4d4 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java @@ -162,6 +162,35 @@ void shouldMatchRealKeysWithoutUnboundedBacktrackingOnAdversarialInput() { assertTrue(elapsedMs < 5_000, () -> "token extraction took " + elapsedMs + "ms"); } + @Test + void shouldRejectHostnamesAndFqnsAndPreserveFirstMentionOrder() { + // A doc line mixing a URL host, a Java FQN, a real property key and a real env name. The + // hostname (trailing TLD, inside a URL) and the FQN (leading reverse-domain TLD) must be + // dropped, and the survivors kept in the order the line mentions them — property first. + var tokens = + ConfigKeyContextResolver.extractTokens( + List.of( + docDiff( + "README.md", + "reach https://api.github.com/repos then set" + + " `thrillhousebot.review.ci-gating`; the" + + " org.eclipse.microprofile.rest.client package reads" + + " `THRILLHOUSEBOT_REVIEW_CI_GATING`"))); + + assertFalse( + tokens.contains("api.github.com"), + () -> "a hostname inside a URL is not a config key: " + tokens); + assertFalse( + tokens.contains("org.eclipse.microprofile.rest.client"), + () -> "a Java fully-qualified name is not a config key: " + tokens); + assertEquals( + List.of("thrillhousebot.review.ci-gating", "THRILLHOUSEBOT_REVIEW_CI_GATING"), + tokens, + () -> + "the property is mentioned before the env name; order must follow position: " + + tokens); + } + @Test void shouldNotMistakeFilenamesForPropertyKeys() { var tokens = From 5108f8f98f233bf413f998f1048d1e86b9d51d49 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 9 Aug 2026 15:07:03 +0000 Subject: [PATCH 3/6] fix(review): prefer an exact config-key match over a generic suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lineDefines fell back to a two-segment suffix (MIN_SUFFIX_SEGMENTS = 2), so THRILLHOUSEBOT_HTTP_PORT degraded to HTTP_PORT and matched quarkus.http.port — an unrelated key's definition rendered as though it were the documented key's. And the exact-vs-suffix choice was made per line, so a fuzzy hit on line 1 beat an exact hit on line 100. The suffix floor is raised to 3, so a short key no longer degrades into a too-generic tail. lineDefines is split into an exact and a suffix predicate, and snippetsFor now prefers an exact whole-key match anywhere in the file over any suffix match; suffix matches are used only when the file holds no exact match, and each is labelled so the model can discount it. Refs audit F5 --- .../review/ConfigKeyContextResolver.java | 67 +++++++++++++++---- .../review/ConfigKeyContextResolverTest.java | 39 +++++++++++ 2 files changed, 94 insertions(+), 12 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java index 3c32bc7a..4ed5c38f 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java @@ -89,8 +89,17 @@ public class ConfigKeyContextResolver { static final int CONTEXT_LINES_AFTER = 2; - /** A key token must keep at least this many segments when prefix segments are dropped. */ - private static final int MIN_SUFFIX_SEGMENTS = 2; + /** + * A key token must keep at least this many segments when prefix segments are dropped. Three, not + * two: a two-segment tail is too generic — {@code THRILLHOUSEBOT_HTTP_PORT} degraded to {@code + * HTTP_PORT} matches {@code quarkus.http.port}, an unrelated key's definition rendered as though + * it were this one's. + */ + private static final int MIN_SUFFIX_SEGMENTS = 3; + + /** Appended to a snippet found only by dropping prefix segments, so the model can discount it. */ + private static final String SUFFIX_MATCH_NOTE = + "(matched by dropping key prefix segments — may name a different key)"; /** Heading of the rendered section. Package-private so tests and callers agree on it. */ static final String SECTION_HEADING = @@ -474,17 +483,34 @@ private String fetchContent(String auth, String owner, String repo, String path, } } - /** Rendered definition sites for one normalized token inside one file. */ + /** + * Rendered definition sites for one normalized token inside one file. An exact whole-key match + * anywhere in the file is preferred over a suffix match anywhere in it, so a fuzzy prefix-dropped + * hit on an early line can no longer beat the key's real definition further down. Only when the + * file holds no exact match at all are suffix matches used, and each is labelled so the model can + * discount it. + */ static List snippetsFor(String path, String[] lines, String normalizedToken) { + var exact = matchingSnippets(path, lines, normalizedToken, true); + return exact.isEmpty() ? matchingSnippets(path, lines, normalizedToken, false) : exact; + } + + private static List matchingSnippets( + String path, String[] lines, String normalizedToken, boolean exactOnly) { var snippets = new ArrayList(); var lastRendered = -1; for (var i = 0; i < lines.length && snippets.size() < MAX_SNIPPETS_PER_KEY; i++) { var from = Math.max(0, i - CONTEXT_LINES_BEFORE); + var defines = + exactOnly + ? lineDefinesExactly(lines[i], normalizedToken) + : lineDefinesBySuffix(lines[i], normalizedToken); // from > lastRendered skips a match the previous window already shows: adjacent matches (a // property and its override on consecutive lines) share one snippet rather than repeating it. - if (lineDefines(lines[i], normalizedToken) && from > lastRendered) { + if (defines && from > lastRendered) { var to = Math.min(lines.length - 1, i + CONTEXT_LINES_AFTER); - snippets.add(renderSnippet(path, lines, from, to)); + var snippet = renderSnippet(path, lines, from, to); + snippets.add(exactOnly ? snippet : snippet + "\n" + SUFFIX_MATCH_NOTE); lastRendered = to; } } @@ -521,23 +547,40 @@ static String truncate(String value, int limit) { } /** - * Whether a line defines the key. The normalized line is searched for the whole key first — the - * literal env name of an explicit {@code ${ENV:default}} override, or the full property key — and - * then for the key with leading segments dropped, which is what a {@code @WithName} mapping - * carries when the env name is derived rather than written out. + * Whether a line defines the key, by an exact whole-key match or by a prefix-dropped suffix + * match. The two are separable — {@link #snippetsFor} prefers exact matches file-wide — but a + * single boolean is what a plain "does this line mention the key at all" caller wants. */ static boolean lineDefines(String line, String normalizedToken) { + return lineDefinesExactly(line, normalizedToken) || lineDefinesBySuffix(line, normalizedToken); + } + + /** + * Whether the line carries the whole key: the literal env name of an explicit {@code + * ${ENV:default}} override, or the full property key. + */ + static boolean lineDefinesExactly(String line, String normalizedToken) { if (line == null || line.isBlank()) { return false; } - var normalizedLine = normalize(line); - if (containsSegment(normalizedLine, normalizedToken)) { - return true; + return containsSegment(normalize(line), normalizedToken); + } + + /** + * Whether the line carries the key with its leading segments dropped, which is what a + * {@code @WithName} mapping carries when the env name is derived rather than written out. At + * least {@link #MIN_SUFFIX_SEGMENTS} segments must survive, so a short key never degrades into a + * too-generic tail that matches an unrelated key's definition. + */ + static boolean lineDefinesBySuffix(String line, String normalizedToken) { + if (line == null || line.isBlank()) { + return false; } var segments = normalizedToken.split("_"); if (segments.length <= MIN_SUFFIX_SEGMENTS) { return false; } + var normalizedLine = normalize(line); // Longest suffix first, so "MANUAL_TRIGGER_ALLOWED_LOGINS" is preferred over "ALLOWED_LOGINS". for (var start = 1; start <= segments.length - MIN_SUFFIX_SEGMENTS; start++) { var suffix = String.join("_", List.of(segments).subList(start, segments.length)); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java index 306ab4d4..a8e49c3c 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java @@ -346,6 +346,45 @@ void shouldRequireWholeSegmentBoundariesAndKeepSearchingPastAPartialHit() { "a hit inside a longer word is not a definition"); } + @Test + void shouldNotDegradeAThreeSegmentTokenToItsTwoSegmentTail() { + // THRILLHOUSEBOT_HTTP_PORT must not degrade to HTTP_PORT and match quarkus.http.port, which + // is a different key's definition rendered as though it were this one's. + assertFalse( + ConfigKeyContextResolver.lineDefines( + "quarkus.http.port=8080", + ConfigKeyContextResolver.normalize("THRILLHOUSEBOT_HTTP_PORT")), + "a 3-segment token dropped to a 2-segment suffix matches far too much"); + assertTrue( + ConfigKeyContextResolver.lineDefines( + "thrillhousebot.http.port=8080", + ConfigKeyContextResolver.normalize("THRILLHOUSEBOT_HTTP_PORT")), + "the whole key still matches its own definition"); + } + + @Test + void shouldPreferAnExactMatchAnywhereInTheFileOverASuffixMatch() { + var token = ConfigKeyContextResolver.normalize("ALPHA_BETA_GAMMA_DELTA"); + var lines = + new String[] { + "x=${BETA_GAMMA_DELTA:1}", // a suffix hit on line 1 + "f", + "f", + "f", + "f", + "y=${ALPHA_BETA_GAMMA_DELTA:2}" // the exact definition further down + }; + + var snippets = ConfigKeyContextResolver.snippetsFor("app.properties", lines, token); + + assertTrue( + snippets.stream().anyMatch(s -> s.contains("ALPHA_BETA_GAMMA_DELTA")), + () -> "the exact definition must be rendered: " + snippets); + assertTrue( + snippets.stream().noneMatch(s -> s.contains("BETA_GAMMA_DELTA:1")), + () -> "a fuzzy suffix hit must not be preferred when an exact match exists: " + snippets); + } + @Test void shouldIgnoreBlankAndAbsentLines() { assertFalse(ConfigKeyContextResolver.lineDefines(null, TOKEN)); From 91f898bcd814bdd499d7de8c4c60875c82277d16 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 9 Aug 2026 15:10:43 +0000 Subject: [PATCH 4/6] perf(review): normalize each config file line once, not once per token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit absorbFile called snippetsFor per token, and snippetsFor re-normalized every line of the file for each token — O(tokens x lines) normalization for a file that is read once. The normalized line array is now computed once per file and passed through a snippetsFor overload; the per-token matchers read the supplied normals (matching) while rendering still uses the raw lines. No behaviour change — a structural hoist. Refs audit F8 --- .../review/ConfigKeyContextResolver.java | 57 ++++++++++++++++--- .../review/ConfigKeyContextResolverTest.java | 23 ++++++++ 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java index 4ed5c38f..5014b50a 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java @@ -457,16 +457,30 @@ private static void absorbFile( String[] lines, Map normalized, Map> found) { + // Normalize each line once for the whole file rather than once per token: matching is otherwise + // O(tokens x lines) in normalization for a file that is only read once. + var normalizedLines = normalizeLines(lines); for (var entry : normalized.entrySet()) { var snippets = found.computeIfAbsent(entry.getKey(), unused -> new ArrayList<>()); var room = MAX_SNIPPETS_PER_KEY - snippets.size(); if (room > 0) { - snippetsFor(path, lines, entry.getValue()).stream().limit(room).forEach(snippets::add); + snippetsFor(path, lines, normalizedLines, entry.getValue()).stream() + .limit(room) + .forEach(snippets::add); } } found.values().removeIf(List::isEmpty); } + /** Each source line normalized once, so per-token matching never re-normalizes the file. */ + static String[] normalizeLines(String[] lines) { + var out = new String[lines.length]; + for (var i = 0; i < lines.length; i++) { + out[i] = normalize(lines[i]); + } + return out; + } + /** A repository file's decoded text, or {@code null} when it cannot be read. */ private String fetchContent(String auth, String owner, String repo, String path, String ref) { try { @@ -491,20 +505,35 @@ private String fetchContent(String auth, String owner, String repo, String path, * discount it. */ static List snippetsFor(String path, String[] lines, String normalizedToken) { - var exact = matchingSnippets(path, lines, normalizedToken, true); - return exact.isEmpty() ? matchingSnippets(path, lines, normalizedToken, false) : exact; + return snippetsFor(path, lines, normalizeLines(lines), normalizedToken); + } + + /** + * The same, but taking the file's lines pre-normalized so a whole-file walk normalizes each line + * once instead of once per token. {@code lines} is rendered; {@code normalizedLines} is matched. + */ + static List snippetsFor( + String path, String[] lines, String[] normalizedLines, String normalizedToken) { + var exact = matchingSnippets(path, lines, normalizedLines, normalizedToken, true); + return exact.isEmpty() + ? matchingSnippets(path, lines, normalizedLines, normalizedToken, false) + : exact; } private static List matchingSnippets( - String path, String[] lines, String normalizedToken, boolean exactOnly) { + String path, + String[] lines, + String[] normalizedLines, + String normalizedToken, + boolean exactOnly) { var snippets = new ArrayList(); var lastRendered = -1; for (var i = 0; i < lines.length && snippets.size() < MAX_SNIPPETS_PER_KEY; i++) { var from = Math.max(0, i - CONTEXT_LINES_BEFORE); var defines = exactOnly - ? lineDefinesExactly(lines[i], normalizedToken) - : lineDefinesBySuffix(lines[i], normalizedToken); + ? definesExactly(normalizedLines[i], normalizedToken) + : definesBySuffix(normalizedLines[i], normalizedToken); // from > lastRendered skips a match the previous window already shows: adjacent matches (a // property and its override on consecutive lines) share one snippet rather than repeating it. if (defines && from > lastRendered) { @@ -563,7 +592,7 @@ static boolean lineDefinesExactly(String line, String normalizedToken) { if (line == null || line.isBlank()) { return false; } - return containsSegment(normalize(line), normalizedToken); + return definesExactly(normalize(line), normalizedToken); } /** @@ -576,11 +605,23 @@ static boolean lineDefinesBySuffix(String line, String normalizedToken) { if (line == null || line.isBlank()) { return false; } + return definesBySuffix(normalize(line), normalizedToken); + } + + /** {@link #lineDefinesExactly} against an already-normalized line. */ + private static boolean definesExactly(String normalizedLine, String normalizedToken) { + return !normalizedLine.isEmpty() && containsSegment(normalizedLine, normalizedToken); + } + + /** {@link #lineDefinesBySuffix} against an already-normalized line. */ + private static boolean definesBySuffix(String normalizedLine, String normalizedToken) { + if (normalizedLine.isEmpty()) { + return false; + } var segments = normalizedToken.split("_"); if (segments.length <= MIN_SUFFIX_SEGMENTS) { return false; } - var normalizedLine = normalize(line); // Longest suffix first, so "MANUAL_TRIGGER_ALLOWED_LOGINS" is preferred over "ALLOWED_LOGINS". for (var start = 1; start <= segments.length - MIN_SUFFIX_SEGMENTS; start++) { var suffix = String.join("_", List.of(segments).subList(start, segments.length)); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java index a8e49c3c..73d51f65 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java @@ -464,6 +464,29 @@ void shouldKeepABlankLineInsideTheWindowAndDropOnlyTheBoundaries() { assertTrue(snippet.contains(" 4 | tail"), snippet); } + @Test + void shouldMatchUsingTheSuppliedNormalizedLinesAndRenderTheRawOnes() { + // The hoisted overload trusts the caller's pre-normalized lines (one normalization per file, + // not one per token) and does not re-derive them from the raw lines: a normalized array that + // maps the raw line to a DIFFERENT key proves the match reads the supplied normals while the + // rendered snippet is still the raw line. + var rawLines = new String[] {"totally unrelated raw text"}; + var normalizedLines = + new String[] {ConfigKeyContextResolver.normalize("x=${WEBHOOK_DEDUP_TTL:1h}")}; + + var snippets = + ConfigKeyContextResolver.snippetsFor("app.properties", rawLines, normalizedLines, TOKEN); + + assertEquals( + 1, + snippets.size(), + () -> "the match must follow the supplied normalized lines: " + snippets); + assertTrue( + snippets.get(0).contains("totally unrelated raw text"), + () -> + "the rendered snippet is the raw line, matched via the normalized one: " + snippets); + } + @Test void shouldNotSplitASurrogatePairWhenTruncating() { var emoji = "ab😀cd"; From c40b50bcd120600dd249996f33b2caed56796310 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 9 Aug 2026 15:44:27 +0000 Subject: [PATCH 5/6] test(review): cover new config-key branch sides; soften the TLD comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codecov flagged 5 partial branches in ConfigKeyContextResolver from the F4 and F5 changes. Four are now covered by load-bearing tests: - looksLikeConfigKey TLD check: a non-URL hostname whose LAST segment is a TLD (service.example.com), a token whose FIRST segment is a TLD (io.quarkus.arc), and a token that is neither (kept) — both sides of both sub-conditions. - isInsideUrl backward scan: a URL at the very start of the line, so the word-start walk reaches offset 0 rather than a preceding space. - isInsideUrl word test: a www. word with no scheme, exercising the second side of the "://" || "www." check. - lineDefines: a line that defines the key only through the suffix branch (an exact whole-key match absent), exercising the || right side. The fifth partial — the isInsideUrl forward scan's "reached end of string" side — is unreachable: the sole caller feeds addedLines() output, which always ends in '\n', so the scan always halts on whitespace before the string end. It is left as correct defensive code, documented in the report. Dogfood finding on the F4 fix (valid): the leading-TLD reject that catches Java FQNs (org.eclipse.microprofile.rest.client) also drops legitimate reverse-domain config keys (com.example.service.timeout). The two cannot be separated by shape without dropping org.eclipse... from extraction and breaking shouldRejectHostnamesAndFqnsAndPreserveFirstMentionOrder, so the behavior is kept as an accepted precision/recall trade-off and the false comment ("No real config key begins or ends with a bare TLD segment") is replaced with one that states the trade-off honestly. Refs audit a4-F4 Refs audit a4-F5 --- .../review/ConfigKeyContextResolver.java | 16 ++++- .../review/ConfigKeyContextResolverTest.java | 60 +++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java index 5014b50a..9be8f444 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java @@ -144,8 +144,17 @@ public class ConfigKeyContextResolver { /** * Trailing/leading dotted segments that mark a token as a hostname or a reverse-domain Java FQN * rather than a configuration key. A hostname trails with the TLD ({@code api.github.com}); a - * reverse-domain package name leads with it ({@code org.eclipse.microprofile.rest.client}). No - * real config key begins or ends with a bare TLD segment, so both ends are checked. + * reverse-domain package name leads with it ({@code org.eclipse.microprofile.rest.client}). Both + * ends are therefore checked. + * + *

This is a precision/recall trade-off, not a law: a legitimate reverse-domain config + * key ({@code com.example.service.timeout}, {@code io.micrometer.export.step}) also leads with a + * TLD segment and is dropped here too. Such a key cannot be told apart from a Java package name + * by shape alone — only by whether it resolves to a key-position definition rather than an {@code + * import} — and a mis-extracted FQN that does resolve (against an {@code import} line in + * a config source) would fill a rendered slot and crowd out the documented key. The trade-off + * deliberately favours dropping the ambiguous reverse-domain token; reverse-domain config keys + * are rare, and one dropped enrichment snippet is cheaper than a wrong one. */ private static final Set TLD_SEGMENTS = Set.of("com", "org", "net", "io", "co", "dev", "gov", "edu", "info", "app", "ai", "me", "us"); @@ -244,7 +253,8 @@ private static void collectTokens(String addedText, Set tokens) { * fully-qualified name that happens to share the dotted-lowercase shape and would otherwise * resolve against unrelated config lines and crowd out the actually-documented key. An {@code * UPPER_SNAKE} env name is always a key. A dotted property token is rejected when it sits inside - * a URL on the source line, or when its first or last segment is a common TLD. + * a URL on the source line, or when its first or last segment is a common TLD (see {@link + * #TLD_SEGMENTS} for the reverse-domain-config-key trade-off the leading-TLD check accepts). */ private static boolean looksLikeConfigKey(String token, String line, int start) { if (token.indexOf('.') < 0) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java index 73d51f65..97068d71 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java @@ -191,6 +191,54 @@ void shouldRejectHostnamesAndFqnsAndPreserveFirstMentionOrder() { + tokens); } + @Test + void shouldRejectDottedTokensByTldPositionAndUrlWord() { + // Exercises each rejection reason on a non-URL token: last segment a TLD (hostname in prose), + // first segment a TLD (reverse-domain FQN, or the accepted reverse-domain-key trade-off), a + // www. word (URL without a scheme), and a token that is none of these (kept). + var tokens = + ConfigKeyContextResolver.extractTokens( + List.of( + docDiff( + "README.md", + "host service.example.com and package io.quarkus.arc but keep" + + " `thrillhousebot.review.enabled` and see www.docs.internal for more"))); + + assertFalse( + tokens.contains("service.example.com"), + () -> "last segment is a TLD (a hostname), even in prose: " + tokens); + assertFalse( + tokens.contains("io.quarkus.arc"), + () -> "first segment is a TLD (reverse-domain shape): " + tokens); + assertFalse( + tokens.contains("www.docs.internal"), + () -> "a www. word is a URL, not a config key: " + tokens); + assertEquals( + List.of("thrillhousebot.review.enabled"), + tokens, + () -> + "only the token that is neither a hostname, an FQN, nor in a URL survives: " + + tokens); + } + + @Test + void shouldTreatAUrlAtTheVeryStartOfALineAsAUrl() { + // The URL begins at offset 0, so scanning back to find the word start reaches the string's + // start rather than a preceding space — the other side of that boundary walk. + var tokens = + ConfigKeyContextResolver.extractTokens( + List.of( + docDiff( + "README.md", + "http://foo.example.internal/x is the base and `keep.this.key`"))); + + assertFalse( + tokens.contains("foo.example.internal"), + () -> "a host at the very start of the line is still inside a URL: " + tokens); + assertEquals( + List.of("keep.this.key"), tokens, () -> "the real key past the URL survives: " + tokens); + } + @Test void shouldNotMistakeFilenamesForPropertyKeys() { var tokens = @@ -403,6 +451,18 @@ void shouldNotSuffixMatchATokenTooShortToHaveAPrefix() { "x=${FOO_BAR:1}", ConfigKeyContextResolver.normalize("FOO_BAR"))); } + @Test + void shouldReportALineAsDefiningTheKeyThroughTheSuffixBranch() { + // No exact whole-key hit on this line, so lineDefines must fall through to the suffix branch: + // the derived @WithName env name matches with its two prefix segments dropped. + assertTrue( + ConfigKeyContextResolver.lineDefines( + " @WithName(\"manual-trigger-allowed-logins\")", + ConfigKeyContextResolver.normalize( + "THRILLHOUSEBOT_REVIEW_MANUAL_TRIGGER_ALLOWED_LOGINS")), + "an exact match is absent, so the suffix branch is what makes this a definition"); + } + @Test void shouldMergeAdjacentMatchesIntoOneSnippetAndCapTheRest() { var lines = From 992c2340391ea9c9655810ae17b816c630844319 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 9 Aug 2026 15:57:02 +0000 Subject: [PATCH 6/6] test(review): cover the isInsideUrl end-of-string guard directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last patch partial was the forward-scan bound `to < text.length()` in isInsideUrl. It is unreachable through the production path — the sole caller feeds addedLines() output, which always ends in '\n' — but the bound is real defence against a token that runs to the end of the buffer, so it is tested rather than left uncovered. isInsideUrl is made package-private and a unit test drives it with a word that reaches the end of the string (both a URL and a non-URL word), exercising the guard and validating it does what it claims. The javadoc now records the caller invariant and why the guard stays. Refs audit a4-F4 --- .../review/ConfigKeyContextResolver.java | 9 +++++++-- .../review/ConfigKeyContextResolverTest.java | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java index 9be8f444..c6efa93b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java @@ -268,8 +268,13 @@ private static boolean looksLikeConfigKey(String token, String line, int start) && !TLD_SEGMENTS.contains(segments[segments.length - 1]); } - /** Whether the whitespace-delimited run of {@code text} containing offset {@code at} is a URL. */ - private static boolean isInsideUrl(String text, int at) { + /** + * Whether the whitespace-delimited run of {@code text} containing offset {@code at} is a URL. The + * forward scan's {@code to < text.length()} bound guards a token that runs to the very end of the + * string; the production caller always feeds {@code \n}-terminated text so that end is never hit + * there, but the guard is exercised directly in a unit test rather than left as untested defence. + */ + static boolean isInsideUrl(String text, int at) { var from = at; while (from > 0 && !Character.isWhitespace(text.charAt(from - 1))) { from--; diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java index 97068d71..7f9fea79 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java @@ -239,6 +239,23 @@ void shouldTreatAUrlAtTheVeryStartOfALineAsAUrl() { List.of("keep.this.key"), tokens, () -> "the real key past the URL survives: " + tokens); } + @Test + void shouldDetectAUrlWordThatRunsToTheEndOfTheString() { + // The word extends to the very end with no trailing whitespace, exercising the forward scan's + // end-of-string bound directly. That bound guards a token at the end of the buffer; the + // production caller always feeds \n-terminated text so it is never hit there, but the guard + // is + // real defence and is validated here rather than left untested. + var url = "see https://api.example.host"; + assertTrue( + ConfigKeyContextResolver.isInsideUrl(url, url.indexOf("api")), + "a URL word reaching the end of the string is still a URL"); + var prose = "plain trailing.host"; + assertFalse( + ConfigKeyContextResolver.isInsideUrl(prose, prose.indexOf("trailing")), + "a non-URL word reaching the end of the string is not a URL"); + } + @Test void shouldNotMistakeFilenamesForPropertyKeys() { var tokens =