From 2eeb1a3d7c11bd925d940a0423ce9df52f82374f Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sat, 8 Aug 2026 00:48:20 +0000 Subject: [PATCH 1/2] feat(review): support per-repo ignore patterns alongside the global default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit thrillhousebot.review.ignored-files is app-wide, so one deployment reviewing many repositories has to pick a single list for all of them — a repo with generated dirs, vendored code, or large fixtures has no way to say so. A repository can now declare ignore globs of its own under review.ignored-files in an optional .github/thrillhousebot.yml. They are additive: the effective set is global union per-repo, so a repository can take more files out of review scope but can never put back a file the deployment excludes. Structured settings live in a dedicated file rather than frontmatter in the instructions file, because the instructions fallback chain deliberately reaches into files owned by other tools (copilot-instructions.md, CLAUDE.md, AGENTS.md) and its content is handed to the model as untrusted prose — config there would either leak into the prompt or need stripping out of it. The existing glob matching in ReviewDiffFormatter is wrapped in an IgnoreGlobs value type that both lists compile through, so a repository cannot get different matching semantics than the global default. The effective set is resolved once per review in ReviewContextLoader and threaded into the single reviewableFiles call, preserving the compute-once property. Everything fails soft: feature off, file absent, transport error, undecodable content, malformed YAML, unexpected shape, or an uncompilable glob all degrade to the global list rather than failing the review. The parser reads a generic tree (no reflection) with snakeyaml loader limits and caps on pattern count and length, since the file is untrusted input from an arbitrary repository. jackson-dataformat-yaml was already on the compile classpath via quarkus-smallrye-openapi and version-managed by the jackson-bom import; it is now declared explicitly because it is used directly. thrillhousebot.review.repo-config-enabled (default true) is the operator kill switch for installs that must not let a repository narrow its own review scope. Refs #51 --- .env.example | 4 + CHANGELOG.md | 4 + README.md | 38 ++- pom.xml | 9 + .../config/ThrillhouseConfig.java | 12 + .../thrillhousebot/github/RepoSettings.java | 47 +++ .../github/RepoSettingsParser.java | 152 +++++++++ .../github/RepoSettingsResolver.java | 178 ++++++++++ .../review/ReviewContextLoader.java | 48 ++- .../review/ReviewDiffFormatter.java | 120 ++++++- .../thrillhousebot/review/SoftLoaders.java | 28 ++ .../github/RepoSettingsResolverTest.java | 310 ++++++++++++++++++ .../review/ReviewContextLoaderTest.java | 69 ++++ .../review/ReviewDiffFormatterTest.java | 86 +++++ .../review/ReviewOrchestratorTest.java | 2 + 15 files changed, 1086 insertions(+), 21 deletions(-) create mode 100644 src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java create mode 100644 src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java create mode 100644 src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java diff --git a/.env.example b/.env.example index 7bef07fe..8b60bdac 100644 --- a/.env.example +++ b/.env.example @@ -70,6 +70,10 @@ GITHUB_WEBHOOK_SECRET=your_webhook_secret # Comma-separated gitignore-style globs excluded from review (lockfiles, generated code, build # output). '*' does not cross '/'; use '**' to span directories: #THRILLHOUSEBOT_REVIEW_IGNORED_FILES=**/pom.xml,**/package-lock.json,**/*.lock,**/*.generated.*,**/target/** +# Let each repository add ignore globs of its own under review.ignored-files in +# .github/thrillhousebot.yml. They are unioned with the list above — a repo can skip more, never +# less. Set false to make the deployment list the only one that counts: +#THRILLHOUSEBOT_REVIEW_REPO_CONFIG_ENABLED=true # Optional: context-aware PR labels (off by default). Enable, then choose apply vs. suggest-only. #REVIEW_LABELS_ENABLED=true diff --git a/CHANGELOG.md b/CHANGELOG.md index 16465a16..2f3edd42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to ThrillhouseBot. ## [Unreleased] +### Added + +- **Per-repo ignore patterns** (#51): a repository can now declare ignore globs of its own in an optional `.github/thrillhousebot.yml`, under `review.ignored-files`. They are **additive** — the effective set is the union of the deployment-wide `thrillhousebot.review.ignored-files` list and whatever the repository declared, so a repo can take more files out of review scope (generated dirs, vendored code, fixtures) but can never put back a file the deployment excludes. One install can therefore serve repositories with different layouts without everyone sharing a single global list. The file is read from the default branch and cached per repository for five minutes, alongside the existing instructions lookup, and fails soft in every direction: a missing file, invalid YAML, an unexpected shape, or an uncompilable glob is logged and skipped, leaving the global list in force. Operators who do not want repositories adjusting their own review scope can disable the mechanism with `THRILLHOUSEBOT_REVIEW_REPO_CONFIG_ENABLED=false` + ## [0.5.0] — 2026-07-26 Review precision: confidence now decides where a finding lands, newly-added parsers and regexes are stress-tested for their own failure modes, and several classes of false positive are guarded at both the generator and the verifier. Operators gain configurable CI-gating and blocking strictness, structured skip reasons, and per-model generation parameters. diff --git a/README.md b/README.md index d6f6eb71..ce96e3a2 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,7 @@ guide, configuration reference, architecture, comparison, and the hosted - OpenTelemetry traces, token histograms, cost counters, and latency metrics - Optional reasoning-effort dial and per-model generation/budget caps for OpenAI-compatible endpoints - Reads per-repo instructions from `.github/thrillhousebot.md`, falling back to Copilot/Claude/Agents files +- Lets each repository add its own ignore globs in `.github/thrillhousebot.yml`, unioned with the deployment default - Compiles ahead-of-time with GraalVM/Mandrel, so it starts fast and stays small @@ -258,6 +259,7 @@ will change per provider: | `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS` | Client-side wait per AI streaming attempt; keep it >= `AI_TIMEOUT` so timed-out attempts don't leave orphaned provider streams | `300` | | `THRILLHOUSEBOT_REVIEW_INSTRUCTIONS_FILE` | Repo-relative path of the per-repo instructions file read on each review | `.github/thrillhousebot.md` | | `THRILLHOUSEBOT_REVIEW_IGNORED_FILES` | Comma-separated gitignore-style globs excluded from review — lockfiles, generated code, build output. `*` does not cross `/`; use `**` to span directories. Replaces (not extends) the default list, so re-include the defaults you still want | `**/pom.xml,**/package-lock.json,**/*.lock,**/*.generated.*,**/target/**` | +| `THRILLHOUSEBOT_REVIEW_REPO_CONFIG_ENABLED` | Let each repository extend the ignore list with globs of its own from `.github/thrillhousebot.yml` (see [Repository configuration](#repository-configuration)). Per-repo globs are additive; set `false` to make the deployment list the only one that counts | `true` | | `REVIEW_LABELS_ENABLED` | Opt in to context-aware PR labels (see [PR labels](#pr-labels)) | `false` | | `REVIEW_LABELS_APPLY` | When labels are enabled, add them to the PR instead of only suggesting them in a comment | `false` | | `REVIEW_LABELS_ALLOW_CREATE` | Allow the bot to create suggested labels that don't exist yet | `false` | @@ -420,6 +422,38 @@ Place a `.github/thrillhousebot.md` file in any repo to customize the review: Fallback chain: `.github/thrillhousebot.md` → `.github/copilot-instructions.md` → `CLAUDE.md` → `AGENTS.md` → `AGENT.md` +## Repository configuration + +The instructions file above is prose for the model. Structured settings live in a +separate, optional `.github/thrillhousebot.yml` (`.github/thrillhousebot.yaml` also +works) — kept apart on purpose, because the instructions fallback chain may land on +a file owned by another tool, and its whole content is fed to the model as untrusted +prose: + +```yaml +review: + # Extra paths this repository never wants reviewed, on top of the deployment default. + ignored-files: + - "docs/generated/**" + - "**/*.snap" + - "testdata/**" +``` + +**Precedence: the effective ignore list is the union of both — global ∪ per-repo.** +A file is skipped if it matches *either* the deployment-wide +`thrillhousebot.review.ignored-files` list *or* a glob the repository declared. A +repository can therefore take more files out of review scope, but never put back a +file the deployment excludes, and a repository that ships no config file gets the +global list exactly as before. Globs use the same gitignore-style syntax as the +global key (`*` does not cross `/`; use `**` to span directories). + +The file is read from the repository's default branch on each review and cached for +five minutes. Everything about it fails soft: a missing file, invalid YAML, an +unexpected shape, or an uncompilable glob is logged and skipped, leaving the global +list in force — it never fails a review. Operators who do not want repositories +adjusting their own review scope can turn the whole mechanism off with +`THRILLHOUSEBOT_REVIEW_REPO_CONFIG_ENABLED=false`. + ## PR labels @@ -541,7 +575,9 @@ This is still an early-stage project; the current constraints are: sourcemaps, generated code (`*.generated.*`, protobuf output), and build or vendor directories (`target/`, `node_modules/`, `dist/`, `build/`, `out/`, `.next/`, `vendor/`, `__pycache__/`, `.venv/`, `bin/`, `obj/`) are skipped by - default (`thrillhousebot.review.ignored-files`, overridable per deployment). + default (`thrillhousebot.review.ignored-files`, overridable per deployment, and + extendable per repository via `.github/thrillhousebot.yml` — see + [Repository configuration](#repository-configuration)). - **Self-hosted** — no managed offering from this project. ## Verifying a release diff --git a/pom.xml b/pom.xml index d2cc5198..21b5f205 100644 --- a/pom.xml +++ b/pom.xml @@ -125,6 +125,15 @@ io.quarkus quarkus-rest-client-jackson + + + com.fasterxml.jackson.dataformat + jackson-dataformat-yaml + io.quarkus quarkus-arc diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java index 41b986d4..8a80473b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java @@ -249,6 +249,18 @@ interface ReviewConfig { @WithName("ignored-files") List ignoredFiles(); + /** + * Whether a repository may extend {@link #ignoredFiles()} with globs of its own, declared under + * {@code review.ignored-files} in {@code .github/thrillhousebot.yml}. Per-repo patterns are + * additive — the effective set is always global ∪ per-repo, so a repository can take more files + * out of review scope but can never put back a file the deployment excludes. This is the + * operator's kill switch for installs that must not let a repository narrow its own review + * coverage; a missing or malformed file is ignored either way and never fails a review. + */ + @WithDefault("true") + @WithName("repo-config-enabled") + boolean repoConfigEnabled(); + /** * GitHub logins permitted to manually trigger reviews regardless of repository permission. When * empty, only users with write access to the repository may trigger a manual review. diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java new file mode 100644 index 00000000..8b0c4373 --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java @@ -0,0 +1,47 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.github; + +import java.util.List; + +/** + * The structured settings a repository declares for itself, read from {@code + * .github/thrillhousebot.yml} by {@link RepoSettingsResolver}. + * + *

This is the one place per-repo structured settings live — deliberately separate from + * the prose instructions file ({@link InstructionsResolver}), whose fallback chain reaches into + * files owned by other tools and whose content is fed to the model as untrusted prose. New + * structured settings are added as components here and parsed in {@link RepoSettingsParser}; every + * one of them must degrade to its {@link #EMPTY} value rather than fail a review. + * + * @param ignoredFiles extra ignore globs, unioned with (never replacing) the deployment-wide {@code + * thrillhousebot.review.ignored-files} list + * @param source the repo-relative path the settings were read from, or {@code "none"} + */ +public record RepoSettings(List ignoredFiles, String source) { + + /** No per-repo settings — the deployment defaults apply unchanged. */ + public static final RepoSettings EMPTY = new RepoSettings(List.of(), "none"); + + public RepoSettings { + ignoredFiles = List.copyOf(ignoredFiles); + } + + /** Whether a config file was found and yielded at least one setting. */ + public boolean isPresent() { + return !ignoredFiles.isEmpty(); + } +} diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java new file mode 100644 index 00000000..e7cdbdea --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java @@ -0,0 +1,152 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.github; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.yaml.snakeyaml.LoaderOptions; + +/** + * Parses the YAML in a repository's {@code .github/thrillhousebot.yml} into {@link RepoSettings}. + * + *

Shape (every key optional): + * + * 

{@code
+ * review:
+ *   ignored-files:
+ *     - "docs/generated/**"
+ *     - "**''/''*.snap"
+ * }
+ * + *

The file is untrusted input from an arbitrary repository, so parsing is deliberately + * defensive: it reads a generic tree rather than binding to a POJO (no reflection, no type + * coercion), bounds the document with snakeyaml loader limits, caps how many patterns a repository + * may contribute, and returns {@link RepoSettings#EMPTY} for anything it cannot make sense of. It + * never throws — a malformed config must degrade to "no per-repo settings", never fail a review. + */ +final class RepoSettingsParser { + + private static final Logger log = LoggerFactory.getLogger(RepoSettingsParser.class); + + /** Ceiling on the YAML document size, guarding against an oversized or hostile config. */ + private static final int MAX_CODE_POINTS = 256 * 1024; + + /** Ceiling on YAML nesting, guarding against deeply nested documents. */ + private static final int MAX_NESTING_DEPTH = 20; + + /** Ceiling on anchor/alias expansion, guarding against "billion laughs"-style blowups. */ + private static final int MAX_ALIASES = 50; + + /** Ceiling on how many extra ignore globs one repository may contribute. */ + static final int MAX_PATTERNS = 200; + + /** Ceiling on a single glob's length — a pathological pattern is dropped, not compiled. */ + static final int MAX_PATTERN_LENGTH = 512; + + private static final ObjectMapper YAML_MAPPER = new ObjectMapper(yamlFactory()); + + private RepoSettingsParser() {} + + private static YAMLFactory yamlFactory() { + var options = new LoaderOptions(); + options.setCodePointLimit(MAX_CODE_POINTS); + options.setNestingDepthLimit(MAX_NESTING_DEPTH); + options.setMaxAliasesForCollections(MAX_ALIASES); + options.setAllowDuplicateKeys(false); + return YAMLFactory.builder().loaderOptions(options).build(); + } + + /** + * Parses {@code yaml}, attributing the result to {@code source} (the repo-relative path it came + * from). Returns {@link RepoSettings#EMPTY} for blank, malformed, or setting-less content. + */ + static RepoSettings parse(String yaml, String source) { + if (yaml == null || yaml.isBlank()) { + return RepoSettings.EMPTY; + } + try { + var root = YAML_MAPPER.readTree(yaml); + if (root == null || !root.isObject()) { + log.warn("Repository config {} is not a YAML mapping; ignoring it", source); + return RepoSettings.EMPTY; + } + var ignoredFiles = readPatterns(root.path("review").path("ignored-files"), source); + return ignoredFiles.isEmpty() ? RepoSettings.EMPTY : new RepoSettings(ignoredFiles, source); + } catch (IOException | RuntimeException e) { + log.warn( + "Could not parse repository config {}; continuing with the global settings only", + source, + e); + return RepoSettings.EMPTY; + } + } + + /** + * Reads {@code review.ignored-files} as a list of globs. A sequence of scalars is the documented + * form; a single scalar is also accepted and split on commas, matching how the global key is + * written as an environment variable. Anything else is ignored. + */ + private static List readPatterns(JsonNode node, String source) { + if (node == null || node.isMissingNode() || node.isNull()) { + return List.of(); + } + var raw = new ArrayList(); + if (node.isArray()) { + for (var element : node) { + if (element.isValueNode()) { + raw.add(element.asText()); + } + } + } else if (node.isValueNode()) { + raw.addAll(List.of(node.asText().split(","))); + } else { + log.warn("Repository config {}: review.ignored-files is not a list; ignoring it", source); + return List.of(); + } + return sanitize(raw, source); + } + + /** Trims, drops blank/oversized entries, and caps the total a repository may contribute. */ + private static List sanitize(List raw, String source) { + var patterns = new ArrayList(Math.min(raw.size(), MAX_PATTERNS)); + for (String value : raw) { + var pattern = value == null ? "" : value.trim(); + if (pattern.isEmpty()) { + continue; + } + 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); + } +} diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java new file mode 100644 index 00000000..8b48a927 --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java @@ -0,0 +1,178 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.github; + +import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import jakarta.ws.rs.ProcessingException; +import jakarta.ws.rs.WebApplicationException; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.List; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.LongSupplier; +import org.eclipse.microprofile.rest.client.inject.RestClient; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Resolves a repository's own structured settings from {@code .github/thrillhousebot.yml}, cached + * per repository with the same TTL scheme {@link InstructionsResolver} uses for the prose + * instructions file. + * + *

A dedicated file — rather than frontmatter in the instructions file — because the instructions + * fallback chain deliberately reaches into files owned by other tools ({@code + * .github/copilot-instructions.md}, {@code CLAUDE.md}, {@code AGENTS.md}), and because that file's + * whole content is handed to the model as untrusted prose; structured settings belong somewhere + * that neither depends on which of those files won nor perturbs the prompt. + * + *

Every failure mode degrades to {@link RepoSettings#EMPTY}: the file is absent, unreadable, + * unparseable, or the feature is switched off — in all cases the deployment defaults apply and the + * review proceeds. + */ +@ApplicationScoped +public class RepoSettingsResolver { + + private static final Logger log = LoggerFactory.getLogger(RepoSettingsResolver.class); + private static final String ACCEPT_HEADER = "application/vnd.github+json"; + + /** Config-file names tried in order; the first that exists wins. */ + static final List CONFIG_FILE_CHAIN = + List.of(".github/thrillhousebot.yml", ".github/thrillhousebot.yaml"); + + private record CachedSettings(RepoSettings settings, long expiresAt) {} + + // Package-private for tests. + final ConcurrentHashMap cache = new ConcurrentHashMap<>(); + static final long CACHE_TTL_MS = 5L * 60 * 1000; // 5 minutes + static final long NEGATIVE_CACHE_TTL_MS = 60_000; // 1 minute + static final int CACHE_SWEEP_THRESHOLD = 1_000; + + private final boolean enabled; + private final GitHubAuthClient authClient; + private final GitHubPullRequestClient prClient; + private final LongSupplier clock; + + @Inject + public RepoSettingsResolver( + ThrillhouseConfig config, + GitHubAuthClient authClient, + @RestClient GitHubPullRequestClient prClient) { + this(config, authClient, prClient, System::currentTimeMillis); + } + + /** Visible for tests: allows controlling the cache clock. */ + RepoSettingsResolver( + ThrillhouseConfig config, + GitHubAuthClient authClient, + GitHubPullRequestClient prClient, + LongSupplier clock) { + this.enabled = config.review().repoConfigEnabled(); + this.authClient = authClient; + this.prClient = prClient; + this.clock = clock; + } + + /** + * The repository's declared settings, or {@link RepoSettings#EMPTY} when it declares none (or the + * feature is disabled). Never throws. + */ + public RepoSettings resolve( + String owner, String repo, String defaultBranch, long installationId) { + if (!enabled) { + return RepoSettings.EMPTY; + } + var cacheKey = owner + "/" + repo; + + var cached = cache.get(cacheKey); + if (cached != null) { + if (clock.getAsLong() < cached.expiresAt()) { + log.debug("Using cached repository config for {}", cacheKey); + return cached.settings(); + } + cache.remove(cacheKey, cached); + } + + var auth = authClient.getAuthHeader(installationId); + + for (String path : CONFIG_FILE_CHAIN) { + var settings = fetchAndParse(auth, owner, repo, defaultBranch, path); + if (settings == null) { + continue; + } + cache.put(cacheKey, new CachedSettings(settings, clock.getAsLong() + CACHE_TTL_MS)); + sweepExpiredEntries(); + log.info( + "Using repository config {} for {} ({} extra ignore pattern(s))", + path, + cacheKey, + settings.ignoredFiles().size()); + return settings; + } + + // Cache the negative result briefly so an unconfigured repo is not re-fetched every review. + cache.put( + cacheKey, + new CachedSettings(RepoSettings.EMPTY, clock.getAsLong() + NEGATIVE_CACHE_TTL_MS)); + sweepExpiredEntries(); + log.debug("No repository config file found for {}", cacheKey); + return RepoSettings.EMPTY; + } + + /** + * Fetches and parses one candidate path. Returns {@code null} when the file is absent or could + * not be read — the caller then tries the next name in the chain. A file that exists but is + * malformed parses to {@link RepoSettings#EMPTY}, which is a real (cacheable) answer: the + * repository has spoken, it just said nothing usable. + */ + private RepoSettings fetchAndParse( + String auth, String owner, String repo, String defaultBranch, String path) { + try { + var file = prClient.getFileContent(auth, ACCEPT_HEADER, owner, repo, path, defaultBranch); + if (file == null || file.content() == null) { + return null; + } + // GitHub wraps base64 content in newlines; only the MIME decoder tolerates them. + var content = + new String(Base64.getMimeDecoder().decode(file.content()), StandardCharsets.UTF_8); + return RepoSettingsParser.parse(content, path); + } catch (WebApplicationException | ProcessingException _) { + log.debug("Repository config file not found: {}", path); + return null; + } catch (RuntimeException e) { + log.warn( + "Failed to read repository config {} for {}/{}; continuing with the global settings only", + path, + owner, + repo, + e); + return null; + } + } + + /** + * The evict-on-read in resolve() only replaces entries whose key is requested again; without this + * sweep the cache keeps one entry forever per repo that is never reviewed again. + */ + void sweepExpiredEntries() { + if (cache.size() < CACHE_SWEEP_THRESHOLD) { + return; + } + var now = clock.getAsLong(); + cache.entrySet().removeIf(entry -> now >= entry.getValue().expiresAt()); + } +} diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java index 616f3970..e8949ff4 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java @@ -25,6 +25,7 @@ import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.github.InstructionsResolver; import dev.thiagogonzaga.thrillhousebot.github.ProjectStackResolver; +import dev.thiagogonzaga.thrillhousebot.github.RepoSettingsResolver; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import io.quarkus.logging.Log; import jakarta.enterprise.context.ApplicationScoped; @@ -53,6 +54,7 @@ public class ReviewContextLoader { private final GitHubReviewClient reviewClient; private final GitHubCommentClient commentClient; private final InstructionsResolver instructionsResolver; + private final RepoSettingsResolver repoSettingsResolver; private final ProjectStackResolver projectStackResolver; private final ReviewDiffFormatter diffFormatter; private final PrLabeler labeler; @@ -68,6 +70,7 @@ public ReviewContextLoader( @RestClient GitHubReviewClient reviewClient, @RestClient GitHubCommentClient commentClient, InstructionsResolver instructionsResolver, + RepoSettingsResolver repoSettingsResolver, ProjectStackResolver projectStackResolver, ReviewDiffFormatter diffFormatter, PrLabeler labeler, @@ -80,6 +83,7 @@ public ReviewContextLoader( this.reviewClient = reviewClient; this.commentClient = commentClient; this.instructionsResolver = instructionsResolver; + this.repoSettingsResolver = repoSettingsResolver; this.projectStackResolver = projectStackResolver; this.diffFormatter = diffFormatter; this.labeler = labeler; @@ -160,7 +164,10 @@ ReviewContext load( String auth, ReviewOrchestrator.ReviewRequest req, ReviewSession session, String repository) { var files = fetchPrFiles(auth, req.owner(), req.repo(), req.prNumber()); var prTotals = fetchPrTotalsForReview(auth, req); - var reviewableFiles = diffFormatter.reviewableFiles(files); + // Global ∪ per-repo ignore globs, compiled once so the ignore filter is still walked a single + // time per review; a repo that declares nothing resolves straight back to the global set. + var ignoreGlobs = resolveIgnoreGlobs(req); + var reviewableFiles = diffFormatter.reviewableFiles(files, ignoreGlobs); var tokenBudgeted = activeModel.maxInputTokens() > 0; var diffResult = tokenBudgeted @@ -170,7 +177,7 @@ ReviewContext load( tokenBudgeted ? new ReviewDiffFormatter.FormattedDiff("", 0) : buildBaseComparisonWithStats( - auth, req.owner(), req.repo(), req.baseSha(), req.commitSha(), true); + auth, req.owner(), req.repo(), req.baseSha(), req.commitSha(), true, ignoreGlobs); var omittedFiles = diffResult.omittedFiles(); // One DiffLineResolver per review, shared by the finding pipeline / backstop / postReview. // Memoized so a no-context path that never touches it (e.g. VerdictBuilder when hasContext is @@ -288,6 +295,24 @@ ReviewOrchestrator.ReviewRequest resolveMissingPrDetails( req.forceSummary()); } + /** + * The ignore set for this review: the deployment-wide {@code review.ignored-files} globs unioned + * with whatever the repository declared in {@code .github/thrillhousebot.yml}. Per-repo patterns + * are strictly additive, and every failure mode (feature off, file absent, YAML malformed, glob + * invalid) collapses back to the global set rather than failing the review. + */ + ReviewDiffFormatter.IgnoreGlobs resolveIgnoreGlobs(ReviewOrchestrator.ReviewRequest req) { + var repoSettings = + SoftLoaders.repoSettings( + repoSettingsResolver, + req.owner(), + req.repo(), + req.defaultBranch(), + req.installationId(), + "review"); + return diffFormatter.ignoreGlobs(repoSettings.ignoredFiles()); + } + /** Stack context is best-effort enrichment — its failure must never fail the review. */ String resolveProjectStack(ReviewOrchestrator.ReviewRequest req) { return SoftLoaders.projectStack( @@ -368,13 +393,30 @@ ReviewDiffFormatter.FormattedDiff buildBaseComparisonWithStats( */ ReviewDiffFormatter.FormattedDiff buildBaseComparisonWithStats( String auth, String owner, String repo, String base, String head, boolean applyLineBudget) { + return buildBaseComparisonWithStats( + auth, owner, repo, base, head, applyLineBudget, diffFormatter.ignoreGlobs(List.of())); + } + + /** + * @param ignoreGlobs the review's effective ignore set (global ∪ per-repo), so the base + * comparison hides exactly what the PR diff hides + */ + ReviewDiffFormatter.FormattedDiff buildBaseComparisonWithStats( + String auth, + String owner, + String repo, + String base, + String head, + boolean applyLineBudget, + ReviewDiffFormatter.IgnoreGlobs ignoreGlobs) { if (base == null || head == null || base.length() < 7 || head.length() < 7) { return new ReviewDiffFormatter.FormattedDiff( "(regression comparison unavailable — refs too short)", 0); } try { var comparison = prClient.compareCommits(auth, ACCEPT, owner, repo, base, head); - return diffFormatter.buildBaseComparisonWithStats(comparison, base, head, applyLineBudget); + return diffFormatter.buildBaseComparisonWithStats( + comparison, base, head, applyLineBudget, ignoreGlobs); } catch (RuntimeException e) { Log.warn("Failed to fetch base comparison, continuing without regression context", e); return new ReviewDiffFormatter.FormattedDiff("(regression comparison unavailable)", 0); diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java index 969c95ce..0b70ff03 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatter.java @@ -42,7 +42,57 @@ @ApplicationScoped public class ReviewDiffFormatter { - private record GlobMatcher(PathMatcher primary, PathMatcher suffix) {} + record GlobMatcher(PathMatcher primary, PathMatcher suffix) {} + + /** + * A compiled set of ignore globs — the single glob-matching implementation in the codebase. The + * deployment-wide {@code thrillhousebot.review.ignored-files} list and the extra patterns a + * repository declares for itself are both compiled and matched through here, so a repository can + * never end up with different matching semantics than the global default. + */ + record IgnoreGlobs(List matchers) { + + static final IgnoreGlobs NONE = new IgnoreGlobs(List.of()); + + IgnoreGlobs { + matchers = List.copyOf(matchers); + } + + static IgnoreGlobs compile(List patterns) { + var compiled = compileGlobMatchers(patterns); + return compiled.isEmpty() ? NONE : new IgnoreGlobs(compiled); + } + + /** + * 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. + */ + IgnoreGlobs union(IgnoreGlobs other) { + if (other.matchers.isEmpty()) { + return this; + } + if (matchers.isEmpty()) { + return other; + } + var merged = new ArrayList(matchers.size() + other.matchers.size()); + merged.addAll(matchers); + merged.addAll(other.matchers); + return new IgnoreGlobs(merged); + } + + boolean matches(String filename) { + if (filename == null || filename.isBlank() || matchers.isEmpty()) { + return false; + } + Path path = Path.of(filename.replace('\\', '/')); + for (GlobMatcher matcher : matchers) { + if (matcher.primary().matches(path) || matchesSuffix(matcher.suffix(), path)) { + return true; + } + } + return false; + } + } /** A formatted diff plus the number of files the line budget dropped (0 when nothing omitted). */ record FormattedDiff(String text, int omittedFiles) { @@ -51,7 +101,7 @@ boolean truncated() { } } - private final List globMatchers; + private final IgnoreGlobs globalGlobs; private final int maxDiffLines; @Inject @@ -61,10 +111,26 @@ public ReviewDiffFormatter(ThrillhouseConfig config) { /** Visible for tests. */ ReviewDiffFormatter(List ignoredPatterns, int maxDiffLines) { - this.globMatchers = compileGlobMatchers(ignoredPatterns); + this.globalGlobs = IgnoreGlobs.compile(ignoredPatterns); this.maxDiffLines = maxDiffLines; } + /** + * The ignore set for one operation: the global {@code review.ignored-files} list unioned with the + * extra globs the repository declared for itself. Compile it once per review and hand the result + * to {@link #reviewableFiles(List, IgnoreGlobs)} so the globs stay walked a single time. + * + *

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 ignoreGlobs(List perRepoPatterns) { + if (perRepoPatterns == null || perRepoPatterns.isEmpty()) { + return globalGlobs; + } + return globalGlobs.union(IgnoreGlobs.compile(perRepoPatterns)); + } + private static List compileGlobMatchers(List patterns) { if (patterns == null || patterns.isEmpty()) { return List.of(); @@ -83,23 +149,14 @@ private static List compileGlobMatchers(List patterns) { : null; matchers.add(new GlobMatcher(primary, suffix)); } catch (InvalidPathException | PatternSyntaxException e) { - Log.warnf(e, "Ignoring invalid review.ignored-files pattern: %s", pattern); + Log.warnf(e, "Ignoring invalid ignored-files glob pattern: %s", pattern); } } return List.copyOf(matchers); } boolean isIgnored(String filename) { - if (filename == null || filename.isBlank() || globMatchers.isEmpty()) { - return false; - } - Path path = Path.of(filename.replace('\\', '/')); - for (GlobMatcher matcher : globMatchers) { - if (matcher.primary().matches(path) || matchesSuffix(matcher.suffix(), path)) { - return true; - } - } - return false; + return globalGlobs.matches(filename); } /** Matches `**`-prefixed patterns against the file name and every sub-path of the file. */ @@ -178,14 +235,27 @@ static String formatPureRenameRollup(List pure return sb.toString(); } - /** Files that are included in AI review scope (non-ignored, non–pure-rename). */ + /** + * Files that are included in AI review scope (non-ignored, non–pure-rename), using the global + * ignore list only. + */ List reviewableFiles( List files) { + return reviewableFiles(files, globalGlobs); + } + + /** + * Same, but scoped by an explicit ignore set — normally {@link #ignoreGlobs(List)} applied to the + * patterns the repository declared, so its own globs are honored on top of the global list. + */ + List reviewableFiles( + List files, IgnoreGlobs globs) { if (files == null || files.isEmpty()) { return List.of(); } + var effective = globs == null ? globalGlobs : globs; return files.stream() - .filter(f -> !isIgnored(f.filename())) + .filter(f -> !effective.matches(f.filename())) .filter(f -> !isPureRename(f)) .toList(); } @@ -341,6 +411,19 @@ FormattedDiff buildBaseComparisonWithStats( String base, String head, boolean applyLineBudget) { + return buildBaseComparisonWithStats(comparison, base, head, applyLineBudget, globalGlobs); + } + + /** + * Same, but scoped by an explicit ignore set so the base comparison hides exactly what the PR + * diff hides for this repository (global ∪ per-repo). + */ + FormattedDiff buildBaseComparisonWithStats( + GitHubPullRequestClient.CompareResponse comparison, + String base, + String head, + boolean applyLineBudget, + IgnoreGlobs globs) { if (comparison.files().isEmpty()) { return new FormattedDiff( "(no changes between " + base.substring(0, 7) + " and " + head.substring(0, 7) + ")", 0); @@ -355,7 +438,10 @@ FormattedDiff buildBaseComparisonWithStats( base.substring(0, 7), head.substring(0, 7), comparison.totalCommits())) .toString(); return formatWithLineBudget( - header, withPatch, namesOf(reviewableFiles(withPatch)), applyLineBudget ? maxDiffLines : 0); + header, + withPatch, + namesOf(reviewableFiles(withPatch, globs)), + applyLineBudget ? maxDiffLines : 0); } /** diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/SoftLoaders.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/SoftLoaders.java index d182640f..252679de 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/SoftLoaders.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/SoftLoaders.java @@ -18,6 +18,8 @@ import dev.thiagogonzaga.thrillhousebot.github.GitHubPullRequestClient; import dev.thiagogonzaga.thrillhousebot.github.InstructionsResolver; import dev.thiagogonzaga.thrillhousebot.github.ProjectStackResolver; +import dev.thiagogonzaga.thrillhousebot.github.RepoSettings; +import dev.thiagogonzaga.thrillhousebot.github.RepoSettingsResolver; import io.quarkus.logging.Log; import java.util.List; @@ -105,4 +107,30 @@ static InstructionsResolver.ResolvedInstructions instructions( return InstructionsResolver.ResolvedInstructions.EMPTY; } } + + /** + * The repository's own structured settings, or {@link RepoSettings#EMPTY}. The resolver already + * degrades internally; this wrapper is the outer guarantee that no per-repo configuration problem + * can reach the review pipeline. + */ + static RepoSettings repoSettings( + RepoSettingsResolver resolver, + String owner, + String repo, + String defaultBranch, + long installationId, + String context) { + try { + var settings = resolver.resolve(owner, repo, defaultBranch, installationId); + return settings != null ? settings : RepoSettings.EMPTY; + } catch (RuntimeException e) { + Log.warnf( + e, + "Repository config resolution failed for %s on %s/%s, continuing with the global settings", + context, + owner, + repo); + return RepoSettings.EMPTY; + } + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java new file mode 100644 index 00000000..affff5e2 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java @@ -0,0 +1,310 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.github; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; +import jakarta.ws.rs.NotFoundException; +import jakarta.ws.rs.ProcessingException; +import jakarta.ws.rs.core.Response; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.concurrent.atomic.AtomicLong; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +/** + * Unit tests for {@link RepoSettingsResolver} — the per-repo structured-settings source added for + * issue #51. Mirrors {@link InstructionsResolverTest}: fallback chain, TTL cache, and the fail-soft + * contract that a missing or malformed config can never break a review. + */ +class RepoSettingsResolverTest { + + @Mock private GitHubAuthClient authClient; + @Mock private GitHubPullRequestClient prClient; + @Mock private ThrillhouseConfig config; + @Mock private ThrillhouseConfig.ReviewConfig reviewConfig; + + private final AtomicLong currentTimeMs = new AtomicLong(1_000_000L); + + private static final String OWNER = "test-owner"; + private static final String REPO = "test-repo"; + private static final String DEFAULT_BRANCH = "main"; + private static final long INSTALLATION_ID = 42L; + private static final String AUTH_HEADER = "Bearer test-jwt"; + private static final String ACCEPT = "application/vnd.github+json"; + private static final String YML = ".github/thrillhousebot.yml"; + private static final String YAML = ".github/thrillhousebot.yaml"; + + @BeforeEach + void setUp() { + MockitoAnnotations.openMocks(this); + when(config.review()).thenReturn(reviewConfig); + when(reviewConfig.repoConfigEnabled()).thenReturn(true); + lenient().when(authClient.getAuthHeader(INSTALLATION_ID)).thenReturn(AUTH_HEADER); + } + + private RepoSettingsResolver resolver() { + return new RepoSettingsResolver(config, authClient, prClient, currentTimeMs::get); + } + + private static GitHubPullRequestClient.FileContent content(String text) { + var encoded = Base64.getEncoder().encodeToString(text.getBytes(StandardCharsets.UTF_8)); + return new GitHubPullRequestClient.FileContent( + "thrillhousebot.yml", ".github/thrillhousebot.yml", encoded, "base64", text.length()); + } + + private void stubFile(String path, String text) { + when(prClient.getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, path, DEFAULT_BRANCH)) + .thenReturn(content(text)); + } + + private void stubMissing(String path) { + when(prClient.getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, path, DEFAULT_BRANCH)) + .thenThrow(new NotFoundException(Response.status(404).build())); + } + + private RepoSettings resolve() { + return resolver().resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID); + } + + @Nested + class Parsing { + + @Test + void readsIgnoredFilesFromTheYmlFile() { + stubFile( + YML, + """ + review: + ignored-files: + - "docs/generated/**" + - "**/*.snap" + """); + + var settings = resolve(); + + assertEquals(java.util.List.of("docs/generated/**", "**/*.snap"), settings.ignoredFiles()); + assertEquals(YML, settings.source()); + assertTrue(settings.isPresent()); + } + + @Test + void fallsBackToTheYamlExtension() { + stubMissing(YML); + stubFile( + YAML, + """ + review: + ignored-files: + - "vendored/**" + """); + + var settings = resolve(); + + assertEquals(java.util.List.of("vendored/**"), settings.ignoredFiles()); + assertEquals(YAML, settings.source()); + } + + @Test + void acceptsACommaSeparatedScalarLikeTheEnvVarForm() { + stubFile(YML, "review:\n ignored-files: \"docs/generated/**, **/*.snap\"\n"); + + var settings = resolve(); + + assertEquals(java.util.List.of("docs/generated/**", "**/*.snap"), settings.ignoredFiles()); + } + + @Test + void ignoresAConfigWithoutTheReviewKey() { + stubFile(YML, "something-else:\n enabled: true\n"); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void capsHowManyPatternsARepositoryMayContribute() { + var sb = new StringBuilder("review:\n ignored-files:\n"); + for (var i = 0; i < RepoSettingsParser.MAX_PATTERNS + 25; i++) { + sb.append(" - \"dir").append(i).append("/**\"\n"); + } + stubFile(YML, sb.toString()); + + var settings = resolve(); + + assertEquals(RepoSettingsParser.MAX_PATTERNS, settings.ignoredFiles().size()); + } + + @Test + void dropsBlankAndOverLongPatterns() { + var overLong = "x".repeat(RepoSettingsParser.MAX_PATTERN_LENGTH + 1); + stubFile( + YML, + "review:\n ignored-files:\n - \" \"\n - \"" + + overLong + + "\"\n - \"kept/**\"\n"); + + var settings = resolve(); + + assertEquals(java.util.List.of("kept/**"), settings.ignoredFiles()); + } + } + + @Nested + class FailSoft { + + @Test + void returnsEmptyWhenNoConfigFileExists() { + stubMissing(YML); + stubMissing(YAML); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void malformedYamlDegradesToEmptyWithoutThrowing() { + stubFile(YML, "review:\n ignored-files: [unterminated\n : : :\n"); + + var settings = assertDoesNotThrow(RepoSettingsResolverTest.this::resolve); + + assertEquals(RepoSettings.EMPTY, settings); + } + + @Test + void nonMappingYamlDegradesToEmpty() { + stubFile(YML, "- just\n- a\n- list\n"); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void wrongShapeForIgnoredFilesDegradesToEmpty() { + stubFile(YML, "review:\n ignored-files:\n nested: mapping\n"); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void transportFailureDegradesToEmpty() { + when(prClient.getFileContent(any(), any(), any(), any(), any(), any())) + .thenThrow(new ProcessingException("connection reset")); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void undecodableContentDegradesToEmpty() { + when(prClient.getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, YML, DEFAULT_BRANCH)) + .thenReturn( + new GitHubPullRequestClient.FileContent( + "thrillhousebot.yml", YML, "!!! not base64 !!!", "base64", 3)); + stubMissing(YAML); + + assertEquals(RepoSettings.EMPTY, assertDoesNotThrow(RepoSettingsResolverTest.this::resolve)); + } + + @Test + void doesNotFetchAnythingWhenTheFeatureIsDisabled() { + when(reviewConfig.repoConfigEnabled()).thenReturn(false); + + assertEquals(RepoSettings.EMPTY, resolve()); + verifyNoInteractions(prClient); + verifyNoInteractions(authClient); + } + } + + @Nested + class Caching { + + @Test + void cachesPerRepositoryAndRefetchesAfterTheTtl() { + stubFile(YML, "review:\n ignored-files:\n - \"gen/**\"\n"); + var resolver = resolver(); + + assertEquals( + java.util.List.of("gen/**"), + resolver.resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID).ignoredFiles()); + assertEquals( + java.util.List.of("gen/**"), + resolver.resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID).ignoredFiles()); + + verify(prClient, times(1)) + .getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, YML, DEFAULT_BRANCH); + + currentTimeMs.addAndGet(RepoSettingsResolver.CACHE_TTL_MS + 1); + resolver.resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID); + + verify(prClient, times(2)) + .getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, YML, DEFAULT_BRANCH); + } + + @Test + void cachesAreKeyedPerRepository() { + stubFile(YML, "review:\n ignored-files:\n - \"gen/**\"\n"); + when(prClient.getFileContent(AUTH_HEADER, ACCEPT, OWNER, "other", YML, DEFAULT_BRANCH)) + .thenReturn(content("review:\n ignored-files:\n - \"other/**\"\n")); + var resolver = resolver(); + + assertEquals( + java.util.List.of("gen/**"), + resolver.resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID).ignoredFiles()); + assertEquals( + java.util.List.of("other/**"), + resolver.resolve(OWNER, "other", DEFAULT_BRANCH, INSTALLATION_ID).ignoredFiles()); + } + + @Test + void negativeResultIsCachedBrieflyThenRetried() { + stubMissing(YML); + stubMissing(YAML); + var resolver = resolver(); + + resolver.resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID); + resolver.resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID); + + verify(prClient, times(1)) + .getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, YML, DEFAULT_BRANCH); + + currentTimeMs.addAndGet(RepoSettingsResolver.NEGATIVE_CACHE_TTL_MS + 1); + resolver.resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID); + + verify(prClient, times(2)) + .getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, YML, DEFAULT_BRANCH); + } + + @Test + void sweepDropsExpiredEntriesOnceTheCacheIsLarge() { + stubMissing(YML); + stubMissing(YAML); + var resolver = resolver(); + for (var i = 0; i < RepoSettingsResolver.CACHE_SWEEP_THRESHOLD; i++) { + resolver.resolve(OWNER, "repo-" + i, DEFAULT_BRANCH, INSTALLATION_ID); + } + assertEquals(RepoSettingsResolver.CACHE_SWEEP_THRESHOLD, resolver.cache.size()); + + currentTimeMs.addAndGet(RepoSettingsResolver.CACHE_TTL_MS + 1); + resolver.sweepExpiredEntries(); + + assertEquals(0, resolver.cache.size()); + } + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java index f5efe3df..61fc703d 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java @@ -46,6 +46,7 @@ class ReviewContextLoaderTest { @Mock private GitHubReviewClient reviewClient; @Mock private GitHubCommentClient commentClient; @Mock private InstructionsResolver instructionsResolver; + @Mock private RepoSettingsResolver repoSettingsResolver; @Mock private ProjectStackResolver projectStackResolver; @Mock private PrLabeler labeler; @Mock private FollowUpAnalyzer followUpAnalyzer; @@ -67,6 +68,7 @@ void setUp() { reviewClient, commentClient, instructionsResolver, + repoSettingsResolver, projectStackResolver, diffFormatter, labeler, @@ -382,6 +384,73 @@ void budgetingDisabledStillBuildsLineCappedDiff() { assertTrue(ctx.diff().contains("### a.java")); assertEquals(0, ctx.omittedFiles()); } + + /** + * Per-repo ignore globs (#51): what the repository declares in {@code + * .github/thrillhousebot.yml} is unioned with the deployment-wide list before the + * reviewable-file set is computed, so the extra paths never reach the model. + */ + @Test + void perRepoIgnorePatternsNarrowTheReviewableFileSet() { + var files = + List.of( + new GitHubPullRequestClient.FileDiff( + "src/App.java", "modified", 1, 0, 1, "@@ -1 +1 @@\n+a"), + new GitHubPullRequestClient.FileDiff( + "docs/generated/api.md", "modified", 90, 0, 90, "@@ -1 +1 @@\n+gen")); + stubCommonLoadDeps(files); + when(repoSettingsResolver.resolve("owner", "repo", "main", 99L)) + .thenReturn(new RepoSettings(List.of("docs/generated/**"), ".github/thrillhousebot.yml")); + var session = ReviewSession.create("owner/repo", 1, "Title", "headsha1"); + session.id = 1L; + + var ctx = loader.load("auth", request(), session, "owner/repo"); + + assertEquals(1, ctx.reviewableFiles().size()); + assertEquals("src/App.java", ctx.reviewableFiles().get(0).filename()); + assertTrue( + ctx.diff().contains("(docs/generated/api.md skipped: matches ignored pattern"), + ctx.diff()); + assertFalse(ctx.diff().contains("+gen"), ctx.diff()); + } + + @Test + void repoWithNoDeclaredPatternsKeepsEveryFileTheGlobalListAllows() { + var files = + List.of( + new GitHubPullRequestClient.FileDiff( + "src/App.java", "modified", 1, 0, 1, "@@ -1 +1 @@\n+a"), + new GitHubPullRequestClient.FileDiff( + "docs/generated/api.md", "modified", 90, 0, 90, "@@ -1 +1 @@\n+gen")); + stubCommonLoadDeps(files); + when(repoSettingsResolver.resolve("owner", "repo", "main", 99L)) + .thenReturn(RepoSettings.EMPTY); + var session = ReviewSession.create("owner/repo", 1, "Title", "headsha1"); + session.id = 1L; + + var ctx = loader.load("auth", request(), session, "owner/repo"); + + assertEquals(2, ctx.reviewableFiles().size()); + assertTrue(ctx.diff().contains("+gen"), ctx.diff()); + } + + @Test + void aFailingRepoSettingsResolverFallsBackToTheGlobalListInsteadOfFailingTheReview() { + var files = + List.of( + new GitHubPullRequestClient.FileDiff( + "src/App.java", "modified", 1, 0, 1, "@@ -1 +1 @@\n+a")); + stubCommonLoadDeps(files); + when(repoSettingsResolver.resolve("owner", "repo", "main", 99L)) + .thenThrow(new IllegalStateException("boom")); + var session = ReviewSession.create("owner/repo", 1, "Title", "headsha1"); + session.id = 1L; + + var ctx = assertDoesNotThrow(() -> loader.load("auth", request(), session, "owner/repo")); + + assertEquals(1, ctx.reviewableFiles().size()); + assertEquals("src/App.java", ctx.reviewableFiles().get(0).filename()); + } } @Nested diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java index f413fe15..07d3ecfc 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java @@ -140,6 +140,92 @@ void shouldExcludeIgnoredFilesFromReviewableList() { } } + /** + * Per-repo ignore globs (#51): a repository extends the deployment-wide list through {@code + * .github/thrillhousebot.yml}, and the effective set is the union of the two. + */ + @Nested + class PerRepoIgnorePatterns { + + private final ReviewDiffFormatter formatter = + new ReviewDiffFormatter(List.of("**/*.lock"), 5000); + + private final GitHubPullRequestClient.FileDiff lockFile = + file("deps.lock", "modified", 3, 1, "@@ -1 +1 @@\n+lock"); + private final GitHubPullRequestClient.FileDiff fixture = + file("test/fixtures/big.json", "modified", 900, 0, "@@ -1 +1 @@\n+fixture"); + private final GitHubPullRequestClient.FileDiff source = + file("src/Main.java", "modified", 5, 1, "@@ -1 +1,2 @@\n+ok"); + + @Test + void repoDeclaredPatternTakesEffect() { + var globs = formatter.ignoreGlobs(List.of("test/fixtures/**")); + + var reviewable = formatter.reviewableFiles(List.of(fixture, source), globs); + + assertEquals(1, reviewable.size()); + assertEquals("src/Main.java", reviewable.get(0).filename()); + } + + @Test + void repoThatDeclaresNothingKeepsGlobalOnlyBehavior() { + var files = List.of(lockFile, fixture, source); + + // The fixture is only excluded by a per-repo pattern, so with none declared it stays in + // scope and only the globally ignored lockfile is dropped. + for (var globs : List.of(formatter.ignoreGlobs(List.of()), formatter.ignoreGlobs(null))) { + var reviewable = formatter.reviewableFiles(files, globs); + + assertEquals(2, reviewable.size()); + assertEquals("test/fixtures/big.json", reviewable.get(0).filename()); + assertEquals("src/Main.java", reviewable.get(1).filename()); + } + assertEquals( + formatter.reviewableFiles(files), + formatter.reviewableFiles(files, formatter.ignoreGlobs(List.of())), + "an empty per-repo list must resolve back to the global set"); + } + + @Test + void effectiveSetIsTheUnionOfGlobalAndPerRepoPatterns() { + var globs = formatter.ignoreGlobs(List.of("test/fixtures/**")); + + var reviewable = formatter.reviewableFiles(List.of(lockFile, fixture, source), globs); + + // A file matching EITHER list is skipped; per-repo patterns never replace the global ones. + assertEquals(1, reviewable.size()); + assertEquals("src/Main.java", reviewable.get(0).filename()); + } + + @Test + void malformedRepoPatternIsDroppedWithoutFailingTheReview() { + var globs = + assertDoesNotThrow( + () -> formatter.ignoreGlobs(List.of("[unclosed", " ", "test/fixtures/**"))); + + var reviewable = formatter.reviewableFiles(List.of(lockFile, fixture, source), globs); + + // The invalid glob is skipped; the valid per-repo glob and the global list both still apply. + assertEquals(1, reviewable.size()); + assertEquals("src/Main.java", reviewable.get(0).filename()); + } + + @Test + void perRepoPatternsAlsoScopeTheBaseComparison() { + var globs = formatter.ignoreGlobs(List.of("test/fixtures/**")); + var comparison = new GitHubPullRequestClient.CompareResponse(1, List.of(fixture, source)); + + var result = + formatter.buildBaseComparisonWithStats(comparison, "abcdefgh", "hijklmno", true, globs); + + assertTrue( + result.text().contains("(test/fixtures/big.json skipped: matches ignored pattern"), + result.text()); + assertTrue(result.text().contains("+ok")); + assertFalse(result.text().contains("+fixture")); + } + } + @Nested class TruncationHelpers { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index 52b2e654..455d2bf4 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -76,6 +76,7 @@ class ReviewOrchestratorTest { @Mock private ReviewThreadService reviewThreadService; @Mock private InstructionsResolver instructionsResolver; + @Mock private RepoSettingsResolver repoSettingsResolver; @Mock private ProjectStackResolver projectStackResolver; @@ -226,6 +227,7 @@ private ReviewOrchestrator newOrchestrator() { reviewClient, commentClient, instructionsResolver, + repoSettingsResolver, projectStackResolver, diffFormatter, labeler, From 70aa5110d29b0e943eb0b4dcd00d89c10cae8824 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sat, 8 Aug 2026 01:40:06 +0000 Subject: [PATCH 2/2] fix(docs): mirror repository configuration onto the docs site and cover its branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs build failed: starlight-links-validator rejected the #repository-configuration cross-reference in the config table. That table is mirrored into website/src/content/docs/configuration.md by the remark-include plugin, so an anchor used inside the docs:configuration block has to resolve on that rendered page — every other anchor in the block already targets a heading that is included alongside it. Wrap the Repository configuration section in its own docs markers and include it on the configuration page, between the config table that links to it and the PR labels section, matching the README's own order. The section's opening line said "the instructions file above", which only held in the README, so it now names the file outright and reads correctly on both pages. Also close the patch-coverage gaps in the same feature. Three of them were dead defensive branches rather than untested behaviour, and are removed: readTree never returns null for non-blank input (an ObjectNode pattern match now rejects a null, missing, scalar or sequence root in one test), path() never returns null, and the entries feeding sanitize come from asText()/String.split so they are never null. readPatterns switches on the node type, which states the four shapes it accepts directly and drops the compound early-return. RepoSettings.isPresent was speculative API with no production caller and is gone. The rest were genuinely untested fail-soft paths, now covered: a blank or comment-only config file, an explicitly empty ignored-files key, a non-scalar entry inside the list, a response carrying no content (which would have NPEd in the base64 decode), the @Inject constructor CDI actually uses, a per-repo list whose patterns are all invalid, and a null ignore set. Every file the feature touches is now fully covered. Refs #51 --- README.md | 12 +- .../thrillhousebot/github/RepoSettings.java | 5 - .../github/RepoSettingsParser.java | 45 +++++--- .../github/RepoSettingsResolver.java | 17 ++- .../github/RepoSettingsResolverTest.java | 108 +++++++++++++++++- .../review/ReviewDiffFormatterTest.java | 26 +++++ website/src/content/docs/configuration.md | 2 + 7 files changed, 182 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index ce96e3a2..322efe95 100644 --- a/README.md +++ b/README.md @@ -422,13 +422,14 @@ Place a `.github/thrillhousebot.md` file in any repo to customize the review: Fallback chain: `.github/thrillhousebot.md` → `.github/copilot-instructions.md` → `CLAUDE.md` → `AGENTS.md` → `AGENT.md` + ## Repository configuration -The instructions file above is prose for the model. Structured settings live in a -separate, optional `.github/thrillhousebot.yml` (`.github/thrillhousebot.yaml` also -works) — kept apart on purpose, because the instructions fallback chain may land on -a file owned by another tool, and its whole content is fed to the model as untrusted -prose: +The instructions file (`.github/thrillhousebot.md`) is prose for the model. +Structured settings live in a separate, optional `.github/thrillhousebot.yml` +(`.github/thrillhousebot.yaml` also works) — kept apart on purpose, because the +instructions fallback chain may land on a file owned by another tool, and its whole +content is fed to the model as untrusted prose: ```yaml review: @@ -453,6 +454,7 @@ unexpected shape, or an uncompilable glob is logged and skipped, leaving the glo list in force — it never fails a review. Operators who do not want repositories adjusting their own review scope can turn the whole mechanism off with `THRILLHOUSEBOT_REVIEW_REPO_CONFIG_ENABLED=false`. + ## PR labels diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java index 8b0c4373..d489aeea 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettings.java @@ -39,9 +39,4 @@ public record RepoSettings(List ignoredFiles, String source) { public RepoSettings { ignoredFiles = List.copyOf(ignoredFiles); } - - /** Whether a config file was found and yielded at least one setting. */ - public boolean isPresent() { - return !ignoredFiles.isEmpty(); - } } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java index e7cdbdea..ff9a507d 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java @@ -17,6 +17,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; import java.io.IOException; import java.util.ArrayList; @@ -84,8 +85,9 @@ static RepoSettings parse(String yaml, String source) { return RepoSettings.EMPTY; } try { - var root = YAML_MAPPER.readTree(yaml); - if (root == null || !root.isObject()) { + // Pattern match rather than isObject(): one test rejects a scalar or sequence document + // (which carries no settings) and a null/missing root alike. + if (!(YAML_MAPPER.readTree(yaml) instanceof ObjectNode root)) { log.warn("Repository config {} is not a YAML mapping; ignoring it", source); return RepoSettings.EMPTY; } @@ -106,30 +108,37 @@ static RepoSettings parse(String yaml, String source) { * written as an environment variable. Anything else is ignored. */ private static List readPatterns(JsonNode node, String source) { - if (node == null || node.isMissingNode() || node.isNull()) { - return List.of(); - } - var raw = new ArrayList(); - if (node.isArray()) { - for (var element : node) { - if (element.isValueNode()) { - raw.add(element.asText()); - } + return switch (node.getNodeType()) { + // path() yields a MissingNode when the key is absent, and `ignored-files:` with no value + // yields a NullNode. Both mean the repository declared nothing — not a malformed config. + case MISSING, NULL -> List.of(); + case ARRAY -> sanitize(scalarEntries(node), source); + // A lone scalar is split on commas, matching how the global key is written as an env var. + case STRING -> sanitize(List.of(node.asText().split(",")), source); + default -> { + log.warn("Repository config {}: review.ignored-files is not a list; ignoring it", source); + yield List.of(); + } + }; + } + + /** The scalar entries of a sequence; a nested mapping or sequence entry is not a glob. */ + private static List scalarEntries(JsonNode array) { + var raw = new ArrayList(array.size()); + for (var element : array) { + if (element.isValueNode()) { + raw.add(element.asText()); } - } else if (node.isValueNode()) { - raw.addAll(List.of(node.asText().split(","))); - } else { - log.warn("Repository config {}: review.ignored-files is not a list; ignoring it", source); - return List.of(); } - return sanitize(raw, source); + return raw; } /** Trims, drops blank/oversized entries, and caps the total a repository may contribute. */ private static List sanitize(List raw, String source) { var patterns = new ArrayList(Math.min(raw.size(), MAX_PATTERNS)); for (String value : raw) { - var pattern = value == null ? "" : value.trim(); + // Never null: entries come from asText() (empty string at worst) or String.split. + var pattern = value.trim(); if (pattern.isEmpty()) { continue; } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java index 8b48a927..8d04e022 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java @@ -134,10 +134,19 @@ public RepoSettings resolve( } /** - * Fetches and parses one candidate path. Returns {@code null} when the file is absent or could - * not be read — the caller then tries the next name in the chain. A file that exists but is - * malformed parses to {@link RepoSettings#EMPTY}, which is a real (cacheable) answer: the - * repository has spoken, it just said nothing usable. + * Fetches and parses one candidate path. The two outcomes are deliberately different: + * + *

    + *
  • {@code null} — nothing readable came back at all: the file is absent (404), the request + * failed, or the payload could not be decoded into text. The caller moves on to the next + * name in the chain, because a second candidate may still hold a usable config. + *
  • {@link RepoSettings#EMPTY} — the file was read, and the parse found no usable settings + * (malformed YAML, wrong shape, no {@code review.ignored-files}). That is a real, cacheable + * answer that ends the chain: the repository has spoken, it just said nothing usable. + *
+ * + *

Either way the effective result is the global list, so the distinction only decides how many + * candidates are tried — never whether a review proceeds. */ private RepoSettings fetchAndParse( String auth, String owner, String repo, String defaultBranch, String path) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java index affff5e2..137ebd53 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolverTest.java @@ -105,7 +105,6 @@ void readsIgnoredFilesFromTheYmlFile() { assertEquals(java.util.List.of("docs/generated/**", "**/*.snap"), settings.ignoredFiles()); assertEquals(YML, settings.source()); - assertTrue(settings.isPresent()); } @Test @@ -167,6 +166,32 @@ void dropsBlankAndOverLongPatterns() { assertEquals(java.util.List.of("kept/**"), settings.ignoredFiles()); } + + @Test + void ignoresAnExplicitlyEmptyIgnoredFilesKey() { + // `ignored-files:` with no value parses to a NullNode, not a missing node. + stubFile(YML, "review:\n ignored-files:\n"); + stubMissing(YAML); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void skipsNonScalarEntriesInsideTheList() { + stubFile( + YML, + """ + review: + ignored-files: + - "kept/**" + - nested: mapping + - ["a", "b"] + """); + + var settings = resolve(); + + assertEquals(java.util.List.of("kept/**"), settings.ignoredFiles()); + } } @Nested @@ -196,6 +221,38 @@ void nonMappingYamlDegradesToEmpty() { assertEquals(RepoSettings.EMPTY, resolve()); } + @Test + void blankConfigFileDegradesToEmpty() { + stubFile(YML, " \n\n"); + stubMissing(YAML); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void commentOnlyConfigFileDegradesToEmpty() { + // Parses to a missing root — no mapping, so nothing to read. + stubFile(YML, "# nothing configured yet\n"); + stubMissing(YAML); + + assertEquals(RepoSettings.EMPTY, resolve()); + } + + @Test + void nullFileContentIsTreatedAsAbsentAndFallsThroughTheChain() { + // The contents endpoint can answer without an inline payload (e.g. an over-size blob); + // decoding null would NPE, so it must be handled as "nothing readable here". + when(prClient.getFileContent(AUTH_HEADER, ACCEPT, OWNER, REPO, YML, DEFAULT_BRANCH)) + .thenReturn( + new GitHubPullRequestClient.FileContent("thrillhousebot.yml", YML, null, "none", 0)); + stubFile(YAML, "review:\n ignored-files:\n - \"from-yaml/**\"\n"); + + var settings = assertDoesNotThrow(RepoSettingsResolverTest.this::resolve); + + assertEquals(java.util.List.of("from-yaml/**"), settings.ignoredFiles()); + assertEquals(YAML, settings.source()); + } + @Test void wrongShapeForIgnoredFilesDegradesToEmpty() { stubFile(YML, "review:\n ignored-files:\n nested: mapping\n"); @@ -232,6 +289,55 @@ void doesNotFetchAnythingWhenTheFeatureIsDisabled() { } } + /** + * Direct contract tests for the parsing seam {@link RepoSettingsParser}, which #33 will reuse for + * its own settings: it must answer {@link RepoSettings#EMPTY} for any input rather than throw. + */ + @Nested + class ParserContract { + + @Test + void nullInputYieldsEmptyRatherThanThrowing() { + assertEquals( + RepoSettings.EMPTY, assertDoesNotThrow(() -> RepoSettingsParser.parse(null, YML))); + } + + @Test + void aReadableFileWithNoUsableSettingsIsAttributedToNoSource() { + // EMPTY carries source "none": only a config that actually yielded settings names its file. + assertEquals("none", RepoSettingsParser.parse("review: {}\n", YML).source()); + } + } + + /** The {@code @Inject} constructor CDI actually uses, which the other tests bypass. */ + @Nested + class InjectionConstructor { + + @Test + void wiresConfigAndClientsAndResolvesWithTheSystemClock() { + stubFile(YML, "review:\n ignored-files:\n - \"gen/**\"\n"); + + var settings = + new RepoSettingsResolver(config, authClient, prClient) + .resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID); + + assertEquals(java.util.List.of("gen/**"), settings.ignoredFiles()); + assertEquals(YML, settings.source()); + } + + @Test + void readsTheEnabledFlagFromConfig() { + when(reviewConfig.repoConfigEnabled()).thenReturn(false); + + var settings = + new RepoSettingsResolver(config, authClient, prClient) + .resolve(OWNER, REPO, DEFAULT_BRANCH, INSTALLATION_ID); + + assertEquals(RepoSettings.EMPTY, settings); + verifyNoInteractions(prClient); + } + } + @Nested class Caching { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java index 07d3ecfc..13d77ca4 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDiffFormatterTest.java @@ -210,6 +210,32 @@ void malformedRepoPatternIsDroppedWithoutFailingTheReview() { assertEquals("src/Main.java", reviewable.get(0).filename()); } + @Test + void aRepoListOfOnlyInvalidPatternsFallsBackToExactlyTheGlobalSet() { + var files = List.of(lockFile, fixture, source); + + var globs = + assertDoesNotThrow(() -> formatter.ignoreGlobs(List.of("[unclosed", "{also[bad"))); + + // Nothing compiled, so the union contributes nothing and global-only behaviour remains: + // the lockfile is still dropped and the fixture is still reviewed. + assertEquals(formatter.reviewableFiles(files), formatter.reviewableFiles(files, globs)); + var reviewable = formatter.reviewableFiles(files, globs); + assertEquals(2, reviewable.size()); + assertEquals("test/fixtures/big.json", reviewable.get(0).filename()); + assertEquals("src/Main.java", reviewable.get(1).filename()); + } + + @Test + void aNullIgnoreSetFallsBackToTheGlobalList() { + var reviewable = formatter.reviewableFiles(List.of(lockFile, fixture, source), null); + + // Not "everything is reviewable": the global lockfile glob must still be applied. + assertEquals(2, reviewable.size()); + assertEquals("test/fixtures/big.json", reviewable.get(0).filename()); + assertEquals("src/Main.java", reviewable.get(1).filename()); + } + @Test void perRepoPatternsAlsoScopeTheBaseComparison() { var globs = formatter.ignoreGlobs(List.of("test/fixtures/**")); diff --git a/website/src/content/docs/configuration.md b/website/src/content/docs/configuration.md index 38c13a65..43c0c887 100644 --- a/website/src/content/docs/configuration.md +++ b/website/src/content/docs/configuration.md @@ -5,4 +5,6 @@ description: Every environment variable the bot reads, with defaults. + +