fix(ci): write the Sonar exclusions as path patterns, not directory prefixes - #26
Merged
amondnet merged 5 commits intoSep 16, 2026
Conversation
…refixes `sonar.exclusions=skills/spring-docs/scripts/` (ee09832) matched nothing. SonarCloud matches these values against whole file paths, so a bare directory prefix is discarded in silence: `main` still indexed 5,018 lines of generated bundle and carried 359 of its 412 open findings inside the directory the exclusion was supposed to cover — 332 of them `fast-xml-parser`'s entity tables, inlined into `detect.mjs` and uneditable because `build:skill:check` byte-compares the bundle against a fresh build. The file's own header caused it: it read the Automatic Analysis page's "Wildcard patterns are not allowed" as covering every value here. That line is about the plain path lists `sonar.sources` and `sonar.tests`; the exclusion properties take patterns, and a whole subtree is written `dir/**/*`. Header corrected alongside the values. `.codacy.yml` needed no change — `skills/*/scripts/**` is already Java glob and Codacy reports 0 of its 211 findings under `skills/`. Add a test asserting each configured pattern matches the committed bundles. A pattern that matches nothing is indistinguishable from one that works by reading it, which is how this survived review once already. Refs #23
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates .sonarcloud.properties to use wildcard patterns for excluding generated bundles, correcting an issue where the previous bare directory prefix was ignored. It also introduces a new test suite (generated-bundle-exclusions.test.ts) to ensure that Sonar, Codacy, and ESLint exclusions correctly cover the committed bundles. The review feedback suggests improving the test suite by using the standard options object with cwd in Bun.Glob.scanSync for better type safety, and handling CRLF line endings and whitespace in the properties parser to ensure cross-platform reliability on Windows.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 22 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
amondnet
marked this pull request as ready for review
September 16, 2026 14:18
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Three review findings on the exclusion guard, all on the same weakness — the
test read configuration as text, and text cannot tell an active setting from a
dead one.
- Read the *active* exclusion, not the file's raw text (gpt, important). A
commented-out `exclude_paths` entry or eslint `ignores` line still contains a
pattern that matches the bundles, so the guard passed while the analyser
received nothing — defeated by exactly the edit it exists to catch. ESLint's
ignores now come off the evaluated config; the other two off a line a leading
`#` disqualifies. Verified by mutation: commenting out either entry, or
restoring the old bare prefix, fails the guard.
- Parse the properties file CRLF-safely (gemini-code-assist). No
`.gitattributes`, so a Windows checkout with `core.autocrlf=true` left a `\r`
on every pattern and failed the test spuriously.
- Correct the stale guidance in `.please/docs/knowledge/gotchas.md` (greptile).
It still carried the "wildcards are not allowed" misreading that produced the
broken exclusion, which is how a future maintainer reintroduces it.
Reading the configs through Bun's file and glob APIs also clears the two Codacy
findings this PR added (`no-non-null-assertion`, `detect-non-literal-fs-filename`).
Not applied: `scanSync({ cwd: ROOT })` (gemini-code-assist). Bun types the
string form itself — `scanSync(optionsOrCwd?: string | GlobScanOptions)`.
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request corrects the SonarQube Cloud exclusion patterns to use proper wildcards instead of bare directory prefixes, and adds a new test suite to verify that analyzer exclusions in .sonarcloud.properties, .codacy.yml, and eslint.config.js actually match the committed bundles. The review feedback identifies two opportunities to reduce test brittleness by making the parsing of .sonarcloud.properties and .codacy.yml more robust against formatting variations like whitespace and double quotes.
Codacy's ESLint runs its own rule set, so the previous commit traded two of its findings for two others: `no-unnecessary-condition` on `entry[1] ?? ''` (its type view lacks this repo's `noUncheckedIndexedAccess`, so the `??` reads as dead), and `no-unsanitized/method` on a dynamic `import()` with a computed specifier. Both go away without weakening the check. The Codacy block parser matches the quoted entry with `startsWith`/`endsWith` instead of a capture group, and the ESLint ignores are read as text with `//` lines dropped rather than by importing the module — a literal-specifier import would satisfy the rule but needs `allowJs` in `tsconfig.json`, which is a project-wide change to suit one test. Dropping comment lines is what closes the gap gpt found; evaluating the module would only have added coverage for a pattern built at runtime, which this config does not do. Mutation-checked: the bare `skills/spring-docs/scripts/` prefix, a commented-out `.codacy.yml` entry, and a commented-out eslint `ignores` line each fail the guard.
gemini-code-assist flagged both readers as brittle, and it is right: neither misreads a config, but each fails the whole guard on a formatting change that leaves the configuration valid. `.properties` permits whitespace around the `=` and before the key, and YAML permits double-quoted or unquoted scalars, so the sonar reader now anchors on a whitespace-tolerant pattern and the Codacy reader strips whichever quoting style it finds. Implemented without the suggested capture-group regex, which would have reintroduced the `entry[1] ?? ''` that Codacy's `no-unnecessary-condition` rejected one commit ago. Mutation-checked, five scenarios: spaces around `=` and double-quoted YAML now pass; the bare `skills/spring-docs/scripts/` prefix, a commented-out `.codacy.yml` entry, and a commented-out eslint `ignores` line still fail.
Building the key matcher with `new RegExp` tripped three Codacy rules at once (`security/detect-non-literal-regexp`, `security-node/non-literal-reg-expr`, `javascript_dos/rule-non-literal-regexp`), all on the constructor taking a template literal. Splitting each line on its first `=` and comparing the trimmed key needs no regex at all, and it is the simpler statement of the same intent: it still tolerates whitespace around the separator and before the key, and a commented-out setting still keeps its `#` inside the key, so it never compares equal. No regex means this class of rule cannot fire here again. Mutation-checked, seven scenarios. Tolerated: whitespace around `=`, a leading indent, double-quoted YAML. Caught: the bare `skills/spring-docs/scripts/` prefix, and a commented-out setting in each of the three configs.
|
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.
Closes #23
What was wrong
sonar.exclusions=skills/spring-docs/scripts/, added in ee09832, matched nothing. SonarCloud matches these values against whole file paths, so a bare directory prefix is discarded in silence — no warning, no unmatched-pattern report.Measured on
mainat 314d38d, viaapi/issues/searchandapi/measures/component_tree:mainskills/spring-docs/scripts/nclocattributed to the two bundles332 of those findings are
fast-xml-parser's HTML entity tables, inlined intodetect.mjsby the bundler. None is editable:bun run build:skill:checkbyte-compares the committed bundle against a fresh build, so any hand-edit fails CI.Root cause
The file's own header caused it. It read the Automatic Analysis page's line "Wildcard patterns are not allowed" as covering every value in the file, and wrote the exclusions as plain prefixes on that basis. That line is about the plain path lists
sonar.sourcesandsonar.tests; the exclusion properties take path patterns, and the pattern docs are explicit that a whole subtree is writtendir/**/*and that a bare prefix does not work.The header is corrected alongside the values, since the wrong reading is what has to not survive.
Codacy needs no change
The issue asked for
.codacy.ymlto be checked the same way. It is already correct:exclude_paths: ['skills/*/scripts/**']is Java glob, wheredir/**does cover everything beneath. Codacy's API reports 0 of its 211 open findings underskills/. PR #21's red Codacy check was the genuinescripts/docs.tsbug that #25 then fixed — the exclusion working, not failing.Guard
scripts/__tests__/generated-bundle-exclusions.test.tsasserts that every exclusion pattern configured in.sonarcloud.properties,.codacy.ymlandeslint.config.jsactually matches the committed bundles. Revertingsonar.exclusionsto the old prefix fails it; that is the check review did not have the first time, when the config read as correct.Bun.Globstands in for three matchers it is not, so a pass is not proof that SonarCloud reads the pattern identically — but it does catch the failure that actually happened, a pattern matching no file at all.Verification
Local:
typecheck,lint,build:skill:check, 241 tests — all pass.Remote is only half-answerable before merge, and this is the part of the issue's Definition of Done this PR cannot close on its own. Automatic Analysis reads
.sonarcloud.propertiesfrom the default branch, so this PR's own SonarCloud check still runs under the broken prefix. After merge,main's next analysis should drop to ~53 findings and ~6kncloc:curl -sS 'https://sonarcloud.io/api/issues/search?componentKeys=pleaseai_spring-plugin&branch=main&resolved=false&ps=500'I will confirm that number on
mainand post it to #23 before the issue is closed.Summary by cubic
Fixes #23 by replacing the inert Sonar exclusions
skills/spring-docs/scripts/withskills/*/scripts/**/*, so generated bundles are excluded from issue and CPD analysis. Automatic Analysis reads.sonarcloud.propertiesfrommain, so this PR's Sonar check still uses the old setting until merge.Changes
After merge
mainanalysis drops from about 412 to 53 findings and no longer counts roughly 5,018 generated lines before closing fix(ci): generated skill bundles still analysed by SonarCloud despite the exclusion #23.Written for commit 76599f8. Summary will update on new commits.