fix(review): read ignore globs as the gitignore syntax they are documented as - #790
Open
devops-thiago wants to merge 2 commits into
Open
fix(review): read ignore globs as the gitignore syntax they are documented as#790devops-thiago wants to merge 2 commits into
devops-thiago wants to merge 2 commits into
Conversation
…ented as
README and RepoSettings promise maintainers that per-repo ignore globs use the
same gitignore-style syntax as the deployment-wide key. The matcher was raw Java
NIO globs plus a `**/`-prefix fallback, under which the common idioms silently
matched nothing: `build/` matched no file at all, a bare `vendor` matched only a
file literally named that, `*.lock` matched only a root-level lockfile, and a
leading `/` compiled to an absolute path that could never match a repo-relative
one. Every one of those failed open — a path the repository asked to exclude was
sent to the model anyway — and failed silently, because compileGlobMatchers only
warns on a pattern that fails to compile, and a pattern that matches nothing is
indistinguishable from one that had nothing to match.
Declared patterns are normalized to gitignore's own reading before compiling:
anchored when they carry a `/`, at any depth when they do not, a trailing `/`
meaning that directory's tree, and a name with no trailing slash meaning a file
or a directory. Negation is dropped with a warning rather than compiled into a
matcher for a file named `!…`, since the effective set is a union nothing may
subtract from. Both keys and the path-scoped rules share the one matcher, so
they cannot drift apart, and the shipped default list is pinned by test to
exclude exactly what it excluded before — starting with its first entry,
`**/pom.xml`, whose reach was never asserted anywhere.
The other half of a dead glob is that nobody finds out. The summary's review
scope note now names the repository's own globs that matched no file in the pull
request, alongside the pure-rename rollup that already answers the same
question.
The parser gaps the same audit found are fixed with it. A brace glob survived
neither the scalar comma-split (`**/*.{js,ts}` became two fragments, one
uncompilable and one a matcher for a file named `ts}`) nor, therefore, the
review; the split now ignores commas inside a brace group. A single duplicate
key threw out of the whole parse and discarded every setting in the file, far
wider than this parser's per-entry rule everywhere else; it is warned about and
re-read with YAML's last-one-wins. Aliases were never resolved at all, because
jackson-dataformat-yaml drives snakeyaml's event parser and never runs its
composer: `*common` arrived as the literal string `common`, and an aliased scope
path became a short literal that passed every check and was rendered into the
review prompt as the repository's rule for those files. Loading through
snakeyaml's own loader resolves aliases and merge keys, and makes the nesting
and alias-expansion ceilings real — they were inert, with comments claiming a
protection that did not exist, which is why the issue asks for them together.
Finally, a transient GitHub failure is no longer cached as "this repository has
no config": WebApplicationException is the parent of ServerErrorException and of
the 403 secondary rate limit as well as of NotFoundException, so a blip pinned
the negative cache for a minute in which every review ran on the deployment
ignore list alone.
The one row of the issue's table not reproduced is `generated/**`, which already
matches the root-level tree today; gitignore anchors a pattern carrying a slash,
which is also the only reading that keeps the documented `docs/generated/**`
example meaning what it says, so it is left anchored and `**/generated/**`
remains how a maintainer says "at any depth".
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis PR fixes six #481 issues: declared ignore globs are normalized to the gitignore semantics they were documented as (slash patterns anchored to the root, slash-less patterns at any depth, trailing-slash directory trees, negations dropped with a warning) through one shared matcher; declared globs that matched no file in a PR are disclosed in the review summary's scope note; the scalar ignored-files split is brace-aware; duplicate YAML keys degrade to last-one-wins instead of discarding the whole file; YAML anchors/aliases/merge keys are resolved via snakeyaml with the nesting/alias/size guards actually enforced; and only genuine 404s are negative-cached while transient failures (5xx, 403 rate limit, transport errors) are re-asked on the next review. Documentation, changelog, and a large test suite pin the new semantics. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["resolve(): try config-file chain"] --> B["fetchAndParse(path)"]
B --> C{"GitHub outcome?"}
C -- "404" --> D["ABSENT: next candidate"]
C -- "5xx / 403 / transport" --> E["UNREADABLE: set flag"]
C -- "200" --> F["parse: aliases resolved, dup-key re-read"]
D --> G{"more candidates?"}
G -- "yes" --> B
G -- "no" --> H{"unreadable?"}
H -- "yes" --> I["EMPTY, not cached"]
H -- "no" --> J["negative result cached"]
F --> K["IgnoreGlobs.compile via gitignoreForms"]
K --> L["summary scope note lists unmatched globs"]
Changes Overview
Changed Files
…and 1 more file(s). Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| frontend | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| dependency-review | 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}
Contributor
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):
- LOW: Fetched javadoc says found-empty is 'cacheable' but resolve() never caches it (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsResolver.java:157)
The new Fetched record javadoc states: "A parse that found no usable settings still answers RepoSettings.EMPTY here: that is a real, cacheable answer that ends the chain" (line 157). The adjacent code contradicts that: the resolve() loop returns settings directly (return settings;, line 130), before the onlycache.put(...)in the method, which sits after the loop and is reached only when every candidate was absent and no candidate was unreadable. No outcome returned through the found branch — including RepoSettings.EMPTY from a readable-but-empty config (malformed YAML, wrong shape) — is ever written to the negative cache, so such configs are re-fetched on every review. The contradiction is in the safe direction (an uncached parse-empty is re-asked next review instead of being pinned), so there is no correctness impact; but the javadoc asserts a caching property the code never exercises, in a record whose whole point is to classify outcomes by what may be remembered. The ABSENT and UNREADABLE bullets describe real cache behavior; the found bullet does not.
thrillhousebot
Bot
added
bug
…lag, and split the glob compile loop SonarCloud flagged two readability traps in this branch's code. `Fetched.UNREADABLE` and the record's `unreadable` component differ only by case (java:S1845) — exactly the pair a reader is most likely to conflate in a type whose whole job is telling three outcomes apart. The constant becomes READ_FAILED rather than the accessor being renamed: `unreadable()` is already how CiStatusEvaluator's CiEvaluation spells this predicate across the codebase, while the constant is a private single-use sentinel. compileGlobMatchers reached four exits out of one loop body (java:S135). The per-declaration verdicts move into compileDeclaration, which returns where each one is decided, leaving the loop to concatenate. The whole-or-nothing rule is unchanged: every form is still compiled into a local list before any of it is handed back, so a declaration whose later form is invalid still contributes nothing rather than a partial matcher set. Also pins the Fetched javadoc's "cacheable" claim with two tests, after a review read the found branch as returning before any cache write. It does not — the write is inside the loop — so a readable-but-unusable config (malformed YAML, wrong shape) is cached like any other answer, and for the full CACHE_TTL_MS rather than the shorter negative one. That is kept deliberately: it costs a repository with broken YAML the same wait after a fix as a repository whose YAML parses, in exchange for not re-fetching it on every single review. Behaviour is unchanged; the property is now tested rather than only asserted in prose, and the javadoc names where the write happens so the same misreading is not available next time.
|
Contributor
🤖 ThrillhouseBot — changes since the last review
|
Contributor
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: Recursive YAML alias may reach Jackson convertValue and throw an uncatchable Error (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/RepoSettingsParser.java:175)
The new loader path resolves aliases —graph = firstDocument(yaml, loaderOptions(false));thenreturn graph == null ? null : MAPPER.convertValue(graph, JsonNode.class);(line 175). SnakeYAML constructs shared and, for recursive aliases, self-referential object graphs before Jackson ever sees them. Input not in the diff: repository configa: &a {self: *a}ora: &a [*a]— a single alias use, far below thesetMaxAliasesForCollections(50)ceiling, so no guard refuses it.SafeConstructorresolves the alias into a Map/List containing itself;MAPPER.convertValuethen walks the cycle. If Jackson'sWRITE_SELF_REFERENCES_AS_NULLdoes not break it (it is documented to cover only direct self-references), the conversion throwsStackOverflowError— anError, which neither} catch (RuntimeException e) {inparse()(line 144) nor the identicalRuntimeExceptioncatch inRepoSettingsResolver.fetchAndParsecan contain. The result contradicts the parser's own contract, stated in the same file: "It never throws — a malformed config must degrade to 'no per-repo settings', never fail a review", and because the error escapes before anyFetchedis produced, it is not cached either — every review of that repository would hit it. No in-diff test covers a cyclic alias:refusesAnAliasExpansionBombInsteadOfExpandingItis a DAG, not a cycle. Verify by runningRepoSettingsParser.parse("a: &a {self: *a}\n", "test")(and a two-node cycle such asa: &a {b: &b {c: *a}}): if SnakeYAML rejects the recursion at construction time, or Jackson emits null for the back-reference, the never-throws contract holds and this finding is moot.
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
The per-repo and deployment-wide ignore keys are documented as "gitignore-style", but the
matcher was raw Java NIO globs plus a
**/-prefix fallback. Under it the common idiomssilently matched nothing, verified against
mainbefore the fix:build/buildtree at any depthvendorvendorvendor, and its tree, at any depth*.lock/MakefileMakefileonly!path!pathEach of those fails open — the file the repository asked to exclude is sent to the
model anyway — and fails silently, because the compile-time warning only fires on a
pattern that fails to compile. A pattern that matches nothing is indistinguishable from
one that had nothing to match.
Declared patterns are now normalized to gitignore's own reading before compiling
(
gitignoreForms, one pure function): a leading/anchors and is dropped, a/anywhereelse anchors to the root, anything else matches at any depth, a trailing
/means thatdirectory's tree, and a bare name means a file or a directory. The deployment key, a
repository's own list, and the path-scoped rule globs all compile through the one matcher,
so the "same syntax" promise is structural rather than a claim.
The issue's fourth row,
generated/**, was the one claim that did not reproduce: it alreadymatches the root-level tree today. gitignore anchors a pattern carrying a slash, and that is
also the only reading under which the documented
docs/generated/**example keeps meaningwhat it says, so it stays anchored and
**/generated/**remains how you say "at any depth".The issue's own prescribed fix ("when a pattern contains no
/, also compile it as**/<pattern>") is exactly that rule.Disclosure. The other half of a dead glob is that nobody finds out. The summary's
review-scope note now names the repository's globs that matched no file in this PR, sharing
the blockquote with the pure-rename rollup that already answers the same question.
Parser gaps (issue items 2–4), fixed together.
**/*.{js,ts}became**/*.{jsandts}— the first uncompilable, the second a matcher for a file namedts}) nor the review.The split now ignores commas inside a brace group.
file. It is warned about, then re-read with YAML's usual last-one-wins.
never runs its composer, so
*commonarrived as the literal stringcommon— an aliasedignore list became a glob for a file called
common, and an aliased scope path became ashort literal that passed every check and reached the review prompt as the repository's
rule for those files. The document is loaded through snakeyaml's own loader now, which also
makes
setNestingDepthLimitandsetMaxAliasesForCollectionsreal; they were inert, withjavadoc claiming a protection that did not exist, which is why the issue asks for them in
the same change.
Item 5. A transient GitHub failure is no longer negative-cached like a 404.
WebApplicationExceptionis the parent ofServerErrorException(5xx) and of the 403secondary rate limit as well as of
NotFoundException, so a blip pinned "this repository hasno config" for a minute of reviews. Only a real 404 is cached; anything else runs on the
global list for that review and is re-asked on the next.
Nothing here can widen review scope: the effective ignore set is still global ∪ per-repo, and
negation is refused precisely because it would subtract from a union.
Related Issues
Closes #481
How Has This Been Tested?
Every behavioural change was written test-first and run against the unfixed code. The six new
gitignore-idiom tests failed with
expected: <true> but was: <false>(and the negation oneexpected: <false> but was: <true>); the parser tests failed withexpected: <[generated/**, **/*.snap]> but was: <[shared]>,expected: <payments/**> but was: <money>,expected: <[kept/**]> but was: <[]>for the duplicate key,expected: <[**/*.{js,ts}]> but was: <[**/*.{js, ts}]>for the brace glob, and the cache test with "a failure we cannot read isnot an answer to cache". The scope-glob and disclosure tests were proven red against a
temporarily reverted matcher/wiring.
The shipped default ignore list is pinned by a test that reads the
@WithDefaultvalueitself, asserting
**/pom.xmlreaches every pom at every depth and matches no neighbour thatmerely contains the name, plus a sample across every other class of default entry — a change
in matcher semantics cannot quietly alter what the defaults exclude.
Gates:
spotless:apply,clean compile spotbugs:check spotless:check(BugInstance size is0),
clean test— 3498 tests, 0 failures, 0 errors. Patch coverage overgit diff -U0 origin/main: 140 instrumented changed lines, 0 uncovered lines, 0 uncovered branches.Review follow-up: what a readable-but-unusable config does to the cache. A review read
the
Fetchedjavadoc's "cacheable" as contradictingresolve(), on the reading that thefound branch returns before any
cache.put. It does not — the write is inside the loop, andRepoSettingsParser.parsenever returnsnull, so a file that was read but parsed to nothingtakes the found branch and is cached. The javadoc was right; it just did not say where the
write happens, so it now does.
The behaviour is kept as it stands, and the judgement is worth stating rather than assuming.
A malformed config is cached for the full
CACHE_TTL_MS, not the shorter negative TTL,because the repository answered — it just said nothing usable. That does mean a maintainer
who fixes broken YAML waits out the TTL, but that is precisely the same wait as a maintainer
who edits YAML that already parsed, so the broken case is not treated worse than the working
one; and the alternative, leaving it uncached, spends a GitHub read on every review, forever,
for exactly the repositories that get no benefit from it. Two tests now pin both halves (it is
cached at all, and on the long TTL) — they fail with
expected: <1> but was: <0>against acopy of the code with the in-loop
cache.putremoved, so the claim is tested rather thanasserted.
SonarCloud. Two issues on this branch, both fixed rather than suppressed.
Fetched.UNREADABLEdiffered only by case from the record's ownunreadablecomponent(
java:S1845), which is a genuine trap in a type whose whole job is telling three outcomesapart; the constant is renamed
READ_FAILED, keeping the accessor, sinceunreadable()ishow
CiStatusEvaluator.CiEvaluationalready spells this predicate.compileGlobMatchershadfour exits out of one loop body (
java:S135); the per-declaration verdicts moved intocompileDeclaration, which returns where each is decided, and the loop now onlyconcatenates. The whole-or-nothing compile is unchanged — every form is still built into a
local list before any of it is returned.
Checklist
Additional Notes
pom.xmlswaps the explicitjackson-dataformat-yamldeclaration fororg.yaml:snakeyaml(same transitive origin, version-managed by the platform BOM) — the parser is the only place
either was used directly.
Docs updated: the README repository-configuration section gains a pattern table and states the
anchoring rule, the
THRILLHOUSEBOT_REVIEW_IGNORED_FILESrow and theapplication.propertiescomment say the same thing for the deployment key, and both note that a comma-separated value
cannot carry a
{a,b}alternation. The website picks these up through its README include.Base-branch trigger globs (
ReviewTriggerFilter) are deliberately untouched: branch names arenot paths, and that table already documents its
*/**behaviour precisely.