diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolver.java index 8f89117a..c6efa93b 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 = @@ -123,6 +132,33 @@ 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}). 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"); + /** 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 +239,52 @@ 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); + } } - var property = PROPERTY_TOKEN.matcher(addedText); - while (property.find() && tokens.size() < MAX_TOKENS) { - tokens.add(property.group()); + } + + /** + * 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 (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) { + 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. 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--; + } + 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). */ @@ -363,9 +437,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); } } @@ -394,16 +472,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 { @@ -420,17 +512,49 @@ 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) { + 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[] 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 + ? 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 (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; } } @@ -467,18 +591,47 @@ 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 definesExactly(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; + } + 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) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java index fb4082ff..7f9fea79 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ConfigKeyContextResolverTest.java @@ -162,6 +162,100 @@ 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 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 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 = @@ -317,6 +411,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)); @@ -335,6 +468,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 = @@ -396,6 +541,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"; @@ -651,6 +819,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();