Skip to content

fix(review): make per-repo ignore globs behave as their documented gitignore-style syntax, and harden the YAML parser #481

Description

@devops-thiago

Problem Statement

An audit of the v0.6.0 repo-settings subsystem (#51 per-repo ignore globs, #33 path-scoped
instructions) found that the matcher does not behave the way its own documentation promises, plus
three narrower parser gaps. All were reproduced against release/v0.6.0 and independently
validated. None is a security or verdict-correctness defect, so they are deferred here rather than
held against the v0.6.0 release — but the first one fails open (paths a repository asked to
exclude are sent to the model anyway), so it is worth fixing before v0.7.0.

1. "gitignore-style" is documented but not implemented — common forms silently match nothing

README.md (the per-repo section, new this milestone) and RepoSettings's javadoc tell maintainers
per-repo globs use "the same gitignore-style syntax as the global key". The matcher is raw Java NIO
FileSystems.getDefault().getPathMatcher("glob:" + pattern) plus a suffix matcher built only when
the pattern starts with **/. Under that matcher, gitignore idioms silently match nothing:

pattern a maintainer writes intended actual
build/ (trailing slash = directory) exclude the tree matches nothing
bare vendor / payments exclude the dir matches nothing (matches only a file literally named that)
*.lock every lockfile matches only a root-level *.lock
generated/** the dir anywhere matches nothing (needs the **/ prefix)

compileGlobMatchers warns only on patterns that fail to compile (InvalidPathException /
PatternSyntaxException); build/ compiles fine, so there is no signal at any log level, and a
path-scoped glob that matches nothing is dropped with no log at all. The shipped global default list
(**/build/**, **/vendor/**, **/*.lock) covers the common cases, so the harm is limited to
novel directory names a repository excludes (generated/, payments/, fixtures/, testdata/).

Decision (maintainer): make the code match the docs, not walk back the promise. Normalize a
per-repo pattern before compiling — strip a trailing / and append /**; when a pattern contains no
/, also compile it as **/<pattern> (the same helper the **/ prefix already uses). This extends
the existing **/-suffix fallback, which already papers over one of the four idioms. Surface unmatched
declarations too: a one-line note in the review summary ("N declared ignore globs matched no file in
this PR") turns a silent miss into a fixable one.

2. Two more silent-drop paths in the same parser

  • A scalar ignored-files value is comma-split (RepoSettingsParser), which destroys a brace
    glob: ignored-files: "**/*.{js,ts}" becomes [**/*.{js, ts}], and both fragments then fail to
    compile and are dropped. The deployment-side env-var list is comma-separated too and has the same
    hazard.
  • A single duplicate key anywhere in the file (setAllowDuplicateKeys(false)) throws and
    discards every setting, warn-logged only — a far wider blast radius than the parser's otherwise
    careful per-entry "one bad entry costs only itself" design.

3. YAML anchors/aliases are not resolved

jackson-dataformat-yaml never runs snakeyaml's Composer, so *common reaches the parser as the
literal string "common". An aliased ignore list becomes a glob matching a file named common; an
aliased scope's instructions becomes a short literal string that passes every parser check and is
rendered into the review prompt as the repository's rule for those files. Merge keys (<<: *base)
fail closed (the entry loses its instructions and is dropped, with a warning). Fix: either reject a
document containing &/* anchors with a clear warning, or pre-resolve with snakeyaml's Composer
path and hand the object graph to Jackson's convertValue.

4. Two snakeyaml loader guards are inert, and their javadoc claims protection that does not exist

setNestingDepthLimit(20) and setMaxAliasesForCollections(50) are enforced in the Composer, which
Jackson never runs — they do nothing. The real nesting bound is jackson-core's StreamReadConstraints
default of 1000, which is caught and degrades to EMPTY, so this is a false invariant, not an
exploitable hole. MAX_NESTING_DEPTH / MAX_ALIASES are dead constants whose comments would let a
future reader believe the parser is hardened where it is not. Delete them (or set the depth via
StreamReadConstraints), and fix the comments. Coupling note: if #3 is fixed by actually
resolving aliases, setMaxAliasesForCollections becomes load-bearing — fix them together.

5. Transient GitHub failure is negative-cached like a 404 (narrower, related)

RepoSettingsResolver.fetchAndParse catches WebApplicationException | ProcessingException and
returns null; WebApplicationException is the parent of ServerErrorException (500/502/503) and
ClientErrorException (403 secondary rate limit), so a transient failure is indistinguishable from a
real 404 and gets RepoSettings.EMPTY cached for NEGATIVE_CACHE_TTL_MS (60s). For that window every
review of the repo runs on the deployment ignore list only. Fix: catch NotFoundException separately
and negative-cache only that; for any other failure return without writing the cache, and log at
warn rather than the current "config file not found" debug.

Acceptance criteria

  • A per-repo pattern of build/, bare vendor, *.lock, or generated/** excludes what a
    gitignore-literate maintainer expects — with tests written the way a maintainer writes them
    (not **/-prefixed)
  • A brace glob and a scalar comma-list are handled without silently dropping fragments
  • A duplicate key degrades to a per-entry drop, not a whole-file discard
  • A YAML alias either resolves correctly or is rejected with a clear warning
  • The inert nesting/alias guards are removed or made real, and their comments are honest
  • A transient 5xx/403 does not get negative-cached as "no config"
  • Unmatched ignore globs are disclosed to the maintainer rather than silently ignored

Environment

All present on release/v0.6.0. The gitignore-syntax wording and the RepoSettings.path javadoc are
new this milestone; the parser guards and the negative-cache behavior predate it. Found in the v0.6.0
deep audit; deferred from the release under the "regressions + live security ship now, latent hardening
to v0.7.0" scope decision.

Code of Conduct

  • I have searched for existing issues and this is not a duplicate.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingjavaPull requests that update java codereworkDefect introduced during this release cycle, not pre-existingtriage

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions