feat(review): support per-repo ignore patterns alongside the global default - #449
Merged
Merged
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
|
devops-thiago
added a commit
that referenced
this pull request
Aug 8, 2026
Absorbs #449 (per-repo ignore patterns), #451 (whole-change-set PR summary), #453 (decline re-check) and four dependency bumps. Two textual conflicts, both from independent additions at the same insertion point rather than any disagreement: - ReviewContextLoader: #449's resolveIgnoreGlobs and this branch's resolveConfigKeyContext are separate private helpers that git could not place. Kept both. - FindingPipelineTest: #451 parameterized the reviewContext helper with an explicit reviewable-file list while this branch added the configKeyContext record component. Kept both — the helper's parameter, with "" in the new component's position. One silent breakage git merged cleanly: #453's new declinedRaceContext helper constructs a ReviewContext without configKeyContext. Filled in. The interaction between the two features is the one worth noting. #449 made load() compute reviewableFiles from the global globs unioned with the repo's own, and config-key resolution already read that post-filter list, so a key documented only in an ignored file is now correctly never resolved — and it inherits per-repo ignore rules for free. Pinned with a test that fails if the raw file list is ever passed instead.
devops-thiago
added a commit
that referenced
this pull request
Aug 8, 2026
/improve was built on the pre-#53 design: one model call over the diff string that ReviewDiffFormatter caps at max-diff-lines. For a whole-PR improvement pass that is the wrong failure mode — a large PR silently shrank to its first N lines, and whole files never reached the model at all. On a change set with a 4-line cap the model received literally "(diff truncated at 4 lines — 2 files omitted)" and no file content. Plan batches with DiffBudgetPlanner over the reviewable file list under the per-call token budget instead, the way the review path has worked since #53, and run one assistant call per batch. The line cap no longer gates coverage; it only shapes the string that is now unused for the model call. Details: - The shared prompt overhead is assembled from this command's own prompts, mirroring plan(reviewable, PromptInputs), so batches sized as in-budget do not overshoot the real input limit. - Results are merged across batches and deduped by file and line, so two batches can never propose the same line twice. - A batch whose call or parse fails is skipped rather than failing the run, and the count is disclosed; only an all-batches failure posts the failure notice. - Coverage disclosure now comes from the plan's omitted and clipped files, named rather than counted, via a new truncationDisclosure overload mirroring truncationNotice's detail variant. - max-input-tokens=0 keeps budgeting off as a single uncapped batch rather than regressing to the line-capped string. - Per-repo ignore patterns (#449) are applied on top of the global set. While the pass stopped at max-diff-lines, an ignored file beyond the cap was excluded by accident; now that every file is in scope it has to be excluded on purpose, or /improve would propose committable edits to code a repository asked the bot to leave alone. Anchoring is unchanged and still resolves against the whole PR's line map, so a suggestion from any batch anchors to its correct absolute line. Refs #316
18 tasks
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.