feat(review): support per-repo ignore patterns alongside the global default - #449
Open
devops-thiago wants to merge 2 commits into
Open
feat(review): support per-repo ignore patterns alongside the global default#449devops-thiago wants to merge 2 commits into
devops-thiago wants to merge 2 commits into
Conversation
Contributor
Dependency ReviewThe following issues were found:
License Issuespom.xml
OpenSSF Scorecard
Scanned Files
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds support for per-repository ignore globs via an optional Control-Flow Diagram🔀 Show diagramflowchart TD
A["ReviewContextLoader.load()"] --> B["resolveIgnoreGlobs(req)"]
B --> C["SoftLoaders.repoSettings()"]
C --> D["RepoSettingsResolver.resolve()"]
D --> E{"Feature
enabled?"}
E -->|no| F["return EMPTY"]
E -->|yes| G{"Cache
hit?"}
G -->|yes| H["return cached"]
G -->|no| I["try .yml → .yaml"]
I --> J{"Fetch & parse"}
J -->|success| K["cache 5min, return settings"]
J -->|fail| L["cache EMPTY 1min, return EMPTY"]
D --> M["diffFormatter.ignoreGlobs(patterns)"]
M --> N["globalGlobs.union(perRepo)"]
N --> O["reviewableFiles(files, ignoreGlobs)"]
O --> P["buildBaseComparison with ignoreGlobs"]
P --> Q["ReviewContext"]
Changes Overview
Changed Files
…and 1 more file(s). Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
| build | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: Undecodable content causes fallback to alternate config name, contradicting design (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java:166)
The documented intention is that the first existing config file (.ymlor.yaml) is selected; if it is present but unreadable, it should parse toRepoSettings.EMPTYand not fall back to the alternate name. However,fetchAndParsecatchesRuntimeExceptionfromBase64.getMimeDecoder().decode()and returnsnull, which causes theresolveloop to try the next file path. If both.yml(corrupted) and.yaml(valid) exist, the settings from the.yamlfile are used instead of EMPTY. This deviates from the stated design that a repo's config file, once found, is the authoritative source.
thrillhousebot
Bot
added
enhancement
…efault 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
devops-thiago
force-pushed
the
feat/51-per-repo-ignores
branch
from
August 8, 2026 00:48
677d27f to
2eeb1a3
Compare
…er its branches 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
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
thrillhousebot.review.ignored-filesis app-wide, so a single deployment reviewing many repositories has to pick one list for all of them. This lets a repository declare ignore globs of its own, unioned with (never replacing) the global default.Where per-repo structured settings live — and why
Decision: a dedicated
.github/thrillhousebot.yml(with.github/thrillhousebot.yamlas an alternate name), not frontmatter in the existing.github/thrillhousebot.md..github/copilot-instructions.md,CLAUDE.md,AGENTS.md,AGENT.md). Frontmatter would mean writing ThrillhouseBot config into whichever of those happened to win — and which one wins varies per repository.This is a substrate for #33 (path-scoped review instructions), which will need per-repo structured settings from the same place. The seam is:
RepoSettings— the settings record. feat(review): path-scoped review instructions #33 adds a component here.RepoSettingsParser— YAML text →RepoSettings. feat(review): path-scoped review instructions #33 adds a reader for its own key under the samereview:map.RepoSettingsResolver— fetch + two-name chain + per-repo TTL cache + fail-soft. feat(review): path-scoped review instructions #33 needs no changes here.Note that #33 is about review rules per path, whereas this controls what is sent to the model at all; they stay distinct.
Shape
Precedence: global ∪ per-repo. A file is skipped if it matches either list. A repository can take more files out of review scope, never put back a file the deployment excludes.
Implementation notes
GlobMatcher/compileGlobMatchers/**-suffix behaviour inReviewDiffFormatteris now wrapped in anIgnoreGlobsvalue type that both the global list and the per-repo list compile through, so a repository can never get different matching semantics than the deployment default.union()is the additive operation.ReviewDiffFormatteris@ApplicationScopedwith its patterns fixed at construction, so per-repo patterns could not be baked in. Instead the effectiveIgnoreGlobsis resolved once inReviewContextLoader.load(...)and threaded into the singlereviewableFiles(...)call (and the base comparison), preserving the existing compute-once property.RepoSettingsResolvermirrorsInstructionsResolver— 5-minute TTL, 1-minute negative cache, size-triggered sweep,LongSupplierclock for tests.EMPTY; file absent → next name, thenEMPTY; transport error →EMPTY; undecodable content →EMPTY; malformed YAML or unexpected shape →EMPTY(RepoSettingsParsernever throws); uncompilable glob → dropped by the existingcompileGlobMatcherscatch; andSoftLoaders.repoSettings(...)is the outer guarantee.thrillhousebot.review.repo-config-enabled(defaulttrue) is the operator kill switch, following theadd-docs-enabledprecedent. Documented inREADME.mdand.env.example.jackson-dataformat-yamlwas already on the compile classpath viaquarkus-smallrye-openapiand version-managed by the existingjackson-bomimport; it is now declared explicitly inpom.xmlbecause it is used directly. No version was added or bumped.Deliberately out of scope
The on-demand commands (
/describe,/changelog,/add-docs, maintainer replies) still use the global list only — they build their diffs through separate call paths, andMaintainerReplyServicehas no default branch in its task to resolve the config with. Extending them is mechanical now that the seam exists (diffFormatter.ignoreGlobs(settings.ignoredFiles())plus thereviewableFiles(files, globs)overload) and is best done as a follow-up rather than widening this diff.Related Issues
Fixes #51
Related: #33 (path-scoped review instructions) will build on
RepoSettings/RepoSettingsParser/RepoSettingsResolver.How Has This Been Tested?
New tests:
RepoSettingsResolverTest(17 cases: parsing,.yml→.yamlfallback, caps, fail-soft, TTL / negative cache / sweep, disabled flag),ReviewDiffFormatterTest$PerRepoIgnorePatterns(5 cases), and 3 end-to-endload(...)cases inReviewContextLoaderTest.Each new behaviour was validated red/green. With the tests in the tree, the production behaviour was neutralized while keeping the API (so the tests still compiled and ran, rather than failing to build):
ReviewDiffFormatter.ignoreGlobs(...)reduced toreturn globalGlobs;,ReviewContextLoader.loadreverted todiffFormatter.reviewableFiles(files), andRepoSettingsResolver.resolveshort-circuited toRepoSettings.EMPTY.Red phase — actual failures produced:
Two of the new tests pass in both phases by design — they are the "global-only behaviour is unchanged" regression guards (
repoThatDeclaresNothingKeepsGlobalOnlyBehaviorandrepoWithNoDeclaredPatternsKeepsEveryFileTheGlobalListAllows); they must hold before and after.With the production change restored, all of the above pass. Gates run locally:
./mvnw -B spotless:apply— clean./mvnw -B clean compile spotbugs:check spotless:check— BUILD SUCCESS,BugInstance size is 0./mvnw -B clean test— BUILD SUCCESS,Tests run: 1902, Failures: 0, Errors: 0, Skipped: 0Checklist
Additional Notes
The native build only runs on
main, not on PRs. Residual risk there is low —snakeyamlandjackson-dataformat-yamlare already exercised at runtime in the native image byquarkus-smallrye-openapi, and this code path usesreadTreewith no reflective POJO binding — but it is worth a glance on the firstmainbuild after merge.