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
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
Problem Statement
An audit of the v0.6.0 repo-settings subsystem (
#51per-repo ignore globs,#33path-scopedinstructions) 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.0and independentlyvalidated. 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) andRepoSettings's javadoc tell maintainersper-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 whenthe pattern starts with
**/. Under that matcher, gitignore idioms silently match nothing:build/(trailing slash = directory)vendor/payments*.lock*.lockgenerated/****/prefix)compileGlobMatcherswarns only on patterns that fail to compile (InvalidPathException/PatternSyntaxException);build/compiles fine, so there is no signal at any log level, and apath-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 tonovel 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 extendsthe existing
**/-suffix fallback, which already papers over one of the four idioms. Surface unmatcheddeclarations 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
ignored-filesvalue is comma-split (RepoSettingsParser), which destroys a braceglob:
ignored-files: "**/*.{js,ts}"becomes[**/*.{js, ts}], and both fragments then fail tocompile and are dropped. The deployment-side env-var list is comma-separated too and has the same
hazard.
setAllowDuplicateKeys(false)) throws anddiscards 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-yamlnever runs snakeyaml'sComposer, so*commonreaches the parser as theliteral string
"common". An aliased ignore list becomes a glob matching a file namedcommon; analiased scope's
instructionsbecomes a short literal string that passes every parser check and isrendered into the review prompt as the repository's rule for those files. Merge keys (
<<: *base)fail closed (the entry loses its
instructionsand is dropped, with a warning). Fix: either reject adocument containing
&/*anchors with a clear warning, or pre-resolve with snakeyaml's Composerpath 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)andsetMaxAliasesForCollections(50)are enforced in the Composer, whichJackson never runs — they do nothing. The real nesting bound is jackson-core's
StreamReadConstraintsdefault of 1000, which is caught and degrades to
EMPTY, so this is a false invariant, not anexploitable hole.
MAX_NESTING_DEPTH/MAX_ALIASESare dead constants whose comments would let afuture 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#3is fixed by actuallyresolving aliases,
setMaxAliasesForCollectionsbecomes load-bearing — fix them together.5. Transient GitHub failure is negative-cached like a 404 (narrower, related)
RepoSettingsResolver.fetchAndParsecatchesWebApplicationException | ProcessingExceptionandreturns
null;WebApplicationExceptionis the parent ofServerErrorException(500/502/503) andClientErrorException(403 secondary rate limit), so a transient failure is indistinguishable from areal 404 and gets
RepoSettings.EMPTYcached forNEGATIVE_CACHE_TTL_MS(60s). For that window everyreview of the repo runs on the deployment ignore list only. Fix: catch
NotFoundExceptionseparatelyand negative-cache only that; for any other failure return without writing the cache, and log at
warnrather than the current "config file not found"debug.Acceptance criteria
build/, barevendor,*.lock, orgenerated/**excludes what agitignore-literate maintainer expects — with tests written the way a maintainer writes them
(not
**/-prefixed)Environment
All present on
release/v0.6.0. The gitignore-syntax wording and theRepoSettings.pathjavadoc arenew 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